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

detect clang/gcc using --version #709

Merged
merged 4 commits into from
Sep 23, 2023
Merged

Conversation

youknowone
Copy link
Contributor

@youknowone youknowone commented Jul 31, 2022

This is useful on macos because gcc and cc is actually clang.
gcc and cc were regarded as clang even when it was gcc.

m32 and m64 is not necessary for clang, but also harmless.
Do you want to do it as now or want to revert it and change tests run only for gcc?

@youknowone
Copy link
Contributor Author

@thomcc This patch is broken after rebase. Could you tell me if it is worth to fix it or not?

Copy link
Member

@thomcc thomcc left a comment

Choose a reason for hiding this comment

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

Yeah, I think this is fine. Sorry for the delay. Can you fix the failures?

src/lib.rs Outdated Show resolved Hide resolved
@@ -1663,6 +1663,16 @@ impl Build {
family.add_force_frame_pointer(cmd);
}

if !cmd.is_like_msvc() {
Copy link
Member

Choose a reason for hiding this comment

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

This change seems unrelated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't remember the details anymore, but they are related to tests gnu_i686 and gnu_x86_64

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I got the idea. This happened because gcc() now target x86_64-apple-darwin on macOS.
I can revert this part and add target to gnu_i686 and gnu_x86_64. Is this better?

Copy link
Contributor

Choose a reason for hiding this comment

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

By itself it's a formally wrong move. While the -m32/m64 are accepted by clang, the --target argument [which is always passed to clang by cc-rs] describes the intention sufficiently and more importantly more adequately. In other words the options in question should have remained in the ToolFamily::Gnu section. If they cause problems in the section in question, then it would have been more appropriate to solve it there.

On a tangential note. The powepc64 gcc defaults to -m64, so that it's actually more interesting to pass -m32 than -m64. But of course making it symmetric would be better, i.e. pass -m32 if the target is 32-bit ppc, and -m64 otherwise...

@youknowone
Copy link
Contributor Author

@thomcc Tests are fixed. Could you review it again?

@youknowone
Copy link
Contributor Author

Maybe replacement of #407

Copy link
Member

@thomcc thomcc left a comment

Choose a reason for hiding this comment

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

Seems good to me.

@thomcc thomcc merged commit d765f30 into rust-lang:main Sep 23, 2023
16 checks passed
@thomcc
Copy link
Member

thomcc commented Sep 23, 2023

It looks like this broke CI, hmmm...

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.

3 participants