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 rustc-bin-link-arg and rustc-link-arg custom build options #7811

Open
wants to merge 2 commits into
base: master
from

Conversation

@npmccallum
Copy link

npmccallum commented Jan 17, 2020

Both are similar to rustc-cdylib-link-arg. The rustc-bin-link-arg option adds -C link-arg=... on binary targets. The rustc-link-arg option adds -C link-arg=... on all supported targets (currently only binaries and cdylib libraries).

@rust-highfive

This comment has been minimized.

Copy link

rust-highfive commented Jan 17, 2020

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@npmccallum

This comment has been minimized.

Copy link
Author

npmccallum commented Jan 17, 2020

@npmccallum npmccallum force-pushed the npmccallum:linkarg branch from 445dec7 to ee5c7e7 Jan 17, 2020
@npmccallum

This comment has been minimized.

Copy link
Author

npmccallum commented Jan 17, 2020

@ehuss @alexcrichton FYI, the Enarx project needs this for our microkernels.

@npmccallum

This comment has been minimized.

Copy link
Author

npmccallum commented Jan 17, 2020

@joshtriplett Tagging you due to your comment here: #6298 (comment)

@joshtriplett

This comment has been minimized.

Copy link
Member

joshtriplett commented Jan 18, 2020

I like this very much, and it mostly looks reasonable to me. I don't think it's worth pulling in a new dependency just for this, though. Would you consider making a version that doesn't pull in bitflags, and just uses an enum? I would happily propose such a version for merge. (This introduces new interfaces, so I would want to get team consensus.

@npmccallum npmccallum force-pushed the npmccallum:linkarg branch 3 times, most recently from 66d69b6 to 2a66af5 Jan 18, 2020
@npmccallum

This comment has been minimized.

Copy link
Author

npmccallum commented Jan 18, 2020

@joshtriplett I pushed a version that does not add a bitflags dependency and uses instead a naked enum. I believe it is ready for review.

@joshtriplett

This comment has been minimized.

Copy link
Member

joshtriplett commented Jan 18, 2020

Looks good to me. Confirming consensus for the new interface:

@rfcbot merge

@rfcbot

This comment has been minimized.

Copy link
Collaborator

rfcbot commented Jan 18, 2020

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 21, 2020

I think before merging this it would be good to add some tests. Additionally, to confirm, this does not transitively apply throughout the crate graph, right? Only for one particular package?

Also, we initially hesitated in adding this degree of custom link arguments for disambiguation reasons. For example there's no way to pass custom link args to just one binary in a multi-binary project. Similarly can you clarify the use cases of rustc-link-arg? When would you want to pass it to all linked artifacts? Additionally another concern is what to do about other sorts of native binaries. For example what about compiled unit tests? Unit tests compiled by rustdoc? (etc)

I personally think that we'll want to add the ability to pass custom link arguments for just one binary at a time, and then having an escape hatch for "pass this everywhere" also seems fine.

@npmccallum

This comment has been minimized.

Copy link
Author

npmccallum commented Jan 22, 2020

@alexcrichton

I think before merging this it would be good to add some tests. Additionally, to confirm, this does not transitively apply throughout the crate graph, right? Only for one particular package?

A test for this (and probably other stuff) would probably be wise. I'm not super familiar with the cargo code base. Could you give me a recommendation of where to start?

Also, we initially hesitated in adding this degree of custom link arguments for disambiguation reasons. For example there's no way to pass custom link args to just one binary in a multi-binary project.

This strikes me as an orthogonal concern. Having a "global args" option doesn't preclude having a "local args" option.

Similarly can you clarify the use cases of rustc-link-arg?

We are producing a kernel. The most important use is a linker script to control layout. We also don't want rustc to strip what it thinks are unused symbols.

We are also producing a specialized ELF loader which needs to ensure its own contents are loaded out of the way. We use a linker script for this.

Someone might theoretically propose an alternative interface for just linker scripts. The problem is that they aren't portable at all. So you're already way into the weeds when you have need for one. The bottom line is that if Rust is a systems language (it is), you need to have control over the linker (is this the right place to complain about rustc forcing the use of -Wl,-Bdynamic on glibc?). That you can shoot yourself in the foot with it is your problem. There isn't really a safe way to do this.

When would you want to pass it to all linked artifacts?

A linker script to strip unwanted sections is something that would be fairly commonly used across all linked artifacts.

Additionally another concern is what to do about other sorts of native binaries. For example what about compiled unit tests? Unit tests compiled by rustdoc? (etc)

Although there may be need for this, I'm not aware of any.

I personally think that we'll want to add the ability to pass custom link arguments for just one binary at a time, and then having an escape hatch for "pass this everywhere" also seems fine.

As I mentioned above, nothing in this patch prevents this. I simply consider it out of scope of this patch. It is also perfectly possible to split crates to have one binary per crate to work around this problem.

@joshtriplett

This comment has been minimized.

Copy link
Member

joshtriplett commented Jan 22, 2020

We discussed this in the Cargo team meeting, and came to the following conclusions:

  • This needs a bit of further discussion on internals; could you please start a thread on it? (We're not asking for it to be bikeshedded extensively, just asking for it to be looked at by a broader audience to find out if it fits the needs of others.)
  • If you'd like to experiment with this before that discussion concludes, or provide a way for people to experiment with it, then you could make it unstable for a bit, requiring a -Z build-link-arg or similar to enable it. (We could detect that output from the build script, and if the flag is not present, provide a warning if not using the -Z build-link-arg option.) When the discussion concludes, we could drop that flag and process this build script output by default.

Does that sound reasonable, as a path forward?

@joshtriplett

This comment has been minimized.

Copy link
Member

joshtriplett commented Jan 22, 2020

Regarding crates with multiple binaries: I personally think it's reasonable to have an option for all binaries; that doesn't preclude us adding another option for a specific binary in the future.

@npmccallum

This comment has been minimized.

Copy link
Author

npmccallum commented Jan 24, 2020

We discussed this in the Cargo team meeting, and came to the following conclusions:

* This needs a bit of further discussion on internals; could you please start a thread on it? (We're not asking for it to be bikeshedded extensively, just asking for it to be looked at by a broader audience to find out if it fits the needs of others.)

Done: https://internals.rust-lang.org/t/rustc-bin-link-arg-and-rustc-link-arg/11695

* If you'd like to experiment with this _before_ that discussion concludes, or provide a way for people to experiment with it, then you could make it unstable for a bit, requiring a `-Z build-link-arg` or similar to enable it. (We could detect that output from the build script, and if the flag is not present, provide a warning if not using the `-Z build-link-arg` option.) When the discussion concludes, we could drop that flag and process this build script output by default.

If this requires unstable Rust, we don't gain any value from this option since our project does not allow the use of nightly. I also personally dislike this option because it will likely break out of the box rls setups (which will be unable to build projects using this option).

@joshtriplett

This comment has been minimized.

Copy link
Member

joshtriplett commented Jan 24, 2020

npmccallum added 2 commits Jan 17, 2020
Both are similar to rustc-cdylib-link-arg. The rustc-bin-link-arg option
adds -C link-arg=... on binary targets. The rustc-link-arg option adds -C
link-arg=... on all supported targets (currently only binaries and cdylib
libraries).
This hides the new rustc-bin-link-arg and rustc-link-arg build script
configuration items behind an unstable flag.
@npmccallum npmccallum force-pushed the npmccallum:linkarg branch from 75294d9 to ee2870c Jan 24, 2020
@npmccallum

This comment has been minimized.

Copy link
Author

npmccallum commented Jan 24, 2020

@joshtriplett I added -Z extra-link-arg.

@joshtriplett

This comment has been minimized.

Copy link
Member

joshtriplett commented Jan 26, 2020

Looks great, thank you!

I would be in favor of merging this, for experimentation purposes, and opening a tracking issue for future stabilization.

@alexcrichton @ehuss @nrc Does this seem reasonable, with the unstable option added to gate it?

@davidhewitt

This comment has been minimized.

Copy link

davidhewitt commented Jan 27, 2020

FYI this looks like it might solve bytecodealliance/wasmtime#468 (based on the final comment in that thread) and its root issue PyO3/pyo3#341

npmccallum added a commit to npmccallum/enarx that referenced this pull request Jan 27, 2020
Cargo currently cannot handle our needs. It cannot build static binaries
with the standard entry point on glibc. Nor can it build cdylib or
static binaries with a custom entry point on musl. Nor can it build
different crates for different targets in the same workspace.

While we work to resolve these issues in upstream cargo, we need to
ditch the top-level workspace for now. We can revisit this at a later
time.

For more background, see:

  rust-lang/cargo#7811
  rust-lang/cargo#7804
  https://github.com/rust-lang/rfcs/pull/2735/files
npmccallum added a commit to enarx/enarx that referenced this pull request Jan 28, 2020
Cargo currently cannot handle our needs. It cannot build static binaries
with the standard entry point on glibc. Nor can it build cdylib or
static binaries with a custom entry point on musl. Nor can it build
different crates for different targets in the same workspace.

While we work to resolve these issues in upstream cargo, we need to
ditch the top-level workspace for now. We can revisit this at a later
time.

For more background, see:

  rust-lang/cargo#7811
  rust-lang/cargo#7804
  https://github.com/rust-lang/rfcs/pull/2735/files
@npmccallum

This comment has been minimized.

Copy link
Author

npmccallum commented Feb 2, 2020

@joshtriplett Any update on this?

@ehuss

This comment has been minimized.

Copy link
Contributor

ehuss commented Feb 3, 2020

Some minor notes:

  • The documentation should move to unstable.md.
  • I would recommend displaying a warning or error if -Zextra-link-arg is not specified and a build script declares one of these options.
  • I think it would be good to display errors or warnings if these options are emitted for packages that do not have a corresponding cdylib/bin. I think this could help avoid confusion.
  • This still needs some tests. The CONTRIBUTING docs has information on running tests. The support module has docs on writing tests. The build script tests have lots of examples. I would probably put these in a new file, since build_script.rs is crazy long.
  • I don't have a strong preference for choosing which targets this applies to, but I do think it could get unnecessarily complex if more things are piled on afterwards. I think it would be bad if there was a multitude of script commands that all do essentially the same thing, just for different targets. Is the existing syntax extensible to something like rustc-bin-link-arg=BIN_NAME=FLAG?
    • It also wasn't really clear to me, was there a specific need for rustc-link-arg? Like, there is a use case where you're building both a binary and cdylib in one package?
@npmccallum

This comment has been minimized.

Copy link
Author

npmccallum commented Feb 3, 2020

Some minor notes:

The documentation should move to unstable.md.

No problem.

I would recommend displaying a warning or error if -Zextra-link-arg is not specified and a build script declares one of these options.

I actually tried to do this when I added -Zextra-link-arg and failed. There is no access to the Context from the parsing code. Can you recommend a place in the code that would be natural to issue such a warning?

I think it would be good to display errors or warnings if these options are emitted for packages that do not have a corresponding cdylib/bin. I think this could help avoid confusion.

It seems to me that such a warning would need to have a view of both the parsed options and all artifacts in the current crate. Could you recommend such a place?

This still needs some tests. The CONTRIBUTING docs has information on running tests. The support module has docs on writing tests. The build script tests have lots of examples. I would probably put these in a new file, since build_script.rs is crazy long.

I think it is reasonable to ask for tests. However, I'm not sure how to actually implement a test that can be applied everywhere. A build script, for example, will only work on some linkers. Adding -lLIBRARY will depend on that library in the target and is therefore target-specific. I'm simply not aware of a linker flag that:

  1. Isn't target-specific.
  2. Isn't linker-specific.
  3. Is measurable after a build.

Does anyone have a suggestion?

I don't have a strong preference for choosing which targets this applies to, but I do think it could get unnecessarily complex if more things are piled on afterwards. I think it would be bad if there was a multitude of script commands that all do essentially the same thing, just for different targets. Is the existing syntax extensible to something like rustc-bin-link-arg=BIN_NAME=FLAG?

I honestly don't know. Many linker arguments contain =. So I don't see any unambiguous way to parse this.

It also wasn't really clear to me, was there a specific need for rustc-link-arg? Like, there is a use case where you're building both a binary and cdylib in one package?

A binary that loads plugins (cdylib) would do this. And it may share a linker script to strip out unneeded ELF sections.

@ehuss

This comment has been minimized.

Copy link
Contributor

ehuss commented Feb 5, 2020

I actually tried to do this when I added -Zextra-link-arg and failed. There is no access to the Context from the parsing code. Can you recommend a place in the code that would be natural to issue such a warning?

You'll need to pass a bool into the parsing code that indicates whether or not the unstable flag is enabled. Code like custom_build::build_work will need to load that bool into a local variable outside of the closure so that the closure can capture it.

Code like config::target::parse_links_overrides already has access to the Config object, and can just pass the bool from there.

It seems to me that such a warning would need to have a view of both the parsed options and all artifacts in the current crate. Could you recommend such a place?

I think similar to above, inside custom_build::build_work, you can check which targets the package has (inspect unit.pkg.targets()), and store whatever information is needed in a local variable that will be captured in the closure where it can compare with the parsed output. For example, if there is a cdylib option, check if there is a cdylib target.

However, I'm not sure how to actually implement a test that can be applied everywhere. A build script, for example, will only work on some linkers.

I think it would be reasonable to create a very basic test that just makes sure the flags are passed to rustc, but they don't actually have to work. So, for example, have a build script that emits cargo:rustc-link-arg=--this_is_a_bogus_flag, and then run cargo build -v and make sure it gets passed, something like:

// This is just verifying the flag is passed. Since it is a bogus flag, it should cause a failure.
p.cargo("build -v")
    .with_status(101)
    .with_stderr_contains("[RUNNING] `rustc --crate-name foo [..]-C link-arg=--this_is_a_bogus_flag[..]")
    .run();

At least, I think that should fail on all platforms.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 7, 2020

☔️ The latest upstream changes (presumably #7857) made this pull request unmergeable. Please resolve the merge conflicts.

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

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.