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

x.py install doesn't install binaries. #80683

Closed
vext01 opened this issue Jan 4, 2021 · 8 comments
Closed

x.py install doesn't install binaries. #80683

vext01 opened this issue Jan 4, 2021 · 8 comments
Labels
C-bug Category: This is a bug. P-high High priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Milestone
1.50.0

Comments

@vext01
Copy link
Contributor

vext01 commented Jan 4, 2021 •
edited
Loading

CC @pietroalbini

On today's git (0cd459f) x.py install with the config below doesn't install any binaries.

Here's the config:

[build]
docs = false

[install]
prefix = "build/ykrustc-stage2-latest"
sysconfdir = "etc"

docs = false is needed to work around a different x.py bug where installing the docs hangs.

After x.py install the resulting install dir contains:

build/ykrustc-stage2-latest/
build/ykrustc-stage2-latest/lib
build/ykrustc-stage2-latest/lib/rustlib
build/ykrustc-stage2-latest/lib/rustlib/manifest-rust-docs
build/ykrustc-stage2-latest/lib/rustlib/install.log
build/ykrustc-stage2-latest/lib/rustlib/rust-installer-version
build/ykrustc-stage2-latest/lib/rustlib/uninstall.sh
build/ykrustc-stage2-latest/lib/rustlib/components
build/ykrustc-stage2-latest/share
build/ykrustc-stage2-latest/share/doc
build/ykrustc-stage2-latest/share/doc/rust
build/ykrustc-stage2-latest/share/doc/rust/html
build/ykrustc-stage2-latest/share/doc/rust/html/SourceCodePro-Regular.woff
build/ykrustc-stage2-latest/share/doc/rust/html/ayu1.51.0.css
build/ykrustc-stage2-latest/share/doc/rust/html/src
build/ykrustc-stage2-latest/share/doc/rust/html/src/std
build/ykrustc-stage2-latest/share/doc/rust/html/src/std/prelude
build/ykrustc-stage2-latest/share/doc/rust/html/src/std/prelude/mod.rs.html
build/ykrustc-stage2-latest/share/doc/rust/html/src/std/prelude/v1.rs.html
build/ykrustc-stage2-latest/share/doc/rust/html/src/std/macros.rs.html
build/ykrustc-stage2-latest/share/doc/rust/html/src/std/sys
build/ykrustc-stage2-latest/share/doc/rust/html/src/std/sys/unix
build/ykrustc-stage2-latest/share/doc/rust/html/src/std/sys/unix/os.rs.html
build/ykrustc-stage2-latest/share/doc/rust/html/src/std/sys/unix/futex.rs.html
...
build/ykrustc-stage2-latest/share/doc/rust/html/std/collections/hash_set
build/ykrustc-stage2-latest/share/doc/rust/html/std/collections/linked_list
build/ykrustc-stage2-latest/share/doc/rust/html/std/collections/hash
build/ykrustc-stage2-latest/share/doc/rust/html/std/collections/hash/map

Also odd that some docs are installed?

Thanks!
@vext01 vext01 added the C-bug Category: This is a bug. label Jan 4, 2021
@vext01 vext01 mentioned this issue Jan 5, 2021
Merged
@pietroalbini pietroalbini added the regression-from-stable-to-beta Performance or correctness regression from stable to beta. label Jan 6, 2021
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Jan 6, 2021
@pietroalbini pietroalbini mentioned this issue Jan 6, 2021
Closed
6 tasks
@pietroalbini
Copy link
Member

@vext01 tried doing a ./x.py install, and it seems that all the files are properly installed, and docs are not installed. Could you try removing the build, build/ykrustc-stage2-latest and etc directories?

@LeSeulArtichaut LeSeulArtichaut added the T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. label Jan 6, 2021
@pietroalbini pietroalbini added P-high High priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Jan 7, 2021
@vext01
Copy link
Contributor Author

vext01 commented Jan 7, 2021 •
edited
Loading

I nuked my whole git clone and started afresh. Cloned today's git code (b5c496d), put the above config in place, ran x.py install:

$ x.py install
...
   Compiling tempfile v3.1.0
   Compiling regex v1.3.9
   Compiling rustdoc v0.0.0 (/home/vext01/source/rust/src/librustdoc)
   Compiling rustdoc-tool v0.0.0 (/home/vext01/source/rust/src/tools/rustdoc)
    Finished release [optimized] target(s) in 1m 32s
Dist rustc-1.51.0-dev-x86_64-unknown-linux-gnu
        finished in 40.785 seconds
Install rustc stage2 (Some(TargetSelection { triple: "x86_64-unknown-linux-gnu", file: None }))
install: creating uninstall script at /home/vext01/source/rust/build/tmp/empty_dir/build/ykrustc-stage2-latest/lib/rustlib/uninstall.sh
install: installing component 'rustc'

    rustc installed.

warning: x.py has made several changes recently you may want to look at
help: consider looking at the changes in `src/bootstrap/CHANGELOG.md`
note: to silence this warning, add `changelog-seen = 2` at the top of `config.toml`
note: this message was printed twice to make it more likely to be seen
Build completed successfully in 0:28:40
$ find build/ykrustc-stage2-latest/
build/ykrustc-stage2-latest/
$

So it seems it created an empty directory this time.

So do we suspect this is something local to me, if you see something else?

@vext01
Copy link
Contributor Author

vext01 commented Jan 7, 2021

I also tried a different prefix outside of build and got the same behaviour.

This is odd...

@pietroalbini
Copy link
Member

I found out why my local result and your local result differ, and the piece of code that change it, but I still need to figure out why my change triggered the bug.

If we look at the log you posted, the path in this line has an extra build/tmp/empty_dir in it:

install: creating uninstall script at /home/vext01/source/rust/build/tmp/empty_dir/build/ykrustc-stage2-latest/lib/rustlib/uninstall.sh

That means the installation script is running successfully, but it's installing stuff on the wrong prefix, which is then deleted at the end of this snippet:

rust/src/bootstrap/install.rs

Lines 67 to 83 in b5c496d

let empty_dir = builder.out.join("tmp/empty_dir");
t!(fs::create_dir_all(&empty_dir));
let mut cmd = Command::new("sh");
cmd.current_dir(&empty_dir)
.arg(sanitize_sh(&tarball.decompressed_output().join("install.sh")))
.arg(format!("--prefix={}", sanitize_sh(&prefix)))
.arg(format!("--sysconfdir={}", sanitize_sh(&sysconfdir)))
.arg(format!("--datadir={}", sanitize_sh(&datadir)))
.arg(format!("--docdir={}", sanitize_sh(&docdir)))
.arg(format!("--bindir={}", sanitize_sh(&bindir)))
.arg(format!("--libdir={}", sanitize_sh(&libdir)))
.arg(format!("--mandir={}", sanitize_sh(&mandir)))
.arg("--disable-ldconfig");
builder.run(&mut cmd);
t!(fs::remove_dir_all(&empty_dir));

I was not reproducing the error myself because I used absolute paths in my config.toml. I'll spend some time right now figuring out how my PRs triggered this bug.

@pietroalbini
Copy link
Member

Found the cause of the regression! #80240 moved the path canonicalization code, causing the prefix path to be absolute while all the rest of the paths are relative. Before that PR all paths were instead absolute.

Working on a PR now.

@pietroalbini pietroalbini mentioned this issue Jan 7, 2021
Merged
@pietroalbini
Copy link
Member

Opened a PR with a fix: #80797

bors added a commit to rust-lang-ci/rust that referenced this issue Jan 9, 2021
@bors
Auto merge of rust-lang#80797 - pietroalbini:fix-relative-install, r=…
46c35c7 
…Mark-Simulacrum

Fix x.py install not working with relative prefix

The code powering `./x.py install` did not handle relative paths well: the installation script is executed inside a temporary directory, so all the relative paths specified in `config.toml` and in the `DESTDIR` environment variable were relative to that temporary directory. The original code fixed the problem for `config.toml` paths by canonicalizing the prefix, but breaking `DESTDIR`. rust-lang#80240 fixed the `DESTDIR` problem, but also regressed `config.toml` paths (rust-lang#80683).

This PR refactors the installation code to generate paths that *in my understanding* are correct, adding comments in the meantime to explain what each step does. There was no documentation on why choices were made before, so my understanding could actually be wrong.

Regardless, executed `./x.py install` with various combinations of `config.toml` and `DESTDIR` paths, and everything seems to work according to my understanding. Still, I'd love if `@vext01` and `@yshui` could test these changes.

r? `@Mark-Simulacrum`
`@rustbot` modify labels: beta-nominated T-infra
@Mark-Simulacrum Mark-Simulacrum added this to the 1.50.0 milestone Jan 14, 2021
@wesleywiser
Copy link
Member

@pietroalbini Since #80797 has merged, can this now be closed?

@pietroalbini
Copy link
Member

I kept it open waiting for a backport, but it landed in #81151! Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
C-bug Category: This is a bug. P-high High priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.50.0
Development

No branches or pull requests
6 participants
@vext01 @wesleywiser @pietroalbini @Mark-Simulacrum @LeSeulArtichaut @rustbot