-
Notifications
You must be signed in to change notification settings - Fork 200
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.
LGTM
Would add some docs about this btw like the example mentioned here |
@@ -108,6 +108,12 @@ async function compileOnUnix(nodeVersion: string, targetArch: string) { | |||
args.push('--dest-cpu', cpu); | |||
} | |||
|
|||
if (hostArch !== targetArch) { | |||
log.warn('Cross compiling!'); | |||
log.warn('You are responsible for appropriate env like CC, CC_host, etc.'); |
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.
This should link out to more information. What is CC, CC_host? Where can I learn more?
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 is no documentation there. Even Node.js does not provide one on the topic of cross-compilation.
CC is "C Compiler".
CC_host is "C Compiler for the host".
Users are kinda expected to know this, if they wish to take advantage of this more advanced option. Of course, I am always open to better docs, but I don't think I can do a "cross compile 101" here.
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 instead of making it working by default and show a warning would be better to force the flag to be there, if it is not there throw like it was before
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.
It would fail (very soon). So there is no need for that. That would also drastically increase the complexity of matrix-based CI workflows, as we probably don't want the flag when compile natively.
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.
ok LGTM for me so
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.
Let's add the documentation to the README then, and then link to the readme in the error message?
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.
and, FYI: upstream (Node.js) builds and tests natively. See https://github.com/nodejs/build . They have native machines donated by IBM, ARM, etc. That explains why there is very little documentation on cross compilation.
There is no guaranteed good documentation here. Cross compilation is quite complicated. For instance, GCC vs LLVM: GCC uses separate binaries but LLVM uses --target=<triple>
and other flags. And, the instructions for how to install a cross toolchain obviously vary a lot between distros.
I think it would be challenging to add any documentation, and we should probably follow the example of upstream, and ask advanced users to explore the codes, workflows and Google for answers instead.
I am merging this for now, so we can start to make binaries. It works here (my local machine, Ubuntu, for arm64) and there (https://github.com/jesec/pkg-fetch/actions/runs/710314809, Windows, for arm64). I am of course always open to better docs and we should do that in a later change. |
To test on Ubuntu (amd64 host):