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

Document OS-specific approaches for noarch packages #1839

Merged
merged 13 commits into from
Jun 14, 2023

Conversation

jaimergp
Copy link
Member

@jaimergp jaimergp commented Oct 21, 2022

PR Checklist:

  • note any issues closed by this PR with closing keywords
  • put any other relevant information below

This creative approach is used by several feedstocks (10 or so, according to this Github CodeSearch query) now but wasn't documented. This approach alleviates the number of builds needed for platform-specific dependencies in noarch-eligible packages.

@jaimergp jaimergp requested a review from a team as a code owner October 21, 2022 12:44
It is possible to build ``noarch`` packages with runtime requirements that depend on the target OS (Linux, Windows,
MacOS), regardless the architecture (amd64, ARM, PowerPC, etc). This approach relies on four concepts:

1. Virtual packages. Prefixed with a double underscore, they are used by conda to represent properties of the running system
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
1. Virtual packages. Prefixed with a double underscore, they are used by conda to represent properties of the running system
1. Virtual packages. Prefixed with a double underscore, they are used by conda
when running the dependency resolver to represent properties of the currently
activated environment

Copy link
Member Author

Choose a reason for hiding this comment

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

Same as below, technically they are environment independent unless overridden.


1. Virtual packages. Prefixed with a double underscore, they are used by conda to represent properties of the running system
as constraints for the solver. We will use ``__linux``, ``__win`` or ``__osx``, which are only present when
the running platform is Linux, Windows, or MacOS, respectively. ``__unix`` is present in both Linux and MacOS. Note
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
the running platform is Linux, Windows, or MacOS, respectively. ``__unix`` is present in both Linux and MacOS. Note
the activated environment is Linux, Windows, or MacOS, respectively. ``__unix`` is present in both Linux and MacOS. Note

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 think one can only change the virtual packages with CONDA_OVERRIDE_* env variables. Otherwise, regardless the active environment, it will still report the native platform type.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, thanks for the clarification. I don't know if "running platform" is defined somewhere?

Copy link
Contributor

Choose a reason for hiding this comment

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

also, it is not clear to me from "as constraints for the solver" that this happens at runtime, and not while building the package.

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'll rephrase, thanks!

@jakirkham
Copy link
Member

Thanks for adding this, Jaime! 🙏

cc @minrk (in case you have thoughts on this 🙂)

Comment on lines 1099 to 1102
- __{{ target_os }}
{% if target_os == 'win' %}
- windows-only-dependency
{% endif %}
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why we need target_os.
We can just do

Suggested change
- __{{ target_os }}
{% if target_os == 'win' %}
- windows-only-dependency
{% endif %}
{% if win %}
- windows-only-dependency
- __win
{% else %}
- __unix
{% endif %}

Copy link
Member Author

Choose a reason for hiding this comment

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

Mmm possibly, I need to double check but indeed we might not need it. Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok my main concern about not using conda_build_config.yaml, @isuruf:

  • The package hashes will be the same (see Noarch click-feedstock#44), so the build string NEEDS to be prefixed with the platform value. If the user forgets about this, it will result in a filename collision. This should be checked by the linter, at the very least. I am unaware if sharing a hash is problematic for other cases.

Copy link
Member

Choose a reason for hiding this comment

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

I share this concern and would prefer not using string. Explained in more detail below ( #1839 (comment) )

Copy link
Member

Choose a reason for hiding this comment

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

Comment on lines 1088 to 1089
string: "unix_pyh{{ PKG_HASH }}_{{ PKG_BUILDNUM }}" # [unix]
string: "win_pyh{{ PKG_HASH }}_{{ PKG_BUILDNUM }}" # [win]
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to avoid manipulating the string and instead rely on different hashes (note Jaime is mentioning something similar elsewhere)?

The main issue with string is pretty finicky. Users need to be very careful to replicate conda-build's behavior and if they missing something they could wind up with duplicate packages (which sometimes can both be accepted by Anaconda.org with neither being downloadable).

Copy link
Member Author

Choose a reason for hiding this comment

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

This will be needed until conda/conda-build#4606 lands and a new conda-build release is cut.

Is it ok to merge as is or would you rather have a little admonition mentioning this caveat?

Copy link
Member

Choose a reason for hiding this comment

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

My understanding from the thread above is using conda_build_config.yaml meant we would not need this.

Fixing conda-build to not need either is certainly better. That said, between setting a string and using conda_build_config.yaml, would prefer the latter (fewer foot guns).

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 am partial to conda_build_config.yaml too as we transition to the new conda-build version.

@jaimergp
Copy link
Member Author

jaimergp commented Nov 8, 2022

After a conversation with @viniciusdc, we realized a simplified version of this approach can also be used to skip OS builds. A common use case that requires non-noarch is to skip Windows, so we can just add a __unix dependency to achieve the same result under a noarch scheme.

@BastianZim
Copy link
Member

Is everyone happy with merging this? Would be good to have it in the docs.

@jaimergp
Copy link
Member Author

jaimergp commented Dec 16, 2022

Is everyone happy with merging this? Would be good to have it in the docs.

We need to agree on the "best" solution to avoid the hash collision. See discussion here.

jaimergp and others added 2 commits December 20, 2022 16:04
Copy link
Contributor

@hmaarrfk hmaarrfk left a comment

Choose a reason for hiding this comment

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

thanks using this now.

src/maintainer/knowledge_base.rst Outdated Show resolved Hide resolved
isuruf and others added 2 commits May 15, 2023 15:32
Co-authored-by: Mark Harfouche <mark.harfouche@gmail.com>
@jaimergp
Copy link
Member Author

The conda-build PR we were waiting for was merged recently and should be available in the upcoming conda-build 3.25, which means we will be able to merge this longstanding PR once and for all!

@asmeurer
Copy link
Member

The linter will need to be updated to recognize this approach. See for instance conda-forge/sympy-feedstock#52.

@jaimergp
Copy link
Member Author

@asmeurer, it was updated a while ago (see conda-forge/conda-smithy#1682), but only for simple platform selectors (e.g. # [win]) not custom ones like in your example (# [with_gmpy]).

@mikemhenry
Copy link
Contributor

Just a friendly bump, I was looking for this recently and it would be great for the documentation to be easier to find, any blockers left?

@jaimergp
Copy link
Member Author

This is ready to go from my side. @conda-forge/core if someone has the time to click merge :) 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.