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

noarch-ify #116

Merged
merged 8 commits into from
Jun 6, 2024
Merged

noarch-ify #116

merged 8 commits into from
Jun 6, 2024

Conversation

theAeon
Copy link
Contributor

@theAeon theAeon commented Jun 5, 2024

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

@conda-forge-webservices
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@theAeon theAeon self-assigned this Jun 5, 2024
@theAeon theAeon changed the title Main noarch-ify Jun 5, 2024
@theAeon
Copy link
Contributor Author

theAeon commented Jun 5, 2024

@conda-forge-admin, please rerender

@theAeon theAeon marked this pull request as ready for review June 5, 2024 05:10
@theAeon
Copy link
Contributor Author

theAeon commented Jun 5, 2024

Seems pdm's pulling in the windows build path per #105, getting it over to noarch should fix that. Only thing that this requires is removing the conditional truststore requirement, but pdm doesn't /actually/ depend on it, its just the default. See: pdm-project/pdm#2200.

@theAeon
Copy link
Contributor Author

theAeon commented Jun 6, 2024

fuck it, merging.

@theAeon theAeon merged commit 42652c4 into conda-forge:main Jun 6, 2024
3 checks passed
@daylinmorgan
Copy link
Contributor

I had looked into this but hadn't had time to comment yet. Leaving my original thoughts below anyways.

It seems this issue actually has to do with the previous lack of entrypoint defined in the meta.yml

See this scrapy-feedstock issue for context.

We should probably also add a basic test (pdm --help) for the entrypoint itself to confirm.

While It might be possible to remove truststore for the current version, I would be cautious
about patching pdm or making any changes in the default dependency requirements if we can avoid it.
Both from a maintenance and end-user perspective.
Indeed the PR you linked suggests the opposite to me, and that it should be installed alongside pdm.

Is removing the python version constraints on individual packages
(tomli, importlib-metadata and importlib-resources) also necessary for noarch: python?

@theAeon
Copy link
Contributor Author

theAeon commented Jun 6, 2024

A few things-

All of the version specifiers do need to go for noarch, but my understanding is that for things that became built-ins conda-forge just builds them as empty for the relevant versions.

I agree that we shouldn't be patching PDM itself, but as PDM supports a lower version of python than truststore, it should always work (barring version tests in the code itself) without it being present. It should still work if truststore is installed by the end user.

Do you know of any noarch packages with a similar pseudo-dependency?

@daylinmorgan
Copy link
Contributor

Do you know of any noarch packages with a similar pseudo-dependency?

Not in conda-forge, I think I've seen in in biconda recipes but those have different requirements.

Was it absolutely necessary to make the recipe noarch to fix #105 or would adding the entrypoint in the build metadata have been enough?

but my understanding is that for things that became built-ins conda-forge just builds them as empty for the relevant versions.

Are you saying the packages themselves would be empty? They are imported from a different namespace entirely.

❯ micromamba list -p ./env 'pdm|^python$|importlib'
List of packages in environment: "/home/daylin/tmp/pdm-test/env"

  Name                 Version  Build               Channel
─────────────────────────────────────────────────────────────────
  importlib-metadata   7.1.0    pyha770c72_0        conda-forge
  importlib-resources  6.4.0    pyhd8ed1ab_0        conda-forge
  importlib_resources  6.4.0    pyhd8ed1ab_0        conda-forge
  pdm                  2.15.4   pyhd8ed1ab_1        conda-forge
  python               3.12.3   hab00c5b_0_cpython  conda-forge
❯ ls ./env/lib/python3.12/site-packages/importlib*
./env/lib/python3.12/site-packages/importlib_metadata:
_adapters.py  _collections.py  compat  _compat.py  diagnose.py  _functools.py  __init__.py  _itertools.py  _meta.py  py.typed  _text.py

./env/lib/python3.12/site-packages/importlib_metadata-7.1.0.dist-info:
direct_url.json  INSTALLER  LICENSE  METADATA  RECORD  REQUESTED  top_level.txt  WHEEL

./env/lib/python3.12/site-packages/importlib_resources:
abc.py  _adapters.py  _common.py  compat  functional.py  future  __init__.py  _itertools.py  py.typed  readers.py  simple.py  tests

./env/lib/python3.12/site-packages/importlib_resources-6.4.0.dist-info:
direct_url.json  INSTALLER  LICENSE  METADATA  RECORD  REQUESTED  top_level.txt  WHEEL

@theAeon
Copy link
Contributor Author

theAeon commented Jun 6, 2024

It shouldn't be necessary to fix #105 but I'm fairly sure that noarch is far preferable for anything that can possibly use it on ci resources alone.

Re: everything else let me poke at the docs rq

@theAeon
Copy link
Contributor Author

theAeon commented Jun 6, 2024

Therefore, the conda-forge community maintains a list of packages that are safe to be installed under all Python versions, even if the original package only requires it for some versions.

For example, the package [pyquil](https://github.com/rigetti/pyquil) only [requires](https://github.com/rigetti/pyquil/blob/497791e8108d8780109d75410be786c5f6e590ea/pyproject.toml#L30) importlib-metadata for python <3.8 but it is actually safe to be installed under python >=3.8 as well.

Currently available packages:

    exceptiongroup
    importlib-metadata

Well, that answers that for importlib_metadata. Curious why tomli/importlib_resources aren't mentioned.

@theAeon
Copy link
Contributor Author

theAeon commented Jun 6, 2024

Tomli is literally a duplicate of tomllib in the standard library of 3.11+, so all that happens there is that we download extra copies of a python module that's already installed, which is probably fine. Better than waiting for 12x ci, at least.

I can't find anything specifically mentioning importlib-resources, but the readme is structured the same as imporlib-metadata (pure backports) so I imagine its fine?

Still trying to figure out what to do with truststore.

@daylinmorgan
Copy link
Contributor

so all that happens there is that we download extra copies of a python module that's already installed, which is probably fine.

I disagree that we should decide for users which modules in addition to pdm should be downloaded and installed (even if it doesn't affect pdm's operation) when it deviates from the actual maintainers dependency requirements/spec.

And this workaround to achieve noarch now means that we are not installing a package the maintainers expect to be installed for python versions > 3.9.

@theAeon
Copy link
Contributor Author

theAeon commented Jun 7, 2024

These are two distinct issues.

Going purely by the documentation, the extra copies thing is acceptable-take it up with core, I guess.

I do agree that the not-having-truststore-on-python-3.10+ thing is an issue though.

@daylinmorgan
Copy link
Contributor

Going purely by the documentation, the extra copies thing is acceptable-take it up with core, I guess.

I don't have a problem with conda or any other conda-forge-compatible tool installing these modules as they exist independent of the standard library. If a users says install importlib_metadata then they should get that package sourced appropriately from pypi and versioned separately from the standard library.

My issue is with us specifying that everyone should have importlib_metadata (and not truststore) when that is not the behavior produced when installed via pip or any other PEP517 compatible package manager.

@theAeon
Copy link
Contributor Author

theAeon commented Jun 11, 2024

@conda-forge/help-python any thoughts on the best practice here?

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.

2 participants