-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Port exit-code run-make test to use rust #121884
Conversation
rustbot has assigned @Mark-Simulacrum. Use r? to explicitly pick a reviewer |
This comment has been minimized.
This comment has been minimized.
fn new() -> Self { | ||
let cmd = setup_common_build_cmd(); | ||
Self { cmd } | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe missing a -L $TARGET_RPATH_ENV
?
see
Line 12 in 4cdd205
RUSTDOC := $(BARE_RUSTDOC) -L $(TARGET_RPATH_DIR) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. It also seems to set up $(HOST_RPATH_ENV)
but I don't see that done in the rustc setup, so I assume it's not needed here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. It also seems to set up
$(HOST_RPATH_ENV)
but I don't see that done in the rustc setup, so I assume it's not needed here?
I think it might be required if host/target differs, so it's safer if we make sure HOST_RPATH_ENV
is available to RUSTDOC
here.
HOST_RPATH_ENV = \
$(LD_LIB_PATH_ENVVAR)="$(TMPDIR):$(HOST_RPATH_DIR):$($(LD_LIB_PATH_ENVVAR))"
rustc
setup not having that is an oversight. If you add a common HOST_RPATH_ENV
fn to set up the env var for rustdoc, can you also add it for rustc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, done, added add_host_rpath_env
which i think does what that line of code does?
TARGET_RPATH_ENV isn't implemented but as far as i can see that's not needed yet(?)
? |
😪 I'm awake I'm awake |
✌️ @jieyouxu, you can now approve this pull request! If @workingjubilee told you to " |
☔ The latest upstream changes (presumably #122036) made this pull request unmergeable. Please resolve the merge conflicts. |
3bb1abb
to
12cdbba
Compare
Also while we're at it, could you please add a comment documenting the intent of the test? |
let temp = env::var("TMPDIR").unwrap(); | ||
let host_rpath_dir = env::var("HOST_RPATH_DIR").unwrap(); | ||
|
||
cmd.env(ld_lib_path_envvar, format!("{temp}:{host_rpath_dir}:{ld_lib_path_value}")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably be using std::env::join_paths here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, "{temp}:{host_rpath_dir}:{ld_lib_path_value}"
won't work on Windows where it uses ;
for path separators.
let ld_lib_path_value = env::var(&ld_lib_path_envvar).unwrap(); | ||
|
||
let temp = env::var("TMPDIR").unwrap(); | ||
let host_rpath_dir = env::var("HOST_RPATH_DIR").unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should all probably use var_os, in combination with join_paths that shouldn't be too hard.
let temp = env::var("TMPDIR").unwrap(); | ||
let host_rpath_dir = env::var("HOST_RPATH_DIR").unwrap(); | ||
|
||
cmd.env(ld_lib_path_envvar, format!("{temp}:{host_rpath_dir}:{ld_lib_path_value}")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK, including publicly-writable directories (like /tmp) into PATH-like environment variables can be dangerous. I think for test purposes it's probably not too bad, but do we actually need to do that? Can we avoid putting things into /tmp?
(Perhaps TMPDIR here is set by bootstrap in a way that isn't /tmp?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$TMPDIR
isn't set by run-make (it's set in bootstrap, I need to go back to check where it's being set and what it's being set to), so this seems to just preserve whatever tools.mk was doing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, $TMPDIR
here isn't /tmp
, it's a build output directory
rust/src/tools/compiletest/src/runtest.rs
Lines 3730 to 3734 in 9023f90
let tmpdir = cwd.join(self.output_base_name()); | |
if tmpdir.exists() { | |
self.aggressive_rm_rf(&tmpdir).unwrap(); | |
} | |
create_dir_all(&tmpdir).unwrap(); |
8263707
to
0de85cd
Compare
... was that meant to ping? I rebased to keep up to date with Anyways, did the changes now (just var_os and path changes, also made the The tempdir stuff doesn't seem like it needs any changes, as far as I can see, it's indeed not |
(Yes, I added myself to be mentioned when someone changes run-make/support lib.) |
☔ The latest upstream changes (presumably #122580) made this pull request unmergeable. Please resolve the merge conflicts. |
☔ The latest upstream changes (presumably #122966) made this pull request unmergeable. Please resolve the merge conflicts. |
🔒 Merge conflict This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again. How do I rebase?Assuming
You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial. Please avoid the "Resolve conflicts" button on GitHub. It uses Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Error message
|
1b53730
to
4fd26e4
Compare
rebased - I don't know if you were planning to make significant changes to the test harness (making it more of a pain to merge a "don't set out_dir" for rustdoc, so I went with the less invasive change of just deleting the file as needed in the test. |
#122460 (comment) (the rework PR hasn't merged yet, but in that PR I removed setting --out-dir on Rustdoc, and instead let the other rustdoc test set out-dir in its recipe) |
☔ The latest upstream changes (presumably #122460) made this pull request unmergeable. Please resolve the merge conflicts. |
@rustbot ready |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor nit for Rustdoc
's API, then r=me after CI is green
Thanks for the PR! |
…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#121884 (Port exit-code run-make test to use rust) - rust-lang#122200 (Unconditionally show update nightly hint on ICE) - rust-lang#123568 (Clean up tests/ui by removing `does-nothing.rs`) - rust-lang#123609 (Don't use bytepos offsets when computing semicolon span for removal) - rust-lang#123612 (Set target-abi module flag for RISC-V targets) - rust-lang#123633 (Store all args in the unsupported Command implementation) - rust-lang#123668 (async closure coroutine by move body MirPass refactoring) Failed merges: - rust-lang#123701 (Only assert for child/parent projection compatibility AFTER checking that theyre coming from the same place) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#121884 - 5225225:rmake-exit-code, r=jieyouxu Port exit-code run-make test to use rust As part of rust-lang#121876 ~~As draft because formatting will fail because `x fmt` isn't working for me for some reason, I'll debug that later, just opening this now for review, will mark as ready when formatting is fixed~~ (misleading message from x fmt) cc `@jieyouxu`
As part of #121876
As draft because formatting will fail because(misleading message from x fmt)x fmt
isn't working for me for some reason, I'll debug that later, just opening this now for review, will mark as ready when formatting is fixedcc @jieyouxu