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

Update to the Rusty rust-installer #41843

Merged
merged 4 commits into from
May 16, 2017
Merged

Conversation

cuviper
Copy link
Member

@cuviper cuviper commented May 8, 2017

This updates the rust-installer submodule to the new version written in Rust (rust-lang/rust-installer#62), now moved to src/tools/rust-installer and invoked in dist.rs as a cargo-based tool command. All of the former shell-script invocations now invoke the tool, otherwise keeping the same arguments as before.

As a small bonus, rustc-src now also uses the same tarball generator, so it gains a smaller .tar.xz too.

Fixes #41569. r? @alexcrichton

@cuviper
Copy link
Member Author

cuviper commented May 8, 2017

All of the generated dist tarballs look sane to me, and seem to install manually just fine.

However, this does fail one bootstrap test:

step: Step { name: "build-crate-std", stage: 0, host: "A", target: "A" }
step: Step { name: "build-crate-test", stage: 0, host: "A", target: "A" }
step: Step { name: "build-crate-rustc-main", stage: 0, host: "A", target: "A" }
step: Step { name: "build-crate-std", stage: 0, host: "A", target: "B" }
thread 'step::tests::dist_host_with_target_flag' panicked at 'assertion failed: step.target != "B"', step.rs:1582

I think that may just be a reflection of the fact that "dist" now depends on a rust-based tool, but I'd like a second opinion before I go clobbering that test into submission.

cc @brson This re-introduces the file-sorting optimization in the tarballs, so if there's a way to test rustup against this, we should. I'm now writing all directory names in the tarball before the sorted file list, so hopefully that will be enough to avoid a recurrence of rust-lang/rustup#1092.

@@ -755,6 +766,7 @@ pub fn build_rules<'a>(build: &'a Build) -> Rules {
.host(true)
.only_host_build(true)
.dep(|s| s.name("tool-cargo"))
.dep(|s| s.name("tool-rust-installer").stage(0))
Copy link
Member

Choose a reason for hiding this comment

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

Oh here (and in other steps) I think you'll want to lock the Step here to build.config.build for both the host and the target triple, that's the architecture we want to build the tool for (as we're running it here, not packaging it)

@alexcrichton
Copy link
Member

If you've got a checkout locally you may also want to run cargo vendor followed by ./x.py test src/tools/tidy, that will do a license check of crates in src/vendor to see whether it'll pass the distcheck bot (which'll do this automatically)

@bors
Copy link
Contributor

bors commented May 9, 2017

☔ The latest upstream changes (presumably #41846) made this pull request unmergeable. Please resolve the merge conflicts.

@arielb1 arielb1 added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 9, 2017
@cuviper
Copy link
Member Author

cuviper commented May 9, 2017

  • Rebased for the Cargo.lock conflict
  • Fixed step::tests::dist_host_with_target_flag by making the rust-installer dep build-only as suggested.
  • Verified that tidy's vendor license check passes.

@alexcrichton
Copy link
Member

@bors: r+

How exciting!

@bors
Copy link
Contributor

bors commented May 9, 2017

📌 Commit 8eaff4d has been approved by alexcrichton

@frewsxcv
Copy link
Member

@cuviper
Copy link
Member Author

cuviper commented May 10, 2017

Hmm, OK, I'll have to take a closer look at what sanitize_sh is doing. It might actually be harmful now.

@cuviper
Copy link
Member Author

cuviper commented May 10, 2017

OK, I removed most of the sanitize_sh calls, where they don't actually reach shell scripts anymore. I don't have a working Windows environment to be sure this suffices though...

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented May 10, 2017

📌 Commit 2729b71 has been approved by alexcrichton

frewsxcv added a commit to frewsxcv/rust that referenced this pull request May 11, 2017
…richton

Update to the Rusty rust-installer

This updates the rust-installer submodule to the new version written in Rust (rust-lang/rust-installer#62), now moved to `src/tools/rust-installer` and invoked in `dist.rs` as a cargo-based tool command.  All of the former shell-script invocations now invoke the tool, otherwise keeping the same arguments as before.

As a small bonus, `rustc-src` now also uses the same tarball generator, so it gains a smaller `.tar.xz` too.

Fixes rust-lang#41569.  r? @alexcrichton
@frewsxcv
Copy link
Member

1 similar comment
@frewsxcv
Copy link
Member

@frewsxcv frewsxcv closed this May 11, 2017
@frewsxcv frewsxcv reopened this May 11, 2017
@cuviper
Copy link
Member Author

cuviper commented May 11, 2017

Hmm, it's a link error? I'm confused by this, as it built fine in the previous appveyor run, and I didn't change rust-installer itself since then. Has something else changed in the Windows build system?

  = note: LINK : warning LNK4098: defaultlib 'MSVCRT' conflicts with use of other libs; use /NODEFAULTLIB:library
          liblzma_sys-f02a9e47b09e132e.rlib(common.obj) : warning LNK4217: locally defined symbol calloc imported in function lzma_alloc_zero
          liblzma_sys-f02a9e47b09e132e.rlib(common.obj) : warning LNK4217: locally defined symbol free imported in function lzma_end
          liblzma_sys-f02a9e47b09e132e.rlib(common.obj) : warning LNK4217: locally defined symbol malloc imported in function lzma_alloc
          liblzma_sys-f02a9e47b09e132e.rlib(lz_encoder.obj) : warning LNK4217: locally defined symbol memmove imported in function fill_window
          liblzma_sys-f02a9e47b09e132e.rlib(lzma_decoder.obj) : warning LNK4049: locally defined symbol memmove imported
          liblzma_sys-f02a9e47b09e132e.rlib(simple_coder.obj) : warning LNK4049: locally defined symbol memmove imported
          liblzma_sys-f02a9e47b09e132e.rlib(stream_encoder_mt.obj) : error LNK2019: unresolved external symbol __imp__beginthreadex referenced in function initialize_new_thread
          C:\projects\rust\build\x86_64-pc-windows-msvc\stage0-tools\x86_64-pc-windows-msvc\release\deps\rust_installer-32c88478981b650b.exe : fatal error LNK1120: 1 unresolved externals
          
error: aborting due to previous error
error: Could not compile `installer`.
To learn more, run the command again with --verbose.

@alexcrichton
Copy link
Member

Oh I think that's a legitimate bug with the lzma-sys crate, it's right now apparently not compatible with -C target-feature=+crt-static, I'll look into fixing that.

@alexcrichton
Copy link
Member

Looks like a fix was indeed needed, once that passes CI I'll publish to crates.io and you can update the dep here

@cuviper
Copy link
Member Author

cuviper commented May 11, 2017

@alexcrichton I noticed that alexcrichton/xz2-rs#2 changed the VS version, which hasn't reached crates.io yet. Will that be a problem here?

bors added a commit that referenced this pull request May 12, 2017
@Mark-Simulacrum
Copy link
Member

I suspect this failure in the rollup was caused by this PR.

@bors r-

[01:24:22] Dist extended stage2 (x86_64-pc-windows-msvc)
[01:27:05] thread 'main' panicked at 'fs::read_dir(src) failed with The system cannot find the path specified. (os error 3)', src\bootstrap\util.rs:61
[01:27:05] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[01:27:05] Build completed unsuccessfully in 1:15:33

@cuviper
Copy link
Member Author

cuviper commented May 13, 2017

Do you have the full appveyor log? or a link to that build?
(but I probably won't be able to work on this over the weekend...)

@Mark-Simulacrum
Copy link
Member

@cuviper
Copy link
Member Author

cuviper commented May 13, 2017

Oh, I see. This is due to an off-hand optimization I made in rewriting combine-installer.sh. After extracting the input tarballs, instead of copying the components to the combined location, I just moved them. But here, both darwin and windows are expecting to still find those extracted components in the work-dir so they can also copy them into their OS-specific installers.

I don't love that sort of implicit expectation, but I'll go revert that part in rust-installer with a comment about this behavior. The alternative is to just extract them again explicitly -- which is also tempting.

@cuviper
Copy link
Member Author

cuviper commented May 13, 2017

Hmm, or they could get components from the combined location. I think I like that better, if it works...

@cuviper
Copy link
Member Author

cuviper commented May 13, 2017

OK, updated rust-installer to stop moving component dirs.

cc @Keruspe the submodule update now includes your changes too.

@bors
Copy link
Contributor

bors commented May 14, 2017

☔ The latest upstream changes (presumably #41987) made this pull request unmergeable. Please resolve the merge conflicts.

@Mark-Simulacrum
Copy link
Member

@cuviper Looks like you'll need to rebase to fix the merge conflict on Cargo.lock.

@ranma42
Copy link
Contributor

ranma42 commented May 14, 2017

Also let me warn you in advance about alexcrichton/xz2-rs#4
On FreeBSD, lzma-sys 0.1.2 does not build, you need 0.1.3+

This gives us an extra rustc-src.tar.xz, which is 33% smaller than the .tar.gz!
There's no shell interpreting the file paths under the new Rusty
rust-installer, so we don't need to use `sanitize_sh` for it.  Plus,
the drive-letter transformation is actually harmful for the now-native
Windows rust-installer to understand those paths.
@cuviper
Copy link
Member Author

cuviper commented May 15, 2017

OK, rebased, and I squashed the trivial dependency fixups. I have included lzma-sys 0.1.3 as suggested.

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented May 15, 2017

📌 Commit 1e709fc has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented May 15, 2017

⌛ Testing commit 1e709fc with merge abe1183...

bors added a commit that referenced this pull request May 15, 2017
Update to the Rusty rust-installer

This updates the rust-installer submodule to the new version written in Rust (rust-lang/rust-installer#62), now moved to `src/tools/rust-installer` and invoked in `dist.rs` as a cargo-based tool command.  All of the former shell-script invocations now invoke the tool, otherwise keeping the same arguments as before.

As a small bonus, `rustc-src` now also uses the same tarball generator, so it gains a smaller `.tar.xz` too.

Fixes #41569.  r? @alexcrichton
@bors
Copy link
Contributor

bors commented May 16, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants