Skip to content
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

Fix msvc stdout not shown on error #1303

Merged
merged 8 commits into from
Nov 26, 2024
Merged

Fix msvc stdout not shown on error #1303

merged 8 commits into from
Nov 26, 2024

Conversation

NobodyXu
Copy link
Collaborator

Fixed #1260

Copy link
Collaborator

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do this in create_compile_object_cmd (and not more generally in try_get_compiler)?

@NobodyXu
Copy link
Collaborator Author

NobodyXu commented Nov 24, 2024

Hmmm I didn't think of this at all!

Looking at it closer, I think there are two reasons not to do that:

  • try_get_compiler is a public function, changing it might break someone else's workflow, i.e. what if user actually expects non-English output?
    try_get_compiler only adds args not env, while create_compile_object_cmds does add env (for apple only though). I guess this also has to do with try_get_compiler being public interface?
  • Upon further checking try_get_compiler is used in try_expand and is_flag_supported_inner, which checks for exit code and stderr output of the compiler https://docs.rs/cc/latest/src/cc/lib.rs.html#713 , add env that changes stdout/stderr might break it

@madsmtm
Copy link
Collaborator

madsmtm commented Nov 24, 2024

what if user actually expects non-English output

If we consider changing the shown language a breaking change, then changing it in create_compile_object_cmd is also a breaking change, no?

try_get_compiler only adds args not env, while create_compile_object_cmds does add env (for apple only though)

I doubt there's a thought-out reason for that? In any case, I'm pretty sure that workaround can be removed nowadays (the problem was using the general *-apple-darwin target instead of the specific *-apple-macosx target).

Upon further checking try_get_compiler is used in try_expand and is_flag_supported_inner, which checks for exit code and stderr output of the compiler https://docs.rs/cc/latest/src/cc/lib.rs.html#713 , add env that changes stdout/stderr might break it

Those all seem positive to force being English (provided we decide to force English at all)?

src/lib.rs Outdated
@@ -1784,6 +1784,9 @@ impl Build {
if cfg!(target_os = "macos") {
self.fix_env_for_apple_os(&mut cmd)?;
}
if msvc {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why only when linking for MSVC? Seems like we'd want to do this everywhere, if we wanted to do it in the first place?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks updated.

@NobodyXu
Copy link
Collaborator Author

NobodyXu commented Nov 24, 2024

If we consider changing the shown language a breaking change, then changing it in create_compile_object_cmd is also a breaking change, no?

yes that's true.

Those all seem positive to force being English (provided we decide to force English at all)?

Yeah, thanks, I've changed it to be set in try_get_compiler

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@NobodyXu NobodyXu merged commit f770d56 into main Nov 26, 2024
54 checks passed
@NobodyXu NobodyXu deleted the NobodyXu-patch-1-1 branch November 26, 2024 12:58
@github-actions github-actions bot mentioned this pull request Nov 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

How to get a more friendly error message in cc?
2 participants