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

Drop macosx_10_0 from compatible wheel tags on aarch64 #2496

Merged
merged 4 commits into from
Mar 18, 2024

Conversation

zanieb
Copy link
Member

@zanieb zanieb commented Mar 17, 2024

Following #2489 this is the last remaining difference from Python 3.12's packaging module.

@zanieb zanieb added the bug Something isn't working label Mar 17, 2024
@@ -444,7 +444,7 @@ fn compatible_tags(platform: &Platform) -> Result<Vec<String>, PlatformError> {
let mut platform_tags = vec![];
// Starting with Mac OS 11, each yearly release bumps the major version number.
// The minor versions are now the midyear updates.
for major in (10..=*major).rev() {
for major in (11..=*major).rev() {
Copy link
Member

Choose a reason for hiding this comment

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

I wonder why this was 10 to start…?

Copy link
Member Author

Choose a reason for hiding this comment

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

We do include 10_4 through 10_16 so it seems easy to expect 10_0 should be included?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah this is handled in

fn get_mac_binary_formats(major: u16, minor: u16, arch: Arch) -> Vec<String> {
let mut formats = vec![match arch {
Arch::Aarch64 => "arm64".to_string(),
_ => arch.to_string(),
}];
if matches!(arch, Arch::X86_64) {
if (major, minor) < (10, 4) {
return vec![];
}
formats.extend([
"intel".to_string(),
"fat64".to_string(),
"fat32".to_string(),
]);
}
if matches!(arch, Arch::X86_64 | Arch::Aarch64) {
formats.push("universal2".to_string());
}
if matches!(arch, Arch::X86_64) {
formats.push("universal".to_string());
}
formats
}
#[cfg(test)]
for the x86_64 variant and it's just broken on aarch64

Copy link
Member Author

Choose a reason for hiding this comment

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

See 0038f18 and 73b561b

@zanieb zanieb changed the title Drop macosx_10_0 from compatible wheel tags Drop macosx_10_0 from compatible wheel tags on aarch64 Mar 17, 2024
Comment on lines +754 to +761
let tags = compatible_tags(&Platform::new(
Os::Macos {
major: 14,
minor: 0,
},
Arch::X86_64,
))
.unwrap();
Copy link
Member Author

Choose a reason for hiding this comment

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

I added some new coverage for x86_64 because it has a separate branch of logic but these were correct without changes.

Base automatically changed from zb/abi-tags-fix to main March 18, 2024 14:21
This is a no-op because `get_mac_binary_formats` handles exclusion of these versions
but in the next commit we will remove handling from there in favor of this
This is a no-op because ce25d68cbb0cb9465e7aecf135d416122d87ad3a added handling in `compatible_tags` directly
@zanieb zanieb force-pushed the zb/macos-platform-tags-fix branch from 73b561b to eea6a52 Compare March 18, 2024 14:44
@zanieb zanieb requested a review from konstin March 18, 2024 14:44
Copy link
Member

@konstin konstin left a comment

Choose a reason for hiding this comment

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

Code looks good but i can't review on mac implementation details

@zanieb zanieb enabled auto-merge (squash) March 18, 2024 14:51
@zanieb zanieb merged commit 01cef87 into main Mar 18, 2024
30 checks passed
@zanieb zanieb deleted the zb/macos-platform-tags-fix branch March 18, 2024 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants