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

rustbuild: don't create a source tarball when installing #42109

Merged
merged 4 commits into from
May 27, 2017

Conversation

Keruspe
Copy link
Contributor

@Keruspe Keruspe commented May 20, 2017

This splits Install out of Dist as it is not a full dist anymore, and creates the source tarball only for the Dist command.
This will allow splitting install in a few rules if we want as it's done for other phases.

@rust-highfive
Copy link
Collaborator

r? @alexcrichton

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

@@ -789,6 +786,10 @@ pub fn build_rules<'a>(build: &'a Build) -> Rules {
.dep(move |s| s.name("tool-build-manifest").target(&build.config.build).stage(0))
.run(move |_| dist::hash_and_sign(build));

rules.install("install", "path/to/nowhere")
.dep(|s| s.name("default:dist"))
Copy link
Member

Choose a reason for hiding this comment

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

Oh I was thinking that this is where we could apply the fix, not depending on default:dist? Ideally the install target doesn't depend on any source tarball, so that stage is just skipped entirely. Does that make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking of that too in the first place, but the install.sh generation is done as part of the dist, so it would be far less trivial.

Still doable though if you think that's a good idea.

Btw the next step could be splitting that into several rules such as install-tool-cargo or such. Just wanted to see if it was a good idea first.

Copy link
Member

Choose a reason for hiding this comment

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

Oh yeah so we'll definitely still need to dist things like libstd/librustc, but we just don't need to dist the source tarball, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right.

Since the rls README recommands adding rls, rust-analysis and rust-src to rustup, I figured rust-src was useful for it to work properly, so I didn't want to drop that part entirely. That's why I kept it as is and just put the actual full fist tarball creation (with the call to cargo vendor and so forth) in a conditional.

@Mark-Simulacrum Mark-Simulacrum added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 20, 2017
@Keruspe
Copy link
Contributor Author

Keruspe commented May 21, 2017

With these commits, install doesn't depend on "default:dist" anymore, and it doesn't require the combined installer anymore either.
Resulting image is identical.

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Looking great! I think there may be some trickery around the flags on the steps though. You may want to copy over stuff like host(true) and only_host_build(true) from the dist steps, as I think that'll be necessary to handle ./x.py install with a cross-compiling setup.

.dep(|s| s.name("dist-src"))
.run(move |s| install::Installer::new(build).install_src(s.stage));
rules.install("install-rustc", "src/librustc")
.default(true)
Copy link
Member

Choose a reason for hiding this comment

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

I think this may need some combination of .host(true) perhaps?

// Create the version file
write_file(&plain_dst_src.join("version"), build.rust_version().as_bytes());

if let Subcommand::Dist { .. } = build.flags.cmd {
Copy link
Member

Choose a reason for hiding this comment

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

Is this new clause still necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. This part generates both the rust-src component installer and the full source tarball with vendorized deps, etc, and we only want the former for install

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I might have found a way not to require that anymore.
I'm doing some testing, will finish that up tomorrow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's definitely still needed.
I tried not to depend on dist-* anymore and to just call the script method from rust-installer, but then it's missing some files like the manifest, so dealing with that would basically end up duplicating dist.

Copy link
Member

Choose a reason for hiding this comment

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

Hm so just to make sure I understand this, during a normal ./x.py install this step isn't invoked right because the rust-src component is optional?

Copy link
Member

Choose a reason for hiding this comment

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

Ah ok, are you thinking of supporting an extended build without cargo-vendor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, exactly.
./x.py dist would run cargo-vendor and create all the proper tarballs for disribution
./x.py install would just install the components without bothering with the vendoring and the full source tarball

Copy link
Member

Choose a reason for hiding this comment

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

Hm ok so if we want this use case to work then we may need to dig a little deeper. My intention was to not have a clause such as this because it means that a "source tarball" is now context-dependent if you're installing source or not. Ideally every single source tarball we produce is the same, regardless of the context.

IIRC, though, the rust-src component is a stripped down version of the actual source tarball, right? (i.e. it doesn't contain the vendor dir anyway?) If that's the case maybe we could separate these code paths so the rust-src component installation doesn't generate a vendor dir but the source tarball itself still unconditionally does?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The contents of "source tarball" is not context dependent, its existence is, fwiw. The if block contains the whole tarball generation, not just the vendoring.
I can try to split that in a separate function.

Copy link
Member

Choose a reason for hiding this comment

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

Oh sorry about that! I think I misread this.

In that case yeah want to just split this up into two functions and two steps? That way the install step could depend on the right one and dist would depend on both. Sorry for the confusion!

@Keruspe
Copy link
Contributor Author

Keruspe commented May 21, 2017

Would it be a good idea to instantiate only one Installer?
It would require using an Arc or such though as we use move for all the run() functions

@alexcrichton
Copy link
Member

Would it be a good idea to instantiate only one Installer?

Nah no worries, the impact here should be quite minor.

@Keruspe
Copy link
Contributor Author

Keruspe commented May 22, 2017

This part should be better now!

@alexcrichton
Copy link
Member

Awesome looks great to me! Want to squash and I'll r+?

only create source tarball for the Dist subcommand
mark install rule as default for Kind::Install
split install-docs
split install-std
factor out empty_dir handling
split install-cargo
split install-analysis
split install-src
rework install-rustc
properly handle cross-compilation setups for install
use pkgname in install
split plain source tarball generation from rust-src dist
document src-tarball in config.toml.exmaple

Signed-off-by: Marc-Antoine Perennou <[email protected]>
@Keruspe
Copy link
Contributor Author

Keruspe commented May 22, 2017

rebased and squashed

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented May 22, 2017

📌 Commit 150d644 has been approved by alexcrichton

frewsxcv added a commit to frewsxcv/rust that referenced this pull request May 23, 2017
rustbuild: don't create a source tarball when installing

This splits Install out of Dist as it is not a full dist anymore, and creates the source tarball only for the Dist command.
This will allow splitting install in a few rules if we want as it's done for other phases.
@Keruspe
Copy link
Contributor Author

Keruspe commented May 23, 2017

That should fix #42173

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented May 23, 2017

📌 Commit 53ae00a has been approved by alexcrichton

@arielb1 arielb1 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 May 23, 2017
frewsxcv added a commit to frewsxcv/rust that referenced this pull request May 24, 2017
rustbuild: don't create a source tarball when installing

This splits Install out of Dist as it is not a full dist anymore, and creates the source tarball only for the Dist command.
This will allow splitting install in a few rules if we want as it's done for other phases.
frewsxcv added a commit to frewsxcv/rust that referenced this pull request May 24, 2017
rustbuild: don't create a source tarball when installing

This splits Install out of Dist as it is not a full dist anymore, and creates the source tarball only for the Dist command.
This will allow splitting install in a few rules if we want as it's done for other phases.
Signed-off-by: Marc-Antoine Perennou <[email protected]>
@Keruspe
Copy link
Contributor Author

Keruspe commented May 24, 2017

Oh, I somehow missed the second part of distcheck in my previous fix.

@Keruspe
Copy link
Contributor Author

Keruspe commented May 24, 2017

@alexcrichton want me to squash the recent fixes?

Distcheck is fine locally now and nothing else seems to need a dependency update

@alexcrichton
Copy link
Member

@bors: r+

Nah looks good to me, thanks for the quick fixes!

@bors
Copy link
Contributor

bors commented May 25, 2017

📌 Commit d0ea705 has been approved by alexcrichton

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request May 26, 2017
rustbuild: don't create a source tarball when installing

This splits Install out of Dist as it is not a full dist anymore, and creates the source tarball only for the Dist command.
This will allow splitting install in a few rules if we want as it's done for other phases.
@bors
Copy link
Contributor

bors commented May 27, 2017

⌛ Testing commit d0ea705 with merge a11c26f...

bors added a commit that referenced this pull request May 27, 2017
rustbuild: don't create a source tarball when installing

This splits Install out of Dist as it is not a full dist anymore, and creates the source tarball only for the Dist command.
This will allow splitting install in a few rules if we want as it's done for other phases.
@bors
Copy link
Contributor

bors commented May 27, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing a11c26f to master...

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