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

Keruspe · 2017-05-20T05:38:17Z

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 · 2017-05-20T05:38:28Z

r? @alexcrichton

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

alexcrichton · 2017-05-20T16:42:31Z

src/bootstrap/step.rs

@@ -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"))


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?

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.

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?

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.

Keruspe · 2017-05-21T09:26:07Z

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.

alexcrichton

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.

alexcrichton · 2017-05-21T15:34:10Z

src/bootstrap/step.rs

+         .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)


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

alexcrichton · 2017-05-21T15:34:47Z

src/bootstrap/dist.rs

+    // Create the version file
+    write_file(&plain_dst_src.join("version"), build.rust_version().as_bytes());
+
+    if let Subcommand::Dist { .. } = build.flags.cmd {


Is this new clause still necessary?

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

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

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.

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?

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

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

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?

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.

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 · 2017-05-21T19:29:06Z

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 · 2017-05-22T14:00:06Z

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

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

Keruspe · 2017-05-22T18:45:15Z

This part should be better now!

alexcrichton · 2017-05-22T20:07:42Z

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 <Marc-Antoine@Perennou.com>

Keruspe · 2017-05-22T20:13:10Z

rebased and squashed

alexcrichton · 2017-05-22T20:15:02Z

@bors: r+

bors · 2017-05-22T20:15:03Z

📌 Commit 150d644 has been approved by alexcrichton

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 <Marc-Antoine@Perennou.com>

Keruspe · 2017-05-23T16:18:41Z

That should fix #42173

alexcrichton · 2017-05-23T17:01:04Z

@bors: r+

bors · 2017-05-23T17:01:05Z

📌 Commit 53ae00a has been approved by alexcrichton

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 <Marc-Antoine@Perennou.com>

Keruspe · 2017-05-24T06:47:23Z

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

Keruspe · 2017-05-24T20:19:59Z

@alexcrichton want me to squash the recent fixes?

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

alexcrichton · 2017-05-25T03:35:35Z

@bors: r+

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

bors · 2017-05-25T03:35:36Z

📌 Commit d0ea705 has been approved by alexcrichton

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 · 2017-05-27T11:23:52Z

⌛ Testing commit d0ea705 with merge a11c26f...

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 · 2017-05-27T13:40:37Z

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

rust-highfive assigned alexcrichton May 20, 2017

alexcrichton reviewed May 20, 2017

View reviewed changes

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

alexcrichton reviewed May 21, 2017

View reviewed changes

Keruspe force-pushed the master branch from cd48d42 to 150d644 Compare May 22, 2017 20:12

frewsxcv mentioned this pull request May 23, 2017

Rollup of 5 pull requests #42173

Closed

Keruspe added 2 commits May 23, 2017 18:13

rustbuild: drop unused macro in tets

9af464b

Signed-off-by: Marc-Antoine Perennou <Marc-Antoine@Perennou.com>

rustbuild: make distcheck depend on dist-plain-source-tarball

53ae00a

Signed-off-by: Marc-Antoine Perennou <Marc-Antoine@Perennou.com>

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 mentioned this pull request May 24, 2017

Rollup of 8 pull requests #42184

Closed

frewsxcv mentioned this pull request May 24, 2017

Rollup of 8 pull requests #42185

Closed

rustbuild: distcheck needs rust-src too

d0ea705

Signed-off-by: Marc-Antoine Perennou <Marc-Antoine@Perennou.com>

Mark-Simulacrum mentioned this pull request May 26, 2017

Rollup of 6 pull requests #42254

Closed

bors merged commit d0ea705 into rust-lang:master May 27, 2017

This was referenced May 27, 2017

Rollup of 11 pull requests #42257

Closed

rust-src: include everything needed to compile libstd with jemalloc #42214

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Keruspe commented May 20, 2017

rust-highfive commented May 20, 2017

alexcrichton May 20, 2017

Keruspe May 20, 2017

alexcrichton May 20, 2017

Keruspe May 20, 2017

Keruspe commented May 21, 2017

alexcrichton left a comment

alexcrichton May 21, 2017

alexcrichton May 21, 2017

Keruspe May 21, 2017

Keruspe May 21, 2017

Keruspe May 22, 2017

alexcrichton May 22, 2017

alexcrichton May 22, 2017

Keruspe May 22, 2017

alexcrichton May 22, 2017

Keruspe May 22, 2017

alexcrichton May 22, 2017

Keruspe commented May 21, 2017

alexcrichton commented May 22, 2017

Keruspe commented May 22, 2017

alexcrichton commented May 22, 2017

Keruspe commented May 22, 2017

alexcrichton commented May 22, 2017

bors commented May 22, 2017

Keruspe commented May 23, 2017

alexcrichton commented May 23, 2017

bors commented May 23, 2017

Keruspe commented May 24, 2017

Keruspe commented May 24, 2017

alexcrichton commented May 25, 2017

bors commented May 25, 2017

bors commented May 27, 2017

bors commented May 27, 2017

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

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

Conversation

Keruspe commented May 20, 2017

rust-highfive commented May 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Keruspe commented May 21, 2017

alexcrichton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Keruspe commented May 21, 2017

alexcrichton commented May 22, 2017

Keruspe commented May 22, 2017

alexcrichton commented May 22, 2017

Keruspe commented May 22, 2017

alexcrichton commented May 22, 2017

bors commented May 22, 2017

Keruspe commented May 23, 2017

alexcrichton commented May 23, 2017

bors commented May 23, 2017

Keruspe commented May 24, 2017

Keruspe commented May 24, 2017

alexcrichton commented May 25, 2017

bors commented May 25, 2017

bors commented May 27, 2017

bors commented May 27, 2017