Warn if dsymutil returns an error code
#79508
Conversation
This was useful during testing of `dsymutil` code paths.
|
r? @lcnr (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
From a technical perspective this seems fine (although it'd be nice to unify the error reporting with the linker error reporting to ensure everything gets printed the same way). I am not comfortable approving this though since it has the possibility of being a regression for many projects. I don't know if there are any or how many projects are actually failing dsymutil but are succeeding to compile today. If this were to land there wouldn't be any stable recourse for those projects to get compiling again. Basically I suspect there will need to be some analysis and/or investigation done to see what the impact of a change like this may be. I don't know what it would be myself, unfortunately. @lcnr would you be ok finding a different reviewer for that aspect? |
|
I don't know who else is at least somewhat knowledgeable about Maybe r? @nagisa
In general, if I reassign an issue it's mostly a guess on who might be the most fitting for the given changes. |
|
My position is similar to that of @alexcrichton's. We lack information on how many silent failures we have today. Ordinarily we'd run crater here, but crater builds on Linux, not macOS... The only way I can see this landing is if the dsymutil failure printed a warning and did not terminate the rest of compilation. |
This checks the error code returned by `dsymutil` and warns if it failed. It also provides the stdout and stderr logs from `dsymutil`, similar to the native linker step. Fixes rust-lang#78770
jryans
force-pushed
the
check-dsymutil-result
branch
from
e77a5b9
to
4ed9083
Compare
jryans
changed the title
dsymutil returns an error codedsymutil returns an error code
|
Thanks everyone for your feedback! 😄 I agree it seems hard to gather evidence of how many silent failures are happening here today, so failing compilation does seem risky. I have changed the message to a warning and allowed compilation to continue, which I believe is still useful so that at least the failure is presented instead of hidden. @nagisa, this should be ready for another look. |
|
@bors r+ |
|
📌 Commit 4ed9083 has been approved by |
bors
added
S-waiting-on-bors
Warn if `dsymutil` returns an error code This checks the error code returned by `dsymutil` and warns if it failed. It also provides the stdout and stderr logs from `dsymutil`, similar to the native linker step. I tried to think of ways to test this change, but so far I haven't found a good way, as you'd likely need to inject some nonsensical args into `dsymutil` to induce failure, which feels too artificial to me. Also, rust-lang#79361 suggests Rust is on the verge of disabling `dsymutil` by default, so perhaps it's okay for this change to be untested. In any case, I'm happy to add a test if someone sees a good approach. Fixes rust-lang#78770
…laumeGomez Rollup of 3 pull requests Successful merges: - rust-lang#79508 (Warn if `dsymutil` returns an error code) - rust-lang#79509 (Improve attribute message error spans) - rust-lang#79600 (std::io: Use sendfile for UnixStream) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
This checks the error code returned by
dsymutiland warns if it failed. Italso provides the stdout and stderr logs from
dsymutil, similar to the nativelinker step.
I tried to think of ways to test this change, but so far I haven't found a good way, as you'd likely need to inject some nonsensical args into
dsymutilto induce failure, which feels too artificial to me. Also, #79361 suggests Rust is on the verge of disablingdsymutilby default, so perhaps it's okay for this change to be untested. In any case, I'm happy to add a test if someone sees a good approach.Fixes #78770