Fix 45345 #46592

o01eg · 2017-12-08T20:24:45Z

There is a fix for #45345

It re-introduces CFG_LIBDIR_RELATIVE which was broken when migration from configure script to x.py.

Other commits fix errors which happen after rustbuild cleanups.

rust-highfive · 2017-12-08T20:24:52Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @aturon (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

aidanhs · 2017-12-14T11:05:07Z

Thanks for the PR @o01eg! We'll check in now and again to make sure this gets reviewed soon.

r? @Mark-Simulacrum

Mark-Simulacrum · 2017-12-15T02:05:08Z

Changes seem fine, but I'd like to r? @alexcrichton since I'm not entirely familiar with what the flag is meant to do.

alexcrichton · 2017-12-16T15:06:39Z

src/bootstrap/config.rs

@@ -416,6 +416,7 @@ impl Config {
            config.docdir = install.docdir.clone().map(PathBuf::from);
            config.bindir = install.bindir.clone().map(PathBuf::from);
            config.libdir = install.libdir.clone().map(PathBuf::from);
+            config.libdir_relative = install.libdir.clone().map(PathBuf::from);


Hm so actually I think this may be an old vestige that just hasn't actually been removed. Could the libdir-relative key get removed in favor of inferring it from libdir?

I've removed libdir_relative from Config struct.

alexcrichton · 2017-12-16T15:06:53Z

src/bootstrap/compile.rs

@@ -500,7 +500,7 @@ impl Step for Rustc {

 /// Same as `std_cargo`, but for libtest
 pub fn rustc_cargo(build: &Build,
-                   compiler: &Compiler,
+                   _compiler: &Compiler,


If this argument remains unused, could it be removed?

Yep, I've added commit to remove unused argument.

alexcrichton · 2017-12-16T15:07:41Z

src/bootstrap/doc.rs

        let compiler = if build.force_use_stage1(compiler, target) {
+            dylib_path.push(builder.rustc_libdir(compiler));


Hm could you detail a bit why this (and the above one) is needed? Shouldn't the rpath configuration cover this?

It builds with rpath disabled because with rpath enabled it ends up with long library path with .. directories.

Hm when you say "it" in "it builds with rpath disabled" what is that referring to?

I mean Rust installation script which builds Rust from source.

I believe the rpath should be configured here and IIRC that's enabled by default, but are you disabling that?

No, its just change path to libdir but there need to be stage1 libdir to generate docs and stage2 libdir for rustdoc stage2 executable.

I've tried to set RUSTC_LIBDIR to stage2 libdir but https://github.com/rust-lang/rust/blob/master/src/bootstrap/doc.rs#L445 use stage1 rustc so it fails.

I wonder if it is better to send libdir for stage2 in environment variable RUSTDOC_LIBDIR or so instead set it with add_lib_path?

Sure yeah adding RUSTDOC_LIBDIR sounds good! I think it'd be best to reuse what's already in the shim executables instead of adding more paths here, but hopefully isn't too bad to add!

Reimplemented this with RUSTDOC_LIBDIR.

o01eg · 2017-12-16T20:48:04Z

Rebased and removed unused data.

o01eg · 2017-12-20T15:09:32Z

Rebased. Introduce environment variable RUSTDOC_LIBDIR to generate rustc documentation instead of putting original libdir path to libdir in library path.

o01eg · 2017-12-21T03:37:13Z

Looks like issue with doctest was fixed.

alexcrichton · 2017-12-21T15:10:07Z

Nice! Could the rustdoc_cargo function get folded directly into the cargo function perhaps?

o01eg · 2017-12-21T18:46:36Z

I did it when tried different solutions for library path issue and reject it because it needs additional optional argument for Builder::cargo which would be used for small subset of Builder::cargo usages.

alexcrichton · 2017-12-23T15:50:35Z

Do things break if the function is folded in? Functions like this are notoriously hard to get right because there's very little testing, so having the build system be as non-surprising as possible is often best for long-term maintenance.

o01eg · 2017-12-23T17:03:12Z

Do things break if the function is folded in?

No, things don't break but I decided it is simple to add call to additional function in some places than add additional argument in all places. I could change a way if it looks better.

alexcrichton · 2017-12-25T15:47:06Z

Yeah if it's ok let's fold it in to reduce the amount of boilerplate at callsites.

o01eg · 2017-12-26T03:13:21Z

Ok, I've rewritten to additional argument.

alexcrichton · 2017-12-26T21:10:36Z

src/bootstrap/builder.rs

@@ -437,7 +437,8 @@ impl<'a> Builder<'a> {
             compiler: Compiler,
             mode: Mode,
             target: Interned<String>,
-             cmd: &str) -> Command {
+             cmd: &str,
+             rustdoc_libdir: Option<PathBuf>) -> Command {


Hm isn't this argument always builder.rustc_libdir(compiler); basically? If so, could this avoid being passed as an argument and just be set internally?

No, compiler here could be stage1 if Build::force_use_stage1 returns true but real launched compiler will be from stage2

Indeed yeah but rustdoc doesn't work at all in stage 1, so I think this is always the stage2 version for rustdoc?

Yes, so real stage2 rustdoc binary is choosen with REAL_RUSTDOC environment variable while all other parameters from stage1.

Er so my point is that this argument is a constant, it's always generated from a stage2 compiler. In that case we can just inline it here, right?

I've tried to do it with

cargo.env("RUSTDOC_LIBDIR", self.rustc_libdir(self.compiler(2, self.build.build)));

but it fails with error:

thread 'main' panicked at ' Cycle in build detected when adding Assemble { target_compiler: Compiler { stage: 2, host: "x86_64-unknown-linux-gnu" } } Any Any Any Any Any ', bootstrap/builder.rs:673:17

Hm, does the same error happen if you only set this env var when the command is "doc"?

I'll try it but it also required here https://github.com/o01eg/rust/blob/50a80028052a4ca4ad939e926032bce8ac4d3cdd/src/bootstrap/check.rs#L1178
Are there "test" command?

Sure yeah, maybe set it for everything that's not "build"?

Yep, it helped.

carols10cents · 2018-01-09T14:53:31Z

r? @alexcrichton

Looks like there's some outstanding questions for you, @alexcrichton !

Re-implement ```bash CFG_LIBDIR_RELATIVE=`echo ${CFG_LIBDIR} | cut -c$((${#CFG_PREFIX}+${CAT_INC}))-` ``` from old `configure` script.

…ariable.

o01eg · 2018-01-12T08:55:30Z

Rebased. Generate RUSTDOC_LIBDIR for stage 2 only.

alexcrichton · 2018-01-12T15:21:12Z

@bors: r+

Thanks!

bors · 2018-01-12T15:21:13Z

📌 Commit 472f4e1 has been approved by alexcrichton

bors · 2018-01-12T15:52:30Z

⌛ Testing commit 472f4e1 with merge 58ecb4753c7dd25cc51cd630f25e2d5fe0b296a5...

bors · 2018-01-12T18:53:17Z

💔 Test failed - status-appveyor

kennytm · 2018-01-13T04:46:11Z

@bors retry

bors · 2018-01-13T05:02:11Z

⌛ Testing commit 472f4e1 with merge 6cf081c...

Fix 45345 There is a fix for #45345 It re-introduces `CFG_LIBDIR_RELATIVE` which was broken when migration from `configure` script to `x.py`. Other commits fix errors which happen after rustbuild cleanups.

bors · 2018-01-13T07:54:24Z

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

o01eg · 2018-01-13T07:58:03Z

Could it be backported to beta due it closes regression-from-stable-to-stable?

kennytm · 2018-01-13T07:59:57Z

I've nominated it, but personally I won't assign a high priority in backporting this since the bug is not regression-from-stable-to-{beta,nightly}.

nikomatsakis · 2018-01-18T15:45:48Z

@o01eg are you using this as part of some packaging scheme? Is it possible to simply apply the patch locally?

nikomatsakis · 2018-01-18T15:46:08Z

We don't usually backport stable-to-stable regressions unless there is a strong need (just because everybackport adds risk).

nikomatsakis · 2018-01-18T15:46:24Z

But it depends also on how recently the regression occurred.

o01eg · 2018-01-18T18:11:43Z

@nikomatsakis Yes, it parts of packaging scheme so it possible to make a patch to fix releases without this PR.

alexcrichton · 2018-01-18T20:03:37Z

Ok thanks for the info @o01eg! In that case I'm going to remove the beta nomination

This re-introduces a `Config.libdir_relative` field, now derived from `libdir` and made relative to `prefix` if necessary. This fixes a regression from rust-lang#46592 when `--libdir` is given an absolute path. `Builder::sysroot_libdir` should always use a relative path so its callers don't clobber system locations, and `librustc` also asserts that `CFG_LIBDIR_RELATIVE` is really relative.

…ulacrum rustbuild: Restore Config.libdir_relative This re-introduces a `Config.libdir_relative` field, now derived from `libdir` and made relative to `prefix` if necessary. This fixes a regression from rust-lang#46592 when `--libdir` is given an absolute path. `Builder::sysroot_libdir` should always use a relative path so its callers don't clobber system locations, and `librustc` also asserts that `CFG_LIBDIR_RELATIVE` is really relative.

This re-introduces a `Config.libdir_relative` field, now derived from `libdir` and made relative to `prefix` if necessary. This fixes a regression from rust-lang#46592 when `--libdir` is given an absolute path. `Builder::sysroot_libdir` should always use a relative path so its callers don't clobber system locations, and `librustc` also asserts that `CFG_LIBDIR_RELATIVE` is really relative.

devurandom · 2018-03-10T11:50:47Z

Is this already in Rust 1.24.1 or will it first appear in Rust 1.25?

Mark-Simulacrum · 2018-03-10T15:00:58Z

I believe this will appear in 1.25.

rust-highfive assigned aturon Dec 8, 2017

kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 9, 2017

alexcrichton reviewed Dec 16, 2017

View reviewed changes

alexcrichton reviewed Dec 26, 2017

View reviewed changes

rust-highfive assigned alexcrichton and unassigned aturon Jan 9, 2018

o01eg added 7 commits January 12, 2018 07:10

Fix #45345.

ee8b578

Re-implement ```bash CFG_LIBDIR_RELATIVE=`echo ${CFG_LIBDIR} | cut -c$((${#CFG_PREFIX}+${CAT_INC}))-` ``` from old `configure` script.

Accept verbosity in rustdoc.

27b4f22

Stage 1 and later use relative libdir.

0f5110e

Build all stages with relative libdirs.

c87a108

Add library path for real rustdoc with RUSTDOC_LIBDIR environment v…

f05282f

…ariable.

Remove unused argument rustc_cargo.

83e76d6

Remove unused configuration parameter libdir_relative.

472f4e1

bors merged commit 472f4e1 into rust-lang:master Jan 13, 2018

o01eg deleted the fix-45345 branch January 13, 2018 07:55

kennytm added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jan 13, 2018

o01eg mentioned this pull request Jan 13, 2018

Use separate lidir. Use cargo from rust distribution. gentoo/gentoo-rust#317

Merged

cuviper mentioned this pull request Jan 18, 2018

./x.py doc fails when using a local_rebuild stage0 #47469

Closed

alexcrichton removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jan 18, 2018

cuviper mentioned this pull request Feb 19, 2018

rustbuild: Restore Config.libdir_relative #48362

Merged

o01eg mentioned this pull request Mar 14, 2019

Fix rustdoc execution on multiple targets and custom libdir #58947

Closed

		let compiler = if build.force_use_stage1(compiler, target) {
		dylib_path.push(builder.rustc_libdir(compiler));

Fix 45345 #46592

Fix 45345 #46592

Conversation

o01eg commented Dec 8, 2017

rust-highfive commented Dec 8, 2017

aidanhs commented Dec 14, 2017

Mark-Simulacrum commented Dec 15, 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

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

o01eg commented Dec 16, 2017

o01eg commented Dec 20, 2017

o01eg commented Dec 21, 2017

alexcrichton commented Dec 21, 2017

o01eg commented Dec 21, 2017

alexcrichton commented Dec 23, 2017

o01eg commented Dec 23, 2017 • edited Loading

alexcrichton commented Dec 25, 2017

o01eg commented Dec 26, 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

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

carols10cents commented Jan 9, 2018

o01eg commented Jan 12, 2018

alexcrichton commented Jan 12, 2018

bors commented Jan 12, 2018

bors commented Jan 12, 2018

bors commented Jan 12, 2018

kennytm commented Jan 13, 2018

bors commented Jan 13, 2018

bors commented Jan 13, 2018

o01eg commented Jan 13, 2018

kennytm commented Jan 13, 2018 • edited Loading

nikomatsakis commented Jan 18, 2018

nikomatsakis commented Jan 18, 2018

nikomatsakis commented Jan 18, 2018

o01eg commented Jan 18, 2018

alexcrichton commented Jan 18, 2018

devurandom commented Mar 10, 2018

Mark-Simulacrum commented Mar 10, 2018

o01eg commented Dec 23, 2017 •

edited

Loading

kennytm commented Jan 13, 2018 •

edited

Loading