Skip to content

Fix handling of +whole-archive native link modifier. #88161

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
Merged
merged 3 commits into from
Sep 7, 2021
Merged

Fix handling of +whole-archive native link modifier. #88161

merged 3 commits into from
Sep 7, 2021

Conversation

@michaelwoerister
Copy link
Member

@michaelwoerister michaelwoerister commented Aug 19, 2021 •
edited
Loading

This PR fixes a bug in add_upstream_native_libraries that led to the +whole-archive modifier being ignored when linking in native libs.

Note that the PR does not address the situation when +whole-archive is combined with +bundle.
@wesleywiser's commit adds validation code that turns combining +whole-archive with +bundle into an error.

Fixes #88085.

r? @petrochenkov
cc @wesleywiser @gcoakes

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 19, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@michaelwoerister michaelwoerister force-pushed the fix-whole-archive-no-bundle branch from 1da2ebd to 4f42015 Compare August 19, 2021 15:39
@michaelwoerister michaelwoerister force-pushed the fix-whole-archive-no-bundle branch from 4f42015 to 7b57e22 Compare August 20, 2021 12:37
petrochenkov
petrochenkov reviewed Aug 20, 2021
View reviewed changes
@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 20, 2021
@michaelwoerister michaelwoerister force-pushed the fix-whole-archive-no-bundle branch from 7b57e22 to fc53fff Compare August 20, 2021 14:14
@petrochenkov petrochenkov added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 20, 2021
petrochenkov
petrochenkov reviewed Aug 21, 2021
View reviewed changes
@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Aug 21, 2021
@wesleywiser wesleywiser force-pushed the fix-whole-archive-no-bundle branch from 34f7f7c to e024481 Compare August 23, 2021 00:41
@michaelwoerister
Copy link
Member Author

michaelwoerister commented Aug 23, 2021

I think this is ready for review now.

@michaelwoerister michaelwoerister added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 23, 2021
@inquisitivecrystal inquisitivecrystal added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 24, 2021
petrochenkov
petrochenkov reviewed Aug 26, 2021
View reviewed changes
@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 26, 2021
@wesleywiser wesleywiser force-pushed the fix-whole-archive-no-bundle branch from e024481 to 2899c45 Compare August 26, 2021 23:30
@wesleywiser wesleywiser added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 26, 2021
@petrochenkov
Copy link
Contributor

petrochenkov commented Aug 27, 2021

Thanks!
@bors r+

@bors
Copy link
Collaborator

bors commented Aug 27, 2021

📌 Commit 2899c45 has been approved by petrochenkov

@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 Aug 27, 2021
@michaelwoerister michaelwoerister force-pushed the fix-whole-archive-no-bundle branch from 2899c45 to 07241e6 Compare August 30, 2021 09:57
@michaelwoerister
Copy link
Member Author

michaelwoerister commented Aug 30, 2021

@petrochenkov, the merge conflict is resolved now.

@michaelwoerister michaelwoerister added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 30, 2021
@petrochenkov
Copy link
Contributor

petrochenkov commented Aug 30, 2021

@bors r+

@bors
Copy link
Collaborator

bors commented Aug 30, 2021

📌 Commit 07241e6 has been approved by petrochenkov

@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 Aug 30, 2021
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Sep 1, 2021
@GuillaumeGomez
Rollup merge of rust-lang#88161 - michaelwoerister:fix-whole-archive-…
6ed7428 
…no-bundle, r=petrochenkov

Fix handling of +whole-archive native link modifier.

This PR fixes a bug in `add_upstream_native_libraries` that led to the `+whole-archive` modifier being ignored when linking in native libs.

~~Note that the PR does not address the situation when `+whole-archive` is combined with `+bundle`.~~
`@wesleywiser's` commit adds validation code that turns combining `+whole-archive` with `+bundle` into an error.

Fixes rust-lang#88085.

r? `@petrochenkov`
cc `@wesleywiser` `@gcoakes`
@GuillaumeGomez GuillaumeGomez mentioned this pull request Sep 1, 2021
Closed
@GuillaumeGomez
Copy link
Member

GuillaumeGomez commented Sep 2, 2021

It failed CI in #88570.

@bors: r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 2, 2021
@michaelwoerister michaelwoerister force-pushed the fix-whole-archive-no-bundle branch from 07241e6 to 70c7317 Compare September 7, 2021 12:14
@michaelwoerister
Copy link
Member Author

michaelwoerister commented Sep 7, 2021

I added ignore-cross-compile to the test case. Let's see if that is enough to disable the platforms that have trouble compiling C++ code.

@michaelwoerister
Copy link
Member Author

michaelwoerister commented Sep 7, 2021

@bors r=petrochenkov rollup=never

@bors
Copy link
Collaborator

bors commented Sep 7, 2021

📌 Commit 70c7317 has been approved by petrochenkov

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 7, 2021
@bors
Copy link
Collaborator

bors commented Sep 7, 2021

⌛ Testing commit 70c7317 with merge 73641cd...

@bors
Copy link
Collaborator

bors commented Sep 7, 2021

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing 73641cd to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 7, 2021
@bors bors merged commit 73641cd into rust-lang:master Sep 7, 2021
@rustbot rustbot added this to the 1.57.0 milestone Sep 7, 2021
@rust-timer
Copy link
Collaborator

rust-timer commented Sep 7, 2021

Finished benchmarking commit (73641cd): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

@petrochenkov petrochenkov mentioned this pull request Jul 18, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wesleywiser wesleywiser wesleywiser left review comments

@petrochenkov petrochenkov petrochenkov left review comments

Assignees

@petrochenkov petrochenkov

Labels

merged-by-bors This PR was explicitly merged by bors. 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.57.0

Development

Successfully merging this pull request may close these issues.

Whole archive modifier for native libraries is not propagated to final output.

10 participants

@michaelwoerister @rust-log-analyzer @petrochenkov @bors @GuillaumeGomez @rust-timer @wesleywiser @rust-highfive @inquisitivecrystal @rustbot