-
Notifications
You must be signed in to change notification settings - Fork 75
modifed y.sh to allow for running cargo tests. #669
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
base: master
Are you sure you want to change the base?
Conversation
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.
Good job!
Could you please update the doc and the CI to use this new command instead of cargo test
?
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.
I like how you add a lot of comments.
Thanks!
build_system/src/test.rs
Outdated
// That would force `cg_gcc` to *rebuild itself* and only then run tests, which is undesirable. | ||
let mut env = test_env.clone(); | ||
env.remove("RUSTFLAGS"); | ||
// FIXME: maybe we could allow for only a select test to be run? |
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.
Yes, that would be nice to forward arguments to cargo test
which would allow to filter the tests.
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.
Args can now be forwarded by using the --
symbol.
Eg.
./y.sh test --cargo-tests --use-system-gcc -- all flags after this get passed to cargo test
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.
Nice.
I believe only this is missing for this PR:
Could you please update the doc and the CI to use this new command instead of cargo test?
@@ -203,6 +206,21 @@ fn clean(_env: &Env, args: &TestArg) -> Result<(), String> { | |||
create_dir(&path) | |||
} | |||
|
|||
fn cargo_tests(test_env: &Env, test_args: &TestArg) -> Result<(), String> { |
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.
The following is not necessary before merging this PR, but would be a nice to have:
currently, this seems to always recompile rustc_codegen_gcc
and the tests/lang_tests_*.rs
stuff, even when there are no changes.
Do you know what causes this?
Would that be easy to fix?
Maybe that's because we have some environment variables set that affect the build, which makes it different than the normal build? (It could be worth just extract LD_LIBRARY_PATH
and LIBRARY_PATH
from test_env
and only sending those to run_command_with_output_and_env
to see if this is the case and fixes this issue).
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.
Sadly, that is unlikely to fix this, at least from my (rather amateur) analysis of the cargo logs.
(One of) the rebuild reasons is this:
fingerprint dirty for rustc_codegen_gcc v0.1.0
cargo::core::compiler::fingerprint: dirty: ProfileConfigurationChanged
Now, if it was a flag issue, I'd expect this to be the recompilation reason:
if self.rustflags != old.rustflags {
return DirtyReason::RustflagsChanged {
old: old.rustflags.clone(),
new: self.rustflags.clone(),
};
}
I guess I will have to ask about what ProfileConfigurationChanged
means - it is compared as a 64 bit hash in the cargo code, I will have to see what it is a hash of.
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.
BTW, all the other rebuild reasons in the logs only tell me that the tests are getting rebuilt, because cg_gcc
got rebuilt.
target="lang_tests_common"}: cargo::core::compiler::fingerprint: dirty: FsStatusOutdated(StaleDependency { name: "rustc_codegen_gcc",
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.
I tested my idea and it does seem to help.
With it, running --cargo-tests
a second time gives:
[BUILD] build system
Finished `release` profile [optimized] target(s) in 0.01s
GCC path retrieved from `config.toml`. Using `/opt/gcc/lib` as path for libgccjit
Finished `dev` profile [optimized + debuginfo] target(s) in 0.01s
[…]
Finished `test` profile [optimized + debuginfo] target(s) in 0.03s
Running unittests src/lib.rs (target/debug/deps/rustc_codegen_gcc-e4ba054637ab4819)
while without it:
[BUILD] build system
Finished `release` profile [optimized] target(s) in 0.03s
GCC path retrieved from `config.toml`. Using `/opt/gcc/lib` as path for libgccjit
Compiling rustc_codegen_gcc v0.1.0 (/home/bouanto/Ordinateur/Programmation/Rust/Projets/rustc_codegen_gcc)
Finished `dev` profile [optimized + debuginfo] target(s) in 1.37s
[…]
Compiling rustc_codegen_gcc v0.1.0 (/home/bouanto/Ordinateur/Programmation/Rust/Projets/rustc_codegen_gcc)
Finished `test` profile [optimized + debuginfo] target(s) in 8.51s
Running unittests src/lib.rs (target/debug/deps/rustc_codegen_gcc-4d02d698ebbfa187)
Added a new option to
./y.sh test
:./y.sh test --cargo-tests
.When this option is selected,
y.sh
will build & test minicore(it is a dependency). After that, it will simply callcargo test
with the selected environment flags.For now
./y.sh test --cargo-tests
ignoresRUSTFLAGS
.Normally,
RUSTFLAGS
contain the-Zcodegen-backend
option, which would makecg_gcc
rebuild itself. This is undesirable, and ignoringRUSTFLAGS
altogether was simply the easiest option.In the future, we maybe could filter out
-Zcodegen-backend
, and keep the rest of the flags intact.