Skip to content
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

Add cross-compilation options only when requested #3275

Merged
merged 2 commits into from
Nov 20, 2019
Merged

Add cross-compilation options only when requested #3275

merged 2 commits into from
Nov 20, 2019

Conversation

jsarenik
Copy link
Collaborator

@jsarenik jsarenik commented Nov 19, 2019

Requesting them is done by setting BUILD=<target_arch>

Otherwise autotools (used by external dependencies like
libsodium) is not happy with setting cross-compilation
variables and may possibly lead to unexpected results.

Changelog-None

Requesting them is done by setting BUILD=<target_arch>

Otherwise autotools (used by external dependencies like
libsodium) is not happy with setting cross-compilation
variables and may possibly lead to unexpected results.
@jsarenik
Copy link
Collaborator Author

Sorry about this, I held it for too long. It was fresh long time ago, when I first started poking into c-lightning. Happy to see it unresolved yet, I decided to send this PR :)

Copy link
Contributor

@rustyrussell rustyrussell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack 6d9471c

@cdecker
Copy link
Member

cdecker commented Nov 20, 2019

Thanks @jsarenik for the PR. Seems quite all right but I'd like to get @rustyrussell's sign-off since he's our resident Makefile expert. (need to read the comments before commenting myself...)

One thing that seems a bit strange is the naming of the variables. I think TARGET might be a better name for the BUILD variable, and the MAKE_HOST_ADD might be better called CROSSCOMPILEOPTS since we usually have names like XYZOPTS for things that get added to the command lines.

@jsarenik
Copy link
Collaborator Author

Thanks @jsarenik for the PR. Seems quite all right

One thing that seems a bit strange is the naming of the variables. I think TARGET might be a better name for the BUILD variable, and the MAKE_HOST_ADD might be better called CROSSCOMPILEOPTS since we usually have names like XYZOPTS for things that get added to the command lines.

Sure. Thank you for feed-back. I agree with CROSSCOMPILEOPTS, but the TARGET is a bit tricky. For one thing, it was BUILD variable used in c-lightning since some time ago so such a change could be considered as a regression for some. And BUILD also seems correct from other point of view:

See Autoconf documentation on this topic which nicely explains how it is meant to be. Generally, I would change even more to not use the $MAKE_HOST variable by not setting --host at all. Just --build as the documentation clearly explains:

They all default to the result of running config.guess, unless you specify either --build or --host. In this case, the default becomes the system type you specified. If you specify both, and they're different, configure enters cross compilation mode, so it doesn't run any tests that require execution.

So now I will change the patch from MAKE_HOST_ADD to CROSSCOMPILEOPTS and wait for @rustyrussell to decide on whether to leave --host together with ${MAKE_HOST} or whether to drop it. I am all for dropping it and leaving only --build and will happily do all the needed testing to (dis)prove it.

Just for reference: Another solid source from GNU Automake docs

@jsarenik
Copy link
Collaborator Author

jsarenik commented Nov 20, 2019

Just a note: doc/INSTALL.md might need to be changed, I am going to test it all now. First unchanged, just following the docs.

@cdecker
Copy link
Member

cdecker commented Nov 20, 2019

Great, thanks @jsarenik, I hadn't considered that BUILD is already part of the external interface, thus changing would be breaking things 👍

@jsarenik
Copy link
Collaborator Author

...I hadn't considered that BUILD is already part of the external interface, thus changing would be breaking things

Maybe it is not really. It is referenced only once in the doc/INSTALL.md.

@jsarenik
Copy link
Collaborator Author

To refresh memories, I went through #1533

@cdecker
Copy link
Member

cdecker commented Nov 20, 2019

ACK 635a9d7

@cdecker cdecker merged commit 170918f into ElementsProject:master Nov 20, 2019
@jsarenik jsarenik deleted the jsn/cross_only_vars branch November 20, 2019 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants