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

Apple: Remove redundant flags #1256

Merged
merged 5 commits into from
Nov 2, 2024
Merged

Conversation

madsmtm
Copy link
Contributor

@madsmtm madsmtm commented Nov 2, 2024

Clang's --target argument is enough to specify the architecture, OS and deployment target, so let's remove the redundant -arch, -m32/-m64 and -m*-version-min flags.

The first commit is a non-functional refactor, the others do the actual removal. Proper GCC support is still broken, since we specify -arch and not -march, will fix that separately.

Fixes #1030.

@madsmtm madsmtm force-pushed the refactor-apple-flags branch 2 times, most recently from 3b8d330 to 75f3ea1 Compare November 2, 2024 02:56
@madsmtm madsmtm marked this pull request as ready for review November 2, 2024 02:56
@NobodyXu
Copy link
Collaborator

NobodyXu commented Nov 2, 2024

cc @BlackHoleFox can you take a look at this PR please?

@madsmtm madsmtm changed the title Refactor Apple architecture flags Apple: Remove redundant flags Nov 2, 2024
@madsmtm
Copy link
Contributor Author

madsmtm commented Nov 2, 2024

I decided to go a different direction with the PR, have updated the title and description to reflect that.

Non-functional change.
The architecture is specified for Clang with the --target option, and
both -arch and -m32/-m64 are passed to GCC elsewhere (should probably be
changed to -march, but that's a different issue).
It is redundant, the version is already specified in `-target`.
Copy link
Collaborator

@NobodyXu NobodyXu left a comment

Choose a reason for hiding this comment

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

Thanks, there's a merge conflict and one review feedback.

src/lib.rs Show resolved Hide resolved
@madsmtm madsmtm requested a review from NobodyXu November 2, 2024 13:53
Copy link
Collaborator

@NobodyXu NobodyXu left a comment

Choose a reason for hiding this comment

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

Thanks LGTM, I will cut a release and hopefully there's no regression.

@NobodyXu NobodyXu merged commit 1a9f345 into rust-lang:main Nov 2, 2024
27 checks passed
@github-actions github-actions bot mentioned this pull request Nov 2, 2024
@BlackHoleFox
Copy link
Contributor

cc @BlackHoleFox can you take a look at this PR please?

Suppose I'm 4 hours late but this looks fine to me 🎉

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.

Don't specify -arch and -m{}os-version-min= on Apple platforms
3 participants