Get make check-stage1 working again
#14000
Conversation
| @@ -1,9 +1,21 @@ | |||
| -include ../tools.mk | |||
|
|
|||
| # This test attempts to run rustc itself from the compiled binary; but | |||
| # that means that you need to set the LD_LIBRARY_PATH for rustc itself | |||
| # while running muliple_files, and that won't work for stage1. | |||
There was a problem hiding this comment.
typo: "muliple_files" (will fix, don't let it hold up review).
|
I'm a little worried about ignoring the rustdoc tests. There's no reason they shouldn't be able to be run at stage1, other than the differences in LD_LIBRARY_PATH, and I think that's only applicable to syntax extensions really. Could you try this patch and see if it fixes rustdoc tests at stage1? diff --git a/src/librustdoc/test.rs b/src/librustdoc/test.rs
index 01cbd15..481fee3 100644
--- a/src/librustdoc/test.rs
+++ b/src/librustdoc/test.rs
@@ -144,12 +144,15 @@ fn runtest(test: &str, cratename: &str, libs: HashSet<Path>, should_fail: bool,
let outdir = TempDir::new("rustdoctest").expect("rustdoc needs a tempdir");
let out = Some(outdir.path().clone());
let cfg = driver::build_configuration(&sess);
+ let path = sess.target_filesearch().get_lib_path();
driver::compile_input(sess, cfg, &input, &out, &None);
if no_run { return }
// Run the code!
let exe = outdir.path().join("rust_out");
+ ::std::unstable::dynamic_lib::DynamicLibrary::add_search_path(
+ &path);
let out = Process::output(exe.as_str().unwrap(), []);
match out {
Err(e) => fail!("couldn't run the test: {}{}", e,I don't think that the patch is quite what we want in the long term, but I think that just adding the target search path should help. If it's still loading the wrong library, it may need this patch, but that's just a guess... |
|
@alexcrichton yeah I was just running a p.s. thanks for the suggested patches, I will see if they help things. update: I was wrong on two counts; The failures I mentioned above we encountered using |
|
Could you paste the errors you've been seeing? I'm curious myself! |
|
@alexcrichton I misspoke earlier; it was actually compilation-fail failures running |
|
and actually, I think the failures I listed above occur on the master branch as well. That is, this may just be a general problem we have with running |
|
it looks to me like the right solution here is to factor out the compile-fail tests that rely on syntax extensions in a separate crate into a separate directory, analogous to our run-pass-fulldeps. I will work on that and file separate PR for it. |
|
(closing this until I get |
Our `make check-stageN` for all N (including N=2) was not working, because there is an unspecified dependence from the cfail tests on certain crates that export syntax. We need to encode that dependence in some manner. The quick hack is to add `$$(CSREQ$(1)_T_$(3)_H_$(3)) $$(SREQ$(1)_T_$(2)_H_$(3))` to the dependency list for *all* of the cfail tests. But I would prefer to not force the make logic to build all of the external crates before it gets to exercise the cfail tests. So, here is how this PR fixes the problem more properly: * Add `CTEST_DEPS_cfail-full` that, analogous to `CTEST_DEPS_rpass-full`, has a dependency on `$$(CSREQ$(1)_T_$(3)_H_$(3)) $$(SREQ$(1)_T_$(2)_H_$(3))`. `CTEST_DEPS_cfail` is left unchanged (and thus continues to match the structure of `CTEST_DEPS_rpass`). * Added all of the other ingredients for `cfail-full` family of make logic. * moved cfail tests that rely on macros in separate crates to separate compile-fail-fulldeps/ directory. ---- (Landing this may or not be a prerequisite for landing PR rust-lang#14000. In any case, this is a pretty isolated change and a net improvement in the overall structure for the tests, IMO, at least given what we currently have in the makefiles.)
|
filed PR #14014 for the |
|
(reopening since huon gave thumbs up on #14014.) |
|
@alexcrichton so it seems like the Of particular interest: it seems like the override of the Here is an example of what I am seeing: Problems like this were what led me to just disable rustdoc entirely for stage1 rather than attempt to decipher where I needed to adjust the |
|
I discussed this with @pnkfelix on IRC. We think we have a fix for rustdoc stage1 tests, but we'll land this in the meantime while the solution cooks. |
|
(Update from pnkfelix: alex is above referring to the branch of work that I have saved at commit pnkfelix/rust@10c46eb ; which I may indeed still try to land if I cannot resolve my problems with his suggested revised patch to get rustdoc working within The discussion log starts here: https://botbot.me/mozilla/rust-internals/msg/14337673/ Our conclusion was that both of the suggestions that @alexcrichton put in earlier comments on this PR (the little patches to prepend the target lib_path to the system dynamic library path env var) do fix the problem locally. But ... they have their own issues (i.e. see nightmare scenarios discussed in the irc log), and so we do not want to adopt those patches. Instead we will put in the more robust solution also discussed in the irc log, where the child process will set up its own |
|
@pnkfelix, if you see this before this merges, would you be willing to include alexcrichton@d2914f8 as part of this PR? It should allow enabling rustdoc at stage1. |
|
@alexcrichton sure, I'll take your r+ off the commit temporarily and see what I can do with alexcrichton/rust@d2914f8 |
|
@alexcrichton I'm going to force-push my adaptation of your patch here, but its not quite working yet; i'lll post the error after I do the force-push. |
|
So atop commit 1dfb5b3, I get an error for I will try to look at this more tomorrow. (Well, maybe. Its Fête de la Victoire tomorrow, so we'll see.) Anyway, thank you for your assistance on this so far alex, it has really gotten me over some hurdles. |
|
@alexcrichton or wait, this is an instance of a case where we just aren't going to work, right? I.e. this problem is probably because of |
|
hmm let me see if rebasing atop a master with PR #13845 landed helps with anything here. |
|
Yes, I would expect |
|
hmm, if I do: then things fail just as they do in the log from the bors attempted run on Linux. But if I do: (note the |
|
Interesting. I would have expected rpaths to take care of this... |
|
@alexcrichton I think the LD_LIBRARY_PATH may be set in the environment in the run-make tests (or at least on this PR's run-make tests)... didn't you say something earlier about that overriding the rpath setting? |
|
Perhaps, requiring it on The LD_LIBRARY_PATH does take precedent over rpath, but the rpath should work in this case because the libboot.so hasn't moved relative to its dependent libraries. |
|
@alexcrichton I wonder if its a linker flag order problem. While trying to make an isolated test case in C to try to replicate this, I noted the following: |
|
The linker will sometimes discard libraries if they don't satisfy any outstanding symbols, so in the second case I think that because |
|
okay, I worked out what was happening (we need to feed an appropriate I still need to make sure fixing linux didn't break mac, but hopefully I'll have an updated PR available soon. |
|
(d'oh needs a rebase) |
See rust-lang#13983 and rust-lang#14000. Conflicts: src/librustc/metadata/filesearch.rs src/libstd/unstable/dynamic_lib.rs
See rust-lang#13983 and rust-lang#14000. Fix was originally authored by alexcrichton and then rebased a couple times by pnkfelix, most recently atop PR 13954.
|
r=me with a rebase |
|
Also, nice catches on basically everything here! |
See rust-lang#13983 and rust-lang#14000. Fix was originally authored by alexcrichton and then rebased a couple times by pnkfelix, most recently atop PR 13954. ---- Regarding the change to librustdoc/lib.rs, to do `map_err` before unwrapping a `TqskResult`: I do not understand how master is passing without this change or something like it, since `Box<Any:Send>` does not implement `Show`. (Is this something that is only a problem for the snapshot stage0 compiler?) Still, the change I have put in here (which was added as part of a rebase after alex's review) seems harmless to me to apply to rustdoc at all stages, since a call to `unwrap` is just going to `fail!` on the err case anyway.
Two line summary: Distinguish HOST_RPATH and TARGET_RPATH; added RPATH_LINK_SEARCH; skip tests broken in stage1; general cleanup. `HOST_RPATH_VAR$(1)_T_$(2)_H_$(3)` and `TARGET_RPATH_VAR$(1)_T_$(2)_H_$(3)` both match the format of the old `RPATH_VAR$(1)_T_$(2)_H_$(3)` (which is still being set the same way that it was before, to one of either HOST/TARGET depending on what stage we are building). Namely, the format is <XXX>_RPATH_VAR = "<LD_LIB_PATH_ENVVAR>=<COLON_SEP_PATH_ENTRIES>" What this commit does: * Pass both of the (newly introduced) HOST and TARGET rpath setup vars to `maketest.py` * Update `maketest.py` to no longer update the LD_LIBRARY_PATH itself Instead, it passes along the HOST and TARGET rpath setup vars in environment variables `HOST_RPATH_ENV` and `TARGET_RPATH_ENV` * Also, pass the current stage number to maketest.py; it in turn passes it (via an env var) to run-make tests. This allows the run-make tests to selectively change behavior (e.g. turn themselves off) to deal with incompatibilities with e.g. stage1. * Cleanup: Distinguish in tools.mk between the command to run (`RUN`) and the file to generate to drive that command (`RUN_BINFILE`). The main thing this enables is that `RUN` can now setup the `TARGET_RPATH_ENV` without having to dirty up the runner code in each of the `run-make` Makefiles. * Cleanup: Factored out commands to delete dylib/rlib into REMOVE_DYLIBS/REMOVE_RLIBS. There were places where we were only calling `rm $(call DYLIB,foo)` even though we really needed to get rid of the whole glob (at least based on alex's findings on rust-lang#13753 that removing the symlink does not suffice). Therefore rather than peppering the code with the awkward `rm $(TMPDIR)/$(call DYLIB_GLOB,foo)`, I instead introduced a common `REMOVE_DYLIBS` user function that expands into that when called. After I adding an analogous `REMOVE_RLIBS`, I changed all of the existing calls that rm dylibs or rlibs to use these routines instead. Note that the latter is not a true refactoring since I may have changed cases where it was our intent to only remove the sym-link. (But if that is the case, then we need to more deeply investigate alex's findings on rust-lang#13753 where the system was still dynamically loading up the non-symlinked libraries that it finds on the load path.) * Added RPATH_LINK_SEARCH command and use it on Linux. On some platforms, namely Linux, when you have libboot.so that has its internal rpath set (to e.g. $(ORIGIN)/path/to/HOSTDIR), the linker still complains when you do the link step and it does not know where to find libraries that libboot.so depends upon that live in HOSTDIR (think e.g. librustuv.so). As far as I can tell, the GNU linker will consult the LD_LIBRARY_PATH as part of the linking process to find such libraries. But if you want to be more careful and not override LD_LIBRARY_PATH for the `gcc` invocation, then you need some other way to tell the linker where it can find the libraries that libboot.so needs. The solution to this on Linux is the `-Wl,-rpath-link` command line option. However, this command line option does not exist on Mac OS X, (which appears to be figuring out how to resolve the libboot.dylib dependency by some other means, perhaps by consulting the rpath setting within libboot.dylib). So, in order to abstract over this distinction, I added the RPATH_LINK_SEARCH macro to the run-make infrastructure and added calls to it where necessary to get Linux working. On architectures other than Linux, the macro expands to nothing. * Disable miscellaneous tests atop stage1. * An especially interesting instance of the previous bullet point: Excuse regex from doing rustdoc tests atop stage1. This was a (nearly-) final step to get `make check-stage1` working again. The use of a special-case check for regex here is ugly but is analogous other similar checks for regex such as the one that landed in PR rust-lang#13844. The way this is written, the user will get a reminder that doc-crate-regex is being skipped whenever their rules attempt to do the crate documentation tests. This is deliberate: I want people running `make check-stage1` to be reminded about which cases are being skipped. (But if such echo noise is considered offensive, it can obviously be removed.) * Got windows working with the above changes. This portion of the commit is a cleanup revision of the (previously mentioned on try builds) re-architecting of how the LD_LIBRARY_PATH setup and extension is handled in order to accommodate Windows' (1.) use of `$PATH` for that purpose and (2.) use of spaces in `$PATH` entries (problematic for make and for interoperation with tools at the shell). * In addition, since the code has been rearchitected to pass the HOST_RPATH_DIR/TARGET_RPATH_DIR rather than a whole sh environment-variable setting command, there is no need to for the convert_path_spec calls in maketest.py, which in fact were put in place to placate Windows but were now causing the Windows builds to fail. Instead we just convert the paths to absolute paths just like all of the other path arguments. Also, note for makefile hackers: apparently you cannot quote operands to `ifeq` in Makefile (or at least, you need to be careful about adding them, e.g. to only one side).
Fix #13732. This is a revised, much less hacky form of PR #13753 The changes here: * add instrumentation to aid debugging of linkage errors, * fine tune some things in the Makefile where we are telling binaries to use a host-oriented path for finding dynamic libraries, when it should be feeding the binaries a target-oriented path for dynamic libraries. * pass along the current stage number to run-make tests, and * skip certain tests when running atop stage1. Fix #13746 as well.
Unsize cast array only on pointer type fix rust-lang#14000
Fix #13732.
This is a revised, much less hacky form of PR #13753
The changes here:
Fix #13746 as well.