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

Self contained linking option #72738

Merged
merged 3 commits into from
Jun 26, 2020
Merged

Self contained linking option #72738

merged 3 commits into from
Jun 26, 2020

Conversation

mati865
Copy link
Contributor

@mati865 mati865 commented May 29, 2020 •
edited
Loading

With objects moved to self-contained directory by #72999 we can now add option to control whether to use self-contained on native linkage mode.

@mati865
Copy link
Contributor Author

mati865 commented Jun 2, 2020

@petrochenkov could you do @bors try?

@petrochenkov petrochenkov self-assigned this Jun 4, 2020
@petrochenkov
Copy link
Contributor

@bors try

@bors
Copy link
Contributor

bors commented Jun 4, 2020

⌛ Trying commit b173dca3afcee12951c6df33733197171f5cf0f8 with merge 97f416b9b95c99013190cc71a59bd73d0596ba36...

@petrochenkov petrochenkov added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jun 4, 2020
@bors
Copy link
Contributor

bors commented Jun 5, 2020

☀️ Try build successful - checks-azure
Build commit: 97f416b9b95c99013190cc71a59bd73d0596ba36 (97f416b9b95c99013190cc71a59bd73d0596ba36)

@mati865
Copy link
Contributor Author

mati865 commented Jun 5, 2020

Proof of concept detection of external MinGW appears to work nice.

FYI: rust-mingw contains mingw-w64 libs and the linker (GCC+LD), CRT is shipped with rust-std.

Observations when linking Rust library to C library built with newer mingw-w64:
rust-mingw installed rust-mingw not installed
link_self_contained=no

ld: cannot find crtbegin.o: No such file or directory

Works
link_self_contained=yes

Error: Rust shipped mingw-w64 libs are too old for the CRT C lib is linked to

Error: external mingw-w64 libs are too new for the CRT Rust tries to link

link_self_contained=yes is not expected to work since Rust shipped mingw-64 might or might not match version that the other lib was built with.
link_self_contained=no does fix #68887 when rust-mingw is not installed. When rust-mingw is installed we use linker from it, even though we detected external mingw-w64 properly.

So to really fix #68887 we have to move the linker to self-contained directory similar way as ff2703c moves the libs and CRT.

@petrochenkov petrochenkov 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 Jun 5, 2020
@petrochenkov
Copy link
Contributor

@mati865
Is it better to land #72999 first and then proceed to this PR, or this PR can be landed independently?
(I haven't read either of them yet.)

@mati865
Copy link
Contributor Author

mati865 commented Jun 5, 2020

At first I though this could be landed separately but now I'm not that sure. This PR is already a slight improvement but full potential requires #72999 + another change to also move bin to self-contained directory.
Ideally I would like all that stuff in different nightly builds so ppl can find possible regressions easier.

@petrochenkov
Copy link
Contributor

Ok, let's start with #72999 then.

@petrochenkov petrochenkov added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 6, 2020
@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 Jun 19, 2020
@mati865 mati865 mentioned this pull request Jun 19, 2020
Closed
@mati865 mati865 force-pushed the self-contained-option branch 4 times, most recently from ec4e7b0 to a988905 Compare June 19, 2020 14:05
@mati865
Copy link
Contributor Author

mati865 commented Jun 19, 2020

Can I have one more try build to test musl and MinGW?

@petrochenkov
Copy link
Contributor

@bors try

@bors
Copy link
Contributor

bors commented Jun 19, 2020

⌛ Trying commit a98890521222b8a209d08db7e3ee68bcf876b503 with merge f90b336e3b94505daa475040dc34722df2ae2b16...

This was referenced Jun 19, 2020
Closed
Merged
@bors
Copy link
Contributor

bors commented Jun 19, 2020

☀️ Try build successful - checks-azure
Build commit: f90b336e3b94505daa475040dc34722df2ae2b16 (f90b336e3b94505daa475040dc34722df2ae2b16)

@petrochenkov petrochenkov mentioned this pull request Jun 21, 2020
Open
@mati865 mati865 force-pushed the self-contained-option branch from 9863a28 to 4c5e9f0 Compare June 22, 2020 14:04
@mati865
Copy link
Contributor Author

mati865 commented Jun 22, 2020

That could use one last try.
I've modified the PR to have no effect unless unstable link-self-contained is passed so it should not require another MCP.
Once it lands we can ask users for the feedback, move detection logic to crt_objects_fallback and remove get_crt_libs_path.

@mati865 mati865 changed the title [WIP] Self contained linking option Self contained linking option Jun 22, 2020
@mati865 mati865 marked this pull request as ready for review June 22, 2020 14:14
@petrochenkov
Copy link
Contributor

@bors try

@bors
Copy link
Contributor

bors commented Jun 22, 2020

🔒 Merge conflict

This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again.

How do I rebase?

Assuming self is your fork and upstream is this repository, you can resolve the conflict following these steps:

  1. git checkout self-contained-option (switch to your branch)
  2. git fetch upstream master (retrieve the latest master)
  3. git rebase upstream/master -p (rebase on top of it)
  4. Follow the on-screen instruction to resolve conflicts (check git status if you got lost).
  5. git push self self-contained-option --force-with-lease (update this PR)

You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial.

Please avoid the "Resolve conflicts" button on GitHub. It uses git merge instead of git rebase which makes the PR commit history more difficult to read.

Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Cargo.lock conflict is handled during merge and rebase. This is normal, and you should still perform step 5 to update this PR.

Error message 
Auto-merging src/librustc_session/options.rs
Auto-merging src/librustc_codegen_ssa/back/link.rs
Auto-merging src/ci/azure-pipelines/try.yml
CONFLICT (content): Merge conflict in src/ci/azure-pipelines/try.yml
Automatic merge failed; fix conflicts and then commit the result.

@petrochenkov
Copy link
Contributor

Needs rebase.

petrochenkov
petrochenkov reviewed Jun 22, 2020
View reviewed changes
src/librustc_codegen_ssa/back/link.rs Outdated Show resolved Hide resolved
src/librustc_codegen_ssa/back/link.rs Outdated Show resolved Hide resolved
src/librustc_session/options.rs Outdated Show resolved Hide resolved
@mati865 mati865 force-pushed the self-contained-option branch from c1d1617 to fed5593 Compare June 22, 2020 18:03
@petrochenkov
Copy link
Contributor

@bors try

@bors
Copy link
Contributor

bors commented Jun 22, 2020

⌛ Trying commit fed5593fa808a369ea5126cfeb09874c2b687e3f with merge 122b602cd8399434540a6b5932a1938330f66c36...

@bors
Copy link
Contributor

bors commented Jun 22, 2020

☀️ Try build successful - checks-azure
Build commit: 122b602cd8399434540a6b5932a1938330f66c36 (122b602cd8399434540a6b5932a1938330f66c36)

@mati865 mati865 force-pushed the self-contained-option branch from fed5593 to f2ee031 Compare June 23, 2020 18:14
@petrochenkov
Copy link
Contributor

Looks like this is ready?
r=me after squashing the commits.

@mati865 mati865 force-pushed the self-contained-option branch from f2ee031 to f27dcd7 Compare June 25, 2020 09:35
@mati865
Copy link
Contributor Author

mati865 commented Jun 25, 2020

Looks like this is ready?

Yeah, forgot to write a comment 🤷‍♂️

@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jun 25, 2020

📌 Commit f27dcd7 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 Jun 25, 2020
Manishearth added a commit to Manishearth/rust that referenced this pull request Jun 26, 2020
@Manishearth
Rollup merge of rust-lang#72738 - mati865:self-contained-option, r=pe…
e98f991 
…trochenkov

Self contained linking option

With objects moved to self-contained directory by rust-lang#72999 we can now add option to control whether to use self-contained on native linkage mode.
This was referenced Jun 26, 2020
Closed
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 26, 2020
@bors
Auto merge of rust-lang#73746 - Manishearth:rollup-80jnynm, r=Manishe…
e093b65 
…arth

Rollup of 14 pull requests

Successful merges:

 - rust-lang#72617 (Add a fast path for `std::thread::panicking`.)
 - rust-lang#72738 (Self contained linking option)
 - rust-lang#72770 (Implement mixed script confusable lint.)
 - rust-lang#73418 (Add unstable `core::mem::variant_count` intrinsic)
 - rust-lang#73460 (Emit line info for generator variants)
 - rust-lang#73534 (Provide suggestions for some moved value errors)
 - rust-lang#73538 (make commented examples use valid syntax, and be more consistent )
 - rust-lang#73581 (Create 0766 error code)
 - rust-lang#73619 (Document the mod keyword)
 - rust-lang#73621 (Document the mut keyword)
 - rust-lang#73648 (Document the return keyword)
 - rust-lang#73673 (Fix ptr doc warnings.)
 - rust-lang#73674 (Tweak binop errors)
 - rust-lang#73687 (Clean up E0701 explanation)

Failed merges:

 - rust-lang#73708 (Explain move errors that occur due to method calls involving `self` (take two))

r? @ghost
@bors bors merged commit 9275ff7 into rust-lang:master Jun 26, 2020
@mati865 mati865 deleted the self-contained-option branch June 26, 2020 07:31
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

Assignees

@petrochenkov petrochenkov

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mati865 @petrochenkov @bors