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.

Sign up for GitHub

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

Jump to bottom

Implement -Zlink-directives=yes/no #107675

Merged
merged 2 commits into from
Feb 26, 2023
Merged

Implement -Zlink-directives=yes/no #107675

merged 2 commits into from
Feb 26, 2023

Conversation

jsgf
Copy link
Contributor

@jsgf jsgf commented Feb 4, 2023 •
edited
Loading

-Zlink-directives=no will ignored #[link] directives while compiling a crate, so nothing is emitted into the crate's metadata. The assumption is that the build system already knows about the crate's native dependencies and can provide them at link time without these directives.

This is another way to address issue # #70093, which is currently addressed by -Zlink-native-libraries (implemented in #70095). The latter is implemented at link time, which has the effect of ignoring #[link] in every crate. This makes it a very large hammer as it requires all native dependencies to be known to the build system to be at all usable, including those in sysroot libraries. I think this means its effectively unused, and definitely under-used.

Being able to control this on a crate-by-crate basis should make it much easier to apply when needed.

I'm not sure if we need both mechanisms, but we can decide that later.

cc @pcwalton @cramertj

@rustbot
Copy link
Collaborator

rustbot commented Feb 4, 2023

r? @lcnr

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 4, 2023
@jsgf jsgf force-pushed the link-directives branch 5 times, most recently from ad959f6 to 39c4b4f Compare February 5, 2023 00:43
@jsgf
Copy link
Contributor Author

jsgf commented Feb 6, 2023

Also cc @joshtriplett - I think I prefer this approach compared to raw_dylib since it can be controlled by the build system and doesn't need source changes.

@lcnr
Copy link
Contributor

lcnr commented Feb 7, 2023

r? compiler

@rustbot rustbot assigned wesleywiser and unassigned lcnr Feb 7, 2023
@joshtriplett
Copy link
Member

I'm not sure if this is the approach we want long-term, but zero objection to it as a -Z flag for experimentation.

@petrochenkov petrochenkov self-assigned this Feb 8, 2023
@petrochenkov petrochenkov removed their assignment Feb 8, 2023
@jsgf jsgf force-pushed the link-directives branch 2 times, most recently from 56595ac to dd678f6 Compare February 8, 2023 22:54
@bors
Copy link
Contributor

bors commented Feb 10, 2023

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

@jsgf
Copy link
Contributor Author

jsgf commented Feb 12, 2023

I've deployed this internally and it's already showing value - when importing third-party packages, we no longer need to edit the source to remove #[link] directives.

@pcwalton
Copy link
Contributor

Seems fine to me. Perhaps we should proactively remove the other Zlink-native-libraries flag as a "failed experiment"?

@jsgf
Copy link
Contributor Author

jsgf commented Feb 14, 2023

Perhaps we should proactively remove the other Zlink-native-libraries flag as a "failed experiment"?

I'll put up a separate PR in case someone is still using it.

@jsgf
Copy link
Contributor Author

jsgf commented Feb 14, 2023

Ping @wesleywiser. Or @joshtriplett is this something you could accept?

@joshtriplett
Copy link
Member

👍 from me but I don't think I'm authoritative on compiler CLI arguments; someone from @rust-lang/compiler should r+ this.

@bors
Copy link
Contributor

bors commented Feb 21, 2023

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

davidtwco
davidtwco approved these changes Feb 21, 2023
View reviewed changes
Copy link
Member

@davidtwco davidtwco left a comment

Choose a reason for hiding this comment

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

r=me after rebase, this looks good to me and it's an unstable flag so we don't need to pin down the final name or take a vote or anything.

jsgf and others added 2 commits February 22, 2023 10:18
@jsgf
Implement -Zlink-directives=yes/no
fc5db2c 
`-Zlink-directives=no` will ignored `#[link]` directives while compiling a
crate, so nothing is emitted into the crate's metadata.  The assumption is
that the build system already knows about the crate's native dependencies
and can provide them at link time without these directives.

This is another way to address issue # rust-lang#70093, which is currently addressed
by `-Zlink-native-libraries` (implemented in rust-lang#70095). The latter is
implemented at link time, which has the effect of ignoring `#[link]`
in *every* crate. This makes it a very large hammer as it requires all
native dependencies to be known to the build system to be at all usable,
including those in sysroot libraries. I think this means its effectively
unused, and definitely under-used.

Being able to control this on a crate-by-crate basis should make it much
easier to apply when needed.

I'm not sure if we need both mechanisms, but we can decide that later.
@jsgf
link-directives: clarify usage message
fde2e40
@jsgf
Copy link
Contributor Author

jsgf commented Feb 22, 2023

@davidtwco OK, I rebased and fixed the conflicts.

@davidtwco
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Feb 23, 2023

📌 Commit fde2e40 has been approved by davidtwco

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 23, 2023
@compiler-errors compiler-errors mentioned this pull request Feb 25, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 25, 2023
@bors
Auto merge of rust-lang#108464 - compiler-errors:rollup-trl1g70, r=co…
f37f9f6 
…mpiler-errors

Rollup of 7 pull requests

Successful merges:

 - rust-lang#105736 (Test that the compiler/library builds with validate-mir)
 - rust-lang#107291 ([breaking change] Remove a rustdoc back compat warning)
 - rust-lang#107675 (Implement -Zlink-directives=yes/no)
 - rust-lang#107848 (Split `x setup` sub-actions to CLI arguments)
 - rust-lang#107911 (Add check for invalid #[macro_export] arguments)
 - rust-lang#108229 ([107049] Recognise top level keys in config.toml.example)
 - rust-lang#108333 (Make object bound candidates sound in the new trait solver)

Failed merges:

 - rust-lang#108337 (hir-analysis: make a helpful note)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 1a599d7 into rust-lang:master Feb 26, 2023
@rustbot rustbot added this to the 1.69.0 milestone Feb 26, 2023
@jsgf
Copy link
Contributor Author

jsgf commented Mar 29, 2023

@pcwalton

Seems fine to me. Perhaps we should proactively remove the other Zlink-native-libraries flag as a "failed experiment"?

I think we still need -Zlink-native-libraries to deal with prebuilt rlibs - ie, the sysroot libraries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petrochenkov petrochenkov petrochenkov left review comments

@davidtwco davidtwco davidtwco approved these changes

Assignees

@wesleywiser wesleywiser

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.69.0
Development

Successfully merging this pull request may close these issues.
9 participants
@jsgf @rustbot @lcnr @joshtriplett @bors @pcwalton @davidtwco @petrochenkov @wesleywiser