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

Support native library modifiers (RFC 2951) #671

Merged
merged 1 commit into from
Oct 25, 2022

Conversation

petrochenkov
Copy link
Contributor

@petrochenkov petrochenkov commented Apr 2, 2022

cc rust-lang/rust#81490

I haven't added a test because stabilization of the feature has just landed on nightly and is not available on stable yet, but I've tested it manually.

Closes #689.

@dot-asm
Copy link
Contributor

dot-asm commented Apr 7, 2022

I for one would argue that link_modifies would be more appropriate name for suggested method. Because cc.modifiers("x") would appear rather as compiler modifier.

@petrochenkov
Copy link
Contributor Author

petrochenkov commented Apr 8, 2022

I'd say the full name is "native library modifiers" because they are not necessarily about linking in the narrow sense, since they can also control bundling of native libraries into rlibs/staticlibs or potentially deduplication before linking.
Not sure how to better name it in this specific interface.
EDIT: But link_modifies should be fine too.

@dot-asm
Copy link
Contributor

dot-asm commented Apr 8, 2022

In the context "native" is effectively synonymous with "ffi" and cc-rs is all about "ffi." In other words "native" feels redundant in the context.

Just a reminder, I'm just a cc-rs user weathering #663.

@petrochenkov
Copy link
Contributor Author

Renamed to link_lib_modifiers to match the modified cargo directive.

@petrochenkov
Copy link
Contributor Author

Ping @m-ou-se.
Can this be merged in some form and released before the Rust 1.61 release?
It helps to address the first compatibility note from https://github.com/rust-lang/rust/pull/96539/files.

@m-ou-se
Copy link
Member

m-ou-se commented May 17, 2022

Hi!

Is this cargo feature documented anywhere? The docs only shows the =[KIND=]NAME syntax, but say nothing about any modifiers.

@petrochenkov
Copy link
Contributor Author

petrochenkov commented May 17, 2022

This is a rustc syntax (https://doc.rust-lang.org/nightly/rustc/command-line-arguments.html#-l-link-the-generated-crate-to-a-native-library), cargo just passes the STRING in cargo:rustc-link-lib=STRING directly to rustc.

But I agree that the cargo docs also need to be updated, I'll send a PR.
UPD: PR sent - rust-lang/cargo#10674.

@m-ou-se
Copy link
Member

m-ou-se commented May 17, 2022

This is a rustc syntax (https://doc.rust-lang.org/nightly/rustc/command-line-arguments.html#-l-link-the-generated-crate-to-a-native-library), cargo just passes the STRING in cargo:rustc-link-lib=STRING directly to rustc.

Ah great. Can you add a link to that in the doc comment for link_lib_modifiers?

Then one more question:

The flag() method takes a single flag and adds it to a list of flags, similar for include(), but this new link_lib_modifiers() method takes a string with multiple modifiers and overrides any previously set value. Do you think it should be link_lib_modifier() that adds a single modifier, instead?

@petrochenkov
Copy link
Contributor Author

Can you add a link to that in the doc comment for link_lib_modifiers?

Done.

The flag() method takes a single flag and adds it to a list of flags, similar for include(), but this new link_lib_modifiers() method takes a string with multiple modifiers and overrides any previously set value. Do you think it should be link_lib_modifier() that adds a single modifier, instead?

Both resetting and appending method could potentially make sense in both cases.
I added only a resetting method for the modifiers just because it's simpler, more general, and sufficient for now.

(I've changed the parameter from &str to Option<&str> though, so you can reset the modifiers back to none if necessary.)

@helgoboss
Copy link

Thanks @petrochenkov. I'm also very interested in your addition because the Rust 1.61 change rust-lang/rust#93901 breaks my application at runtime (although it compiles and links successfully). Using .link_lib_modifiers(Some("+whole-archive")) for one specific cc build invocation fixes that. For the record, here's the commit which fixes it: helgoboss/helgobox@0055d6c.

@petrochenkov
Copy link
Contributor Author

@helgoboss
FWIW, +whole-archive can be passed even without this API, by using build.cargo_metadata(false) first, and then emitting the cargo:rust-ling-* directives manually:

println!("cargo:rustc-link-lib=static:+whole-archive={}", lib_name);
println!("cargo:rustc-link-search=native={}", env::var("OUT_DIR"));

Ping @m-ou-se again.

@helgoboss
Copy link

@petrochenkov Thanks, good to know. Your solution is much more convenient though.

@m-ou-se
Copy link
Member

m-ou-se commented Jul 6, 2022

I added only a resetting method for the modifiers just because it's simpler, more general, and sufficient for now.

(I've changed the parameter from &str to Option<&str> though, so you can reset the modifiers back to none if necessary.)

The other methods like flags(), include(), file(), define(), are all append-only and provide no way of resetting. I think we should keep this consistent with that.

Right now it doesn't matter much with only "+whole-archive", but it'd be nice if we can keep things unchanged when more useful modifiers are added in the furture.

@petrochenkov
Copy link
Contributor Author

Updated in accordance with #671 (comment).

@petrochenkov
Copy link
Contributor Author

ping @m-ou-se

Wilfred added a commit to Wilfred/difftastic that referenced this pull request Aug 28, 2022
This fixes the build for Rust 1.61+ on some machines. I can reliably
reproduce this locally, but CI does not exhibit this issue (I'm not
sure why).

The Rust compatibility notes document this change:
https://github.com/rust-lang/rust/blob/1.61.0/RELEASES.md#compatibility-notes

and eventually this will be supported by cc:
rust-lang/cc-rs#671

Fixes #339
felixwilhelm added a commit to weggli-rs/weggli that referenced this pull request Sep 23, 2022
rustc >=1.61.0 breaks the build of our Python API (see https://github.com/rust-lang/rust/blob/1.61.0/RELEASES.md#compatibility-notes).

Lets wait for rust-lang/cc-rs#671 to land and hardcode the CI environment to 1.60.0 for now.
Copy link
Member

@thomcc thomcc left a comment

Choose a reason for hiding this comment

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

This looks good to me, thanks.

Can this be merged in some form and released before the Rust 1.61 release?

Obviously we missed the boat here, but I'll try to cut a release after I finish going through the pending PRs and merging the ones that are viable.

@thomcc thomcc merged commit 39fc215 into rust-lang:main Oct 25, 2022
Wilfred added a commit to Wilfred/difftastic that referenced this pull request Dec 31, 2022
rust-lang/cc-rs#671 has now been merged and
released, so a247218 is now unncessary.
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.

Support linking modifiers
5 participants