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 macro compile options #9425

Closed

Conversation

dburnsii
Copy link

@dburnsii dburnsii commented Jun 4, 2020

Compiling Macros within Crystal currently uses system default compile options, not using the options passed to the compiler itself. In order to be compatible with confined build systems like Yocto, the compiler options should be respected across the board, so that even the macros only use libraries explicitly provided by that system if needed.

Concretely, Yocto loads all dependencies (i.e. libraries, binaries, etc.) for it's packages into a folder, and any executable built within the system should only use the provided files, to ensure build portability and repeatability.

@asterite
Copy link
Member

asterite commented Jun 4, 2020

I think the intention here is good, but I'm not sure it's correct.

When cross compiling you have macros run in the host system, but the code is generated for a different target system. The flags of the compiler will be relative to the target system, and it's incorrect to set those too for the host system (the macro run code won't compile).

So I think we should only do this if we are not cross-compiling. Basically, cross-compiling and macro run code are not compatible with each other... I think.

About link flags, maybe it's fine to pass those along, I'm not sure.

Is this for macro run?

@jhass
Copy link
Member

jhass commented Jun 4, 2020

Yes this is for macro run. Also LGTM, except for skipping the pass when cross compiling, including link flags. So stay as is when in cross compile mode.

@straight-shoota
Copy link
Member

This might be fine as a first step, but for cross compilation, target configuration needs to be independent of host configuration. So there needs to be a way to specificy compiler options for the host system explicitly.
This is similar to the discussion in #7196 (comment) ff.

@straight-shoota
Copy link
Member

This is a proposal for a proper implementation: We need specific compiler options for the host system (used for macro run). They could be specified as command line arguments (exactly the same as the existing options but with --host prefix). An environment variable CRYSTAL_HOST_OPTS would supplement that.
Unless the compiler does a cross-compile, these options would generally default to the regular compiler options. And you can specify different options explicitly.
For a cross compile, regular compiler options should not be used as defaults for host options.

We probably don't need to implement the whole host options thing right now. A simple and quick fix like this to just use the target options as defaults is a good start.

I don't think assigning link_flags to the program is a good solution, though. It's just one config setting. There are others that would also need to be considered. Instead, the entire compiler config should be attached to the program. That means a compiler instance. It could probably directly be a special instance for host compilation that can be reused. I'm not sure why macro_compile currently creates a new compiler instance every time. As far as I can see, it should be easily reusable.

@jhass
Copy link
Member

jhass commented Nov 15, 2020

Maybe --cross-compile should move from build to its own subcommand. Having --host- options sounds good until it's confusing to specify them outside a cross compile. Moving to crystal cross-compile (maybe crystal cross-build for better naming consistency) gives us a nice place to add the --host options only there without polluting crystal build. On crystal cross-build the existing options could even be prefixed with --target- for extra clarity.

More on topic, using default compile options for macro runs when in cross compile mode for the moment sounds good to me, until we solve this holistically. Using above approach or something else.

@straight-shoota
Copy link
Member

Closing. The intended use case should be resolved by #9920

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants