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

Add architecture-awareness to platform #12367

Merged

Conversation

chrisjrn
Copy link
Contributor

@chrisjrn chrisjrn commented Jul 16, 2021

@chrisjrn chrisjrn marked this pull request as draft July 16, 2021 19:57
@chrisjrn
Copy link
Contributor Author

I think the most consequential decision I've made was to add architecture helpers to osutil.py. That file should either be renamed, or we should move the arch formats out into another place.

Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Nice! This looks great, and I agree with using osutil.py for arch helpers.

src/python/pants/util/osutil.py Outdated Show resolved Hide resolved
Comment on lines 75 to 78
MacOsX86_64,
MacOsArm64,
LinuxX86_64,
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this would be less awkward? Technically you're supposed to do UpperCamelCase for enum variants, but we can turn off Clippy (Rust linter) for this one.

  Macos_x86_64,
  Macos_arm64,
  Linux_x86_64,

Copy link
Contributor

Choose a reason for hiding this comment

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

@stuhood thoughts?

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Yea, allowing snake case here probably makes sense. Can skip the lint with:

#[allow(non_snake_case)]
enum .. {
  ..
}

@chrisjrn
Copy link
Contributor Author

This is done, pending tests, unfortunately I cannot run tests because pex-tool/pex#1384

src/python/pants/engine/platform.py Outdated Show resolved Hide resolved
src/python/pants/engine/platform.py Outdated Show resolved Hide resolved
src/python/pants/util/osutil.py Outdated Show resolved Hide resolved
@chrisjrn chrisjrn force-pushed the chrisjrn/10620-armier-platforms branch from 52b0b58 to 7d79c13 Compare July 20, 2021 22:33
@chrisjrn chrisjrn marked this pull request as ready for review July 20, 2021 22:33
@chrisjrn
Copy link
Contributor Author

marking as ready to get CI's opinion

Christopher Neugebauer added 2 commits July 20, 2021 15:34
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@chrisjrn chrisjrn marked this pull request as draft July 20, 2021 22:35
Christopher Neugebauer added 6 commits July 20, 2021 15:35
[ci skip-rust]

[ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
[ci skip-rust]

[ci skip-build-wheels]
[ci skip-rust]

[ci skip-build-wheels]
@chrisjrn chrisjrn force-pushed the chrisjrn/10620-armier-platforms branch from 7d79c13 to dccac56 Compare July 20, 2021 22:39
Christopher Neugebauer added 3 commits July 21, 2021 08:12
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@chrisjrn chrisjrn marked this pull request as ready for review July 21, 2021 18:37
@chrisjrn
Copy link
Contributor Author

@Eric-Arellano Tests pass at this point, there are still remaining issues around running x86 and ARM side by side, but they're not coming from the Platform side of thigns.

Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Nicely done 🙌

@Eric-Arellano Eric-Arellano changed the title [WIP] Add architecture-awareness to platform Add architecture-awareness to platform Jul 21, 2021
@Eric-Arellano Eric-Arellano requested a review from tdyas July 21, 2021 19:39
Copy link
Sponsor Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

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

Looks good, thanks.

It would be good to fix the example in the pydoc of ExternalTool to explain how to set the default_known_versions: https://github.com/pantsbuild/pants/pull/12367/files#diff-0591b1668767ffb0bf44c7197bfe0d73e1d4577327fdb1a2ee47c15a1ef463a1R57 ... and maybe also to reference how the dict lookups into the known_versions are computed?

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@chrisjrn
Copy link
Contributor Author

@stuhood This commit should address most of your concerns. I am not sure what you mean about "referencing how the dict lookups … are computed" -- there is already complete documentation for --url-platform-mapping, but if that's not what you mean, I'm happy to add it.

@stuhood
Copy link
Sponsor Member

stuhood commented Jul 21, 2021

Should be fine. Can follow up later if its not clear based on the existing examples.

Christopher Neugebauer added 3 commits July 21, 2021 15:45
# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@Eric-Arellano Eric-Arellano merged commit 9cd4a3a into pantsbuild:main Jul 22, 2021
@chrisjrn chrisjrn deleted the chrisjrn/10620-armier-platforms branch July 22, 2021 16:51
@Eric-Arellano
Copy link
Contributor

Great work @chrisjrn! Exciting :)

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.

Teach engine.platform about CPU architectures
3 participants