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

Promote to_apple_arch in conan.tools.apple #11915

Merged
merged 2 commits into from
Aug 24, 2022

Conversation

jcar87
Copy link
Contributor

@jcar87 jcar87 commented Aug 19, 2022

Changelog: Feature: Promote to_apple_arch in the new conan.tools.apple module.
Docs: conan-io/docs#2722

Closes #11865

Notes:

  • Where possible, have replaced uses of conan.tools.apple.apple.to_apple_arch to the new one taking a conanfile as an argument
  • Elsewhere with this wasn't possible (because there's no conanfile object in the context), have made those call the private implementation.
  • Have added a barebones test to cover this

@jcar87 jcar87 requested a review from lasote August 19, 2022 09:23
conan/tools/apple/__init__.py Outdated Show resolved Hide resolved
Co-authored-by: Luis Martinez <lasote@gmail.com>
@jcar87 jcar87 marked this pull request as ready for review August 19, 2022 09:27
'armv8.3': 'arm64e',
'armv7s': 'armv7s',
'armv7k': 'armv7k'}.get(str(arch))
arch_ = conanfile.settings.get_safe("arch")
Copy link
Member

Choose a reason for hiding this comment

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

I am slightly concerned about some recent tools similar to this, as I am wondering what will happen if at some point users want to use it for the build context. They will not be able. So the options would be:

  • To expose the underlying dict, something like apple_archs(arch)
  • To create a new to_apple_arch_build(conanfile) (a bit ugly?)

I am fine with this change at this moment, but something to have in mind and maybe discuss.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point! If there's a risk that we may end up with too many small tool functions that have a similar issue, I'd be more inclined to try and sort this out sooner rather than later. to_apple_arch has very few uses in Conan Center at the moment, so we may have an opportunity to get it right to cover all cases (although we also want to avoid a repeat of the situations where recipes are using it from the "private" module apple.apple)

Copy link
Contributor

Choose a reason for hiding this comment

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

there are some other alternatives, like:

  • change context in scope via RAII, e.g. with context_switch(ctx="build"): do something
  • provide a nice moniker like if is_apple_arch(conanfile.build) where conanfile.build is ConanFIle instance with context switched to build

Copy link
Member

Choose a reason for hiding this comment

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

Related #11926

@memsharded memsharded added this to the 1.52 milestone Aug 23, 2022
Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

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

We will move forward with this approach, and add a context_build=False argument in the future if need to apply to the build context.

@memsharded memsharded merged commit 9c4487e into conan-io:develop Aug 24, 2022
@jcar87 jcar87 deleted the feature/conan-tools-to-apple-arch branch August 24, 2022 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[feature] Expose is_apple_os & to_apple_arch in conan.tools.apple
4 participants