Uh oh!
There was an error while loading. Please reload this page.
Conversation
Codecov Report
@@ 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.
|
jsjoeio
left a comment
There was a problem hiding this comment.
Overall, I don't see anything concerning here. Nice work 👏
0f1c434 to
06466f4Compare| CXX: aarch64-linux-gnu-g++ | ||
| LINK: aarch64-linux-gnu-g++ | ||
| NPM_CONFIG_ARCH: arm64 | ||
| AR: ${{ format('{0}-ar', matrix.prefix) }} |
There was a problem hiding this comment.
just a minor style thing, I wonder if this would be more (or less) readable as:
env:
AR: ${{ matrix.prefix }}-arwhat do you think?
There was a problem hiding this comment.
^that looks a little easier to read IMO
There was a problem hiding this comment.
Oh! Didn't know I could do it this way :o
TIL
| CXX: ${{ format('{0}-g++', matrix.prefix) }} | ||
| LINK: ${{ format('{0}-g++', matrix.prefix) }} | ||
| NPM_CONFIG_ARCH: ${{ matrix.arch }} | ||
| NODE_VERSION: v14.17.4 |
There was a problem hiding this comment.
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.
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.
There was a problem hiding this comment.
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
jsjoeio
left a comment
There was a problem hiding this comment.
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
| "rpm": { | ||
| "armv7l": "armhfp" | ||
| }, | ||
| "deb": { | ||
| "armv7l": "armhf" |
There was a problem hiding this comment.
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.
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.
Ah, okay that makes sense then :) Let's leave as is!
| get_nfpm_arch() { | ||
| if jq -re ".${PKG_FORMAT}.${ARCH}" ./ci/build/arch-override.json > /dev/null; then | ||
| jq -re ".${PKG_FORMAT}.${ARCH}" ./ci/build/arch-override.json | ||
| else | ||
| echo "$ARCH" | ||
| fi | ||
| } | ||
There was a problem hiding this comment.
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.
Sure! I can do that - just give me a minute.
There was a problem hiding this comment.
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!
Hopefully should cross-compile and work for armv7!
Fixes#1337.