-
Notifications
You must be signed in to change notification settings - Fork 34
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not much of my business :-) and most likely I'm missing something, but why is this PR necessary? The way I see it, it's 172 lines of build-stubs.yml
versus the following 6 lines executed on any Intel machine, any OS, without the need for docker or QEMU:
BIN=windows.exe; GOOS=windows go build -o $BIN stub.go && echo >> $BIN && echo "### CAXA ###" >> $BIN
BIN=macos; GOOS=darwin go build -o $BIN stub.go && echo >> $BIN && echo "### CAXA ###" >> $BIN
BIN=linux; GOOS=linux go build -o $BIN stub.go && echo >> $BIN && echo "### CAXA ###" >> $BIN
BIN=linux-armv6; GOOS=linux GOARCH=arm GOARM=6 go build -o $BIN stub.go && echo >> $BIN && echo "### CAXA ###" >> $BIN
BIN=linux-armv7; GOOS=linux GOARCH=arm GOARM=7 go build -o $BIN stub.go && echo >> $BIN && echo "### CAXA ###" >> $BIN
BIN=linux-arm64; GOOS=linux GOARCH=arm64 go build -o $BIN stub.go && echo >> $BIN && echo "### CAXA ###" >> $BIN
OK, I see the value in making sure that the stub binaries are up to date in relation to the Go source code, and the fact that a binary commit from some random contributor cannot be easily reviewed for including a bitcoin miner, :-) but then maybe the workflow could be just ubuntu-20.04
on Intel, running the 6 lines above?
caxa
's Readme makes a point about not supporting cross compilation, but I understand that's about native Node.js modules of the end user's application, not caxa
's own stubs. After all, the binary stubs for all platforms are even committed to the GitHub repo.
The main point of this PR is to put stub compilation in the CI/CD pipeline. I was avoiding cross-compilation because the repo owner seemed not to be a fan of it as you pointed out. From the Readme:
Using your cross-compilation script would certainly simplify the workflow, but puts all your trust in the go cross-compiler. Emulation at compilation is more complex but also ensures the build environment is closest to the actual hardware. Pick your poison, I guess 🤷
This is really what I'm aiming to change. Committing binaries to a git repo is a bit sketchy in my opinion. |
Hi all, I’m just checking in to you know that I’m going to work on this soon. I maintain several open-source projects and go around spending some time on each; caxa is up next. I also started streaming my open-source work, which you may follow here: https://www.youtube.com/channel/UC_R-6HcHW5V9_FlZe30tnGA
What makes you think that? How’s a CI workflow better? Reasons why I like stubs in the Git repository:
|
Playing devil's advocate... I see pros and cons.
There may be room for simplification as I pointed out in my previous comment, but:
Each. 7 x 3MB = 21MB (7 counting windows, mac-intel, mac-arm, linux-intel, linux-armv6, linux-armv7, linux-arm64). This will show in the "unpacked size" entry at https://www.npmjs.com/package/caxa and the git repo will keep the history: one-line changes to
Even so, transparency could be increased by adopting a GitHub workflow. You might be able add a feature bullet point to the Readme: "binary stubs reliably generated with GitHub workflows", with a link to
I'd say that's fine until there is evidence to the contrary: "Add complexity only when justified". |
Git was designed to handle version control of text files. The workflow at least shows the whole process of generating the binaries. Anyone would be able to see the exact commit of the source and the whole build process. I can include md5sums in the build process as well to mitigate administrator abuse. Of course you can never 100% trust binaries from the internet, but you can take steps to help people I think the current repo structure of |
I'll add a simple smoke test.
That's a totally fair point. I think adding tests of the stubs to the workflow would justify the complexity though. |
You do make some good points and I’ll give a more detailed answer in the near future. For now, a couple quick comments:
Oh, a teachable moment 😀 In any case, that’s not central to your argument: You’re right that storing blobs will blow up the repository’s size.
I believe it amounts to the same as Line 23 in 3097992
|
I stand corrected 😄
A workflow shows the user the build log that produced the binary files. The Here is the build log I am talking about. You can trace the process from source code all the way to build artifact. |
As far as binary distribution goes, I think removing them from the repo is a net positive. The
|
b697c77
to
4129720
Compare
I've added some new features to this that may persuade you of its usefulness.
|
I changed this to cross-compile all the binaries and then test them natively or emulated in the case of arm architecture. |
@leafac I just watched your stream on caxa maintenance. It was good to see your vision of where caxa should be heading! As far as the method of compilation goes, this PR is currently cross-compiling the stubs and then testing them in native or emulated environments. If you would prefer they be compiled in a native/emulated environment, I can easily revert back to that. I'd say it's a judgement call up to you which method should be used. I hope this PR takes compiling and distributing the stubs off your plate. |
@maxb2: You’re doing a fantastic job on this pull request. Thank you very much. And thanks for joining me on the stream today. Do you want to be a guest in tomorrow’s stream at 17:30 UTC? I can call you and we can have a pairing session in which we review your contribution and merge it. If you’re interested, please contact me on [email protected] and we can arrange the details. |
Here is an updated overview of this PR as of efbcc02 Features
Special Considerations
Future Work (Separate PR)
|
I thought a little more about using node to perform the binary tests. There isn't any reason that the docker image used to compile the binary has to be the same that tests it. In short, compile the binary with a golang image, copy the binary into the node image and run the test.
This is currently failing with the same error as the main workflow: https://github.com/leafac/caxa/runs/2138993751?check_suite_focus=true We could also test the binary against multiple node versions like in the main workflow by pulling the appropriate images. This may have to wait until the javascript side of caxa is aware of the arm binaries. |
Check this out: https://github.com/maxb2/caxa/runs/2641839545?check_suite_focus=true#step:3:6 Fun stuff, huh? |
Update
This PR has evolved a lot since it was opened. See the newer comments below.
Original
This adds a workflow that compiles the stubs and deploys them to a Release page. Related #4. This runs on both
master
and tags. If the run is on a tag, it also creates a draft release with the stubs uploaded automatically. A maintainer then has to go to the releases page and finalize the release. See an example release here.I adapted the workflow from dungeon-revealer. I wasn't sure how you would want to package the stubs so I left it like the previous project.
Here is the output of the command
file
on each of the stubs:TODO: