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

Create self-contained directory and move there some of external binaries/libs #72999

Merged
merged 6 commits into from
Jun 19, 2020

Conversation

mati865
Copy link
Contributor

@mati865 mati865 commented Jun 4, 2020

One of the steps to reach design described in #68887 (comment)
This PR moves things around and allows link code to handle the new directory structure.

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 4, 2020
@mati865
Copy link
Contributor Author

mati865 commented Jun 4, 2020

cc @petrochenkov

Copy link
Member

@Mark-Simulacrum Mark-Simulacrum left a comment

Choose a reason for hiding this comment

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

I know essentially nothing about these so happy to cede review to @petrochenkov (or someone else). The bootstrap code looks reasonable, though only from a "doesn't do anything unexpected" rather than "does the right thing" perspective.

src/bootstrap/dist.rs Show resolved Hide resolved
src/bootstrap/compile.rs Outdated Show resolved Hide resolved
src/bootstrap/lib.rs Outdated Show resolved Hide resolved
src/bootstrap/compile.rs Outdated Show resolved Hide resolved
src/bootstrap/compile.rs Show resolved Hide resolved
@petrochenkov
Copy link
Contributor

LGTM.
I can't comment on the cross-compilation case because I'm not too familiar with bootstrap.

@petrochenkov
Copy link
Contributor

I'd like to interject for a moment to paint the bike shed.
I was thinking about choosing a name for the "fallback" / "self-contained" mode and using it consistently.

On one hand this is a "fallback" mode because it's not desirable (also "fallback" is a shorter word).

On the other hand, we can't really fallback from the libraries and objects provided by native toolchain to libraries and objects provided by us, in a sense that "<sysroot>/fallback is always in search paths, but "shadowed" by the native toolchain it it is present".
If gcc is used as a linker, it will hardcode CRT objects without using any search paths, and for libraries there's no way to add our search directory to the end of the list implicitly supplied by gcc.
So we'll have to explicitly use or not use the <sysroot>/fallback directory depending on the mode and if used it will take priority over the native toolchain if it happens to exist after all.

So, I guess, "self-contained" it is.

@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 Jun 6, 2020
@mati865
Copy link
Contributor Author

mati865 commented Jun 8, 2020

I don't mind either name.
Once you are happy with the changes and @bors try proves nothing has broke I'll squash the commits.

@mati865 mati865 force-pushed the separate-self-contained-dir branch from ef5f5f8 to da03b26 Compare June 8, 2020 11:25
@mati865
Copy link
Contributor Author

mati865 commented Jun 8, 2020

Included commit moving bundled GCC and Binutils to self-contained dir since it was smaller than I expected. I'll retitle the PR if you are fine with that.

@petrochenkov
Copy link
Contributor

@bors try

@bors
Copy link
Contributor

bors commented Jun 8, 2020

⌛ Trying commit da03b26cfe80fc12c10d9ed41dd8afe4bdb3484a with merge 8391cc93e571cc4a32d6b0a9958e88a154268c7a...

@bors
Copy link
Contributor

bors commented Jun 8, 2020

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

@petrochenkov petrochenkov removed their assignment Jun 8, 2020
@petrochenkov
Copy link
Contributor

This distribution layout change may require tweaking manifests used by rustup, for the rust-mingw component in particular.
I'm not sure how to do it and whether it's actually necessary, Mark-Simulacrum may know more, but it all should be in this repository.

@mati865
Copy link
Contributor Author

mati865 commented Jun 9, 2020

This distribution layout change may require tweaking manifests used by rustup, for the rust-mingw component in particular.

I'm not sure what you mean. This works fine when installed with rustup-toolchain-install-master 8391cc93e571cc4a32d6b0a9958e88a154268c7a -c rust-mingw.

Contents of the manifest:
file:lib/rustlib/x86_64-pc-windows-gnu/bin/self-contained/dlltool.exe
file:lib/rustlib/x86_64-pc-windows-gnu/bin/self-contained/GCC-WARNING.txt
file:lib/rustlib/x86_64-pc-windows-gnu/bin/self-contained/ld.exe
file:lib/rustlib/x86_64-pc-windows-gnu/bin/self-contained/libwinpthread-1.dll
file:lib/rustlib/x86_64-pc-windows-gnu/bin/self-contained/x86_64-w64-mingw32-gcc.exe
file:lib/rustlib/x86_64-pc-windows-gnu/lib/self-contained/libadvapi32.a
file:lib/rustlib/x86_64-pc-windows-gnu/lib/self-contained/libbcrypt.a
file:lib/rustlib/x86_64-pc-windows-gnu/lib/self-contained/libcomctl32.a
file:lib/rustlib/x86_64-pc-windows-gnu/lib/self-contained/libcomdlg32.a
file:lib/rustlib/x86_64-pc-windows-gnu/lib/self-contained/libcredui.a
file:lib/rustlib/x86_64-pc-windows-gnu/lib/self-contained/libcrypt32.a
file:lib/rustlib/x86_64-pc-windows-gnu/lib/self-contained/libdbghelp.a
file:lib/rustlib/x86_64-pc-windows-gnu/lib/self-contained/libgcc.a
file:lib/rustlib/x86_64-pc-windows-gnu/lib/self-contained/libgcc_eh.a
file:lib/rustlib/x86_64-pc-windows-gnu/lib/self-contained/libgcc_s.a
file:lib/rustlib/x86_64-pc-windows-gnu/lib/self-contained/libgdi32.a
file:lib/rustlib/x86_64-pc-windows-gnu/lib/self-contained/libiconv.a
file:lib/rustlib/x86_64-pc-windows-gnu/lib/self-contained/libimagehlp.a
file:lib/rustlib/x86_64-pc-windows-gnu/lib/self-contained/libiphlpapi.a
file:lib/rustlib/x86_64-pc-windows-gnu/lib/self-contained/libkernel32.a
file:lib/rustlib/x86_64-pc-windows-gnu/lib/self-contained/libm.a
file:lib/rustlib/x86_64-pc-windows-gnu/lib/self-contained/libmingw32.a
file:lib/rustlib/x86_64-pc-windows-gnu/lib/self-contained/libmingwex.a
file:lib/rustlib/x86_64-pc-windows-gnu/lib/self-contained/libmoldname.a
file:lib/rustlib/x86_64-pc-windows-gnu/lib/self-contained/libmsimg32.a
file:lib/rustlib/x86_64-pc-windows-gnu/lib/self-contained/libmsvcrt.a
file:lib/rustlib/x86_64-pc-windows-gnu/lib/self-contained/libodbc32.a
file:lib/rustlib/x86_64-pc-windows-gnu/lib/self-contained/libole32.a
file:lib/rustlib/x86_64-pc-windows-gnu/lib/self-contained/liboleaut32.a
file:lib/rustlib/x86_64-pc-windows-gnu/lib/self-contained/libopengl32.a
file:lib/rustlib/x86_64-pc-windows-gnu/lib/self-contained/libpsapi.a
file:lib/rustlib/x86_64-pc-windows-gnu/lib/self-contained/libpthread.a
file:lib/rustlib/x86_64-pc-windows-gnu/lib/self-contained/librpcrt4.a
file:lib/rustlib/x86_64-pc-windows-gnu/lib/self-contained/libsecur32.a
file:lib/rustlib/x86_64-pc-windows-gnu/lib/self-contained/libsetupapi.a
file:lib/rustlib/x86_64-pc-windows-gnu/lib/self-contained/libshell32.a
file:lib/rustlib/x86_64-pc-windows-gnu/lib/self-contained/libstdc++.a
file:lib/rustlib/x86_64-pc-windows-gnu/lib/self-contained/libsynchronization.a
file:lib/rustlib/x86_64-pc-windows-gnu/lib/self-contained/libuser32.a
file:lib/rustlib/x86_64-pc-windows-gnu/lib/self-contained/libuserenv.a
file:lib/rustlib/x86_64-pc-windows-gnu/lib/self-contained/libuuid.a
file:lib/rustlib/x86_64-pc-windows-gnu/lib/self-contained/libwinhttp.a
file:lib/rustlib/x86_64-pc-windows-gnu/lib/self-contained/libwinmm.a
file:lib/rustlib/x86_64-pc-windows-gnu/lib/self-contained/libwinspool.a
file:lib/rustlib/x86_64-pc-windows-gnu/lib/self-contained/libws2_32.a
file:lib/rustlib/x86_64-pc-windows-gnu/lib/self-contained/libwsock32.a

@petrochenkov
Copy link
Contributor

It's great if it works without changes, I just mentioned it as something to check and keep in mind.

@mati865 mati865 force-pushed the separate-self-contained-dir branch from da03b26 to 7c45b38 Compare June 9, 2020 21:46
@mati865 mati865 changed the title Bootstrap: improve handling of CRT objects Create self-contained directory and move there some of external binaries/libs Jun 9, 2020
@mati865
Copy link
Contributor Author

mati865 commented Jun 9, 2020

Okay, this should be ready. Should I squash Address the review commits into previous commits?

@Mark-Simulacrum
Copy link
Member

Please do squash the review-addressing commits.

@mati865 mati865 force-pushed the separate-self-contained-dir branch from 7c45b38 to 43905cd Compare June 11, 2020 16:49
@mati865
Copy link
Contributor Author

mati865 commented Jun 11, 2020

Squashed, rebased and testuite still passes.

@mati865

This comment has been minimized.

@rustbot rustbot 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 12, 2020
@Mark-Simulacrum
Copy link
Member

IMO, this is good to r+, but I want to stop before we move ahead and ask -- it feels like this is a pretty major change, and should go through at least a MCP (and maybe even compiler team FCP, given impacts to stable users). But, I don't know much about this area, so I'm going to cede r? @petrochenkov for final approval of the PR in a more conceptual sense (r=me on the literal code changes, though).

@petrochenkov
Copy link
Contributor

Sigh.

@mati865, could you fill the MCP form (https://forge.rust-lang.org/compiler/mcp.html#major-change-proposals-1) and point it to #68887 (comment)?

Then I'll r+ this once someone else sanity checks the idea.

@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 Jun 16, 2020
@Mark-Simulacrum
Copy link
Member

To be clear if this isn't changing anything all that major, just moving some files around and in a way that shouldn't matter to anyone, then I'm fine moving ahead without that -- I don't really understand our linkage story wrt to self-contained and mingw etc well enough that I can comment well there.

@mati865
Copy link
Contributor Author

mati865 commented Jun 16, 2020

@mati865, could you fill the MCP form (forge.rust-lang.org/compiler/mcp.html#major-change-proposals-1) and point it to #68887 (comment)?

I'll try to prepare it tomorrow.

To be clear if this isn't changing anything all that major, just moving some files around and in a way that shouldn't matter to anyone

The intention is to make no visible changes to the user yet. Few libraries are moved to a subdirectory but changes to src/librustc_codegen_ssa/back/link.rs make it always searched by the linker.
Is the internal structure of rustlib/ considered stable?

@Mark-Simulacrum
Copy link
Member

Aha okay, then I'm fine with just @bors r+ this PR, and we can MCP in parallel, before we land user visible changes.

Would be good to get some of the broader strategy written up for rustc dev guide too.

@bors
Copy link
Contributor

bors commented Jun 16, 2020

📌 Commit 43905cd has been approved by Mark-Simulacrum

@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 16, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 19, 2020
Rollup of 13 pull requests

Successful merges:

 - rust-lang#70740 (Enabling static-pie for musl)
 - rust-lang#72331 (Report error when casting an C-like enum implementing Drop)
 - rust-lang#72486 (Fix asinh of negative values)
 - rust-lang#72497 (tag/niche terminology cleanup)
 - rust-lang#72999 (Create self-contained directory and move there some of external binaries/libs)
 - rust-lang#73130 (Remove const prop for indirects)
 - rust-lang#73142 (Ensure std benchmarks get tested.)
 - rust-lang#73305 (Disallow loading crates with non-ascii identifier name.)
 - rust-lang#73346 (Add rust specific features to print target features)
 - rust-lang#73362 (Test that bounds checks are elided when slice len is checked up-front)
 - rust-lang#73459 (Reduce pointer casts in Box::into_boxed_slice)
 - rust-lang#73464 (Document format correction)
 - rust-lang#73479 (Minor tweaks to liballoc)

Failed merges:

r? @ghost
@bors bors merged commit ea3c309 into rust-lang:master Jun 19, 2020
@mati865 mati865 deleted the separate-self-contained-dir branch June 19, 2020 12:56
Manishearth added a commit to Manishearth/rust that referenced this pull request Jun 26, 2020
…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.
Manishearth added a commit to Manishearth/rust that referenced this pull request Jun 26, 2020
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants