-
Notifications
You must be signed in to change notification settings - Fork 6.3k
feat(ci): armv7 cross builds #3892
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report
Merging #3892 (b3df83f) into main (ff3b188) will not change coverage.
The diff coverage isn/a.
❗ Current head b3df83f differs from pull request most recent head 1da2558. Consider uploading reports for the commit 1da2558 to get more accurate results
Impacted file tree graph
@@ Coverage Diff @@ ## main #3892 +/- ## ======================================= Coverage 63.51% 63.51% ======================================= Files 36 36 Lines 1872 1872 Branches 379 379 ======================================= Hits 1189 1189 Misses 580 580 Partials 103 103
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered by Codecov. Last update ff3b188...1da2558. Read the comment docs.
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.
Overall, I don't see anything concerning here. Nice work 👏
0f1c434 to
06466f4
Compare
@jawnsy
jawnsy
Aug 7, 2021
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.
just a minor style thing, I wonder if this would be more (or less) readable as:
env: AR: ${{ matrix.prefix }}-ar
what do you think?
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.
^that looks a little easier to read IMO
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.
Oh! Didn't know I could do it this way :o
TIL
@jawnsy
jawnsy
Aug 7, 2021
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.
Is it possible to install the "latest" node? The rest of the build scripts seem to bundle the latest Node 14.x, which would help us avoid security problems.
I don't have strong feelings about pinning specific versions or using the "latest" minor/patch release, but I do think we should be consistent with the versions we're using between the native vs cross-compiled builds
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.
Unfortunately, there's no stable URL I can point to for latest node, which is why I resolved to this :(
Even in https://nodejs.org/dist/latest-v14.x/ - you need to know the latest version to have the right URL.
@jawnsy
jawnsy
Aug 9, 2021
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.
Apparently, they publish a JSON file, so we can write a script to extract the latest version using jq or whatever: https://nodejs.org/dist/index.json
it seems... unfortunate, that there doesn't seem to be an easier way to do this
anyway, shouldn't let this hold up this PR, this LGTM
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.
Wondering if we can explain why we want to to go with the arch-override.json approach instead of providing a function and returns values based on input.
I'd rather us do that then add more files like this json file but open to hearing why the current approach is a better approach
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.
I worry about adding an extra file just because it seems like more to maintain. Also the json means we can't really put comments to explain why we have this. Any thoughts on writing a function to pass in rpm or deb and then returning values instead?
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.
My reasoning to put this in a separate file is that its easier to add overrides for other architectures should we need it in the future (say powerpc/s390x) and that it also separates code (building packages) from data (what overrides to apply when)
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.
Ah, okay that makes sense then :) Let's leave as is!
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.
Maybe we could add a comment above explaining why we have to override the arch? Or why we have this function?
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.
Sure! I can do that - just give me a minute.
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.
Only request then is to add a comment or two explaining why we have to override the arch and then we should approve and merge!
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.
Done!
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.
Hopefully should cross-compile and work for armv7!
Fixes #1337.