Check exit status only in Build::is_flag_supported
#790
Conversation
This commit/PR enables warnings_into_errors, which causes the compiler to fail if there is any warning unless user override them by specifying env variable `CFLAGS` or `CXXFLAGS`. Thus we no longer need to check stderr for warnings and can now only check exit status of the compiler. This commit also fixed the fn `reset_env` in `tests/test.rs` to use `std::env::remove_var` instead of `std::env::set_env($var, "")`. This commit/PR is done according to the advice from @mqudsi in #784 (comment) Signed-off-by: Jiahao XU <[email protected]>
| cmd.arg(&src) | ||
| .stdin(Stdio::null()) | ||
| .stdout(Stdio::null()) | ||
| .stderr(Stdio::null()); |
There was a problem hiding this comment.
Should we inherit stdout and stderr?
There was a problem hiding this comment.
I believe we should not, since stdout and stderr is for messages from compiler when building the project.
Build::is_flag_supported does not build the project but only test the flags, so I think the stdout and stderr should be piped to null.
There was a problem hiding this comment.
But we don't provide any other mechanism for the developer to check why a test failed, unlike other build systems that write the output of all these checks/tests to a log file.
There was a problem hiding this comment.
But we don't provide any other mechanism for the developer to check why a test failed, unlike other build systems that write the output of all these checks/tests to a log file.
Ehh that's true, I can set that to inherit but I think the stdout/stderr might be a little bit confusing to users.
If that's fine, then I can remove these lines.
There was a problem hiding this comment.
Let’s wait for another opinion? That was just my two cents.
|
I'm not sure about this. I'm pretty sure there's a reason the current code was the way it was... I'll have to go looking. |
|
I had done some git spelunking when I was looking into #784 and found that it was just originally added as "execute and check if stderr is empty" to see if the presence of the flag caused any errors or warnings (you can't just check stderr in that case because the presence of an unrecognized flag/switch would trigger a warning but not an error). Later @NobodyXu added the exit code check in #757 but without any other changes and without a motivating bug (I don't think it changed the behavior in any way since "exit code is 0" is probably a strict subset of "stderr is empty"). If you're not using which is only needed because stderr may sometimes contain non-error output (because no one says otherwise). |
|
I've been rethinking what I said in #784, and I'm not sure using CFLAGS and CXXFLAGS is necessarily the right approach. If a user has an invalid flag in CFLAGS, that means every |
Signed-off-by: Jiahao XU <[email protected]>
|
Hmmm I think it's better to check stderr just to be sure that the flag is indeed supported and there might be cases where the flag is just ignored and compiler exits with success. |
This commit/PR enables warnings_into_errors, which causes the compiler to fail if there is any warning unless user override them by specifying env variable
CFLAGSorCXXFLAGS.Thus we no longer need to check stderr for warnings and can now only check exit status of the compiler.
This commit also fixed the fn
reset_envintests/test.rsto usestd::env::remove_varinstead ofstd::env::set_env($var, "").This commit/PR is done according to the advice from @mqudsi in #784 (comment)
Signed-off-by: Jiahao XU [email protected]