Skip to content

Fix generated wheel naming and platform metadata as -any#24

Merged
bmcfee merged 6 commits intoconda-forge:mainfrom
larsoner:check
Dec 9, 2024
Merged

Fix generated wheel naming and platform metadata as -any#24
bmcfee merged 6 commits intoconda-forge:mainfrom
larsoner:check

Conversation

@larsoner
Copy link
Copy Markdown
Contributor

@larsoner larsoner commented Dec 2, 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.

In conda-forge/yasa-feedstock#11 I get a pip check failure:

soundfile 0.12.1 is not supported on this platform

which suggests to me that something is wrong with the metadata somehow. Adding a pip check here to see if the problem also shows up.

@larsoner
Copy link
Copy Markdown
Contributor Author

larsoner commented Dec 2, 2024

@conda-forge-admin, please rerender

@conda-forge-admin
Copy link
Copy Markdown
Contributor

conda-forge-admin commented Dec 2, 2024

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/meta.yaml) and found it was in an excellent condition.

I do have some suggestions for making it better though...

For recipe/meta.yaml:

  • ℹ️ noarch: python recipes should usually follow the syntax in our documentation for specifying the Python version.

    • For the host section of the recipe, you should usually use python {{ python_min }} for the python entry.
    • For the run section of the recipe, you should usually use python >={{ python_min }} for the python entry.
    • For the test.requires section of the recipe, you should usually use python {{ python_min }} for the python entry.
    • If the package requires a newer Python version than the currently supported minimum version on conda-forge, you can override the python_min variable by adding a Jinja2 set statement at the top of your recipe (or using an equivalent context variable for v1 recipes).
  • ℹ️ PyPI default URL is now pypi.org, and not pypi.io. You may want to update the default source url.

  • ℹ️ In conda-forge.yml: $.compiler_stack = comp7.

    'Compiler Stack' is deprecated.
    Compiler stack environment variable. This is used to specify the compiler
    stack to use for builds. Deprecated.

    compiler_stack: comp7
    Schema
    {
      "anyOf": [
        {
          "type": "string"
        },
        {
          "type": "null"
        }
      ],
      "default": "comp7",
      "deprecated": true,
      "title": "Compiler Stack"
    }
  • ℹ️ In conda-forge.yml: $.max_py_ver = 37.

    'Max Py Ver' is deprecated.
    Maximum Python version. This is used to specify the maximum Python version
    to use for builds. Deprecated.

    max_py_ver: 37
    Schema
    {
      "anyOf": [
        {
          "type": "string"
        },
        {
          "type": "null"
        }
      ],
      "default": "37",
      "deprecated": true,
      "title": "Max Py Ver"
    }
  • ℹ️ In conda-forge.yml: $.max_r_ver = 35.

    'Max R Ver' is deprecated.
    Maximum R version. This is used to specify the maximum R version to use
    for builds. Deprecated.

    max_r_ver: 34
    Schema
    {
      "anyOf": [
        {
          "type": "string"
        },
        {
          "type": "null"
        }
      ],
      "default": "34",
      "deprecated": true,
      "title": "Max R Ver"
    }

This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/12123239925. Examine the logs at this URL for more detail.

@larsoner
Copy link
Copy Markdown
Contributor Author

larsoner commented Dec 2, 2024

Ohhh there is a max_py_ver of 37 so it should be listed as incompatible. I'm going to remove those hoping it's the right thing to do (I assume that isn't the max version anymore???) and see if it helps

@larsoner
Copy link
Copy Markdown
Contributor Author

larsoner commented Dec 2, 2024

@conda-forge-admin, please rerender

@conda-forge-admin
Copy link
Copy Markdown
Contributor

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/meta.yaml) and found it was in an excellent condition.

@larsoner
Copy link
Copy Markdown
Contributor Author

larsoner commented Dec 2, 2024

Okay, successfully replicated the problem:

+ python -c 'import soundfile; print(soundfile._libname)'
$PREFIX/lib/libsndfile.so.1
+ pip check -vvv
soundfile 0.12.1 is not supported on this platform

Looks like there is something fishy with the metadata for this package

@larsoner
Copy link
Copy Markdown
Contributor Author

larsoner commented Dec 2, 2024

@bmcfee any ideas?

@bmcfee
Copy link
Copy Markdown
Contributor

bmcfee commented Dec 2, 2024

Nothing immediately comes to mind as to why this would suddenly start failing. Last time I was in the guts of this I noted some fishiness with the test here: #17 (comment) ; could be worth probing into that a little more?

@larsoner
Copy link
Copy Markdown
Contributor Author

larsoner commented Dec 2, 2024

Nothing immediately comes to mind as to why this would suddenly start failing

Given that pip check wasn't being run before, it's not clear to me when it started failing, or if it has always failed but it's just now being noticed. I only noticed because yasa uses wfdb (who also does not pip check), and wfdb just updated their version to a newer one that requires soundfile.

@larsoner
Copy link
Copy Markdown
Contributor Author

larsoner commented Dec 2, 2024

Okay looking at the build log, it looks like the build command python -m pip install . --no-deps -vv internally now calls a bdist_wheel. And you have set PYSOUNDFILE_PLATFORM=win32 so it should trigger all the tag business from the upstream setup.py so we get a win_amd64 in the filename:

2024-12-02T16:15:41.8583825Z   Created wheel for soundfile: filename=soundfile-0.12.1-py2.py3-none-win_amd64.whl size=24055 sha256=dac71c7e47f8246ae7c8b88f57d8d0506c1e8b1f6628cb74be25a4595eb1d6da

I wouldn't be surprised if this caused pip to say "this is for Windows only" in its metadata.

EDIT: I checked locally the soundfile-0.12.1-py2.py3-none-win_amd64.whl that is produced and indeed it does:

$ cat soundfile-0.12.1.dist-info/WHEEL 
Wheel-Version: 1.0
Generator: bdist_wheel (0.43.0)
Root-Is-Purelib: true
Tag: py2-none-win_amd64
Tag: py3-none-win_amd64

I'll try setting the platform to any which I think should cause it to create a wheel that is named to work on all platforms... 🤞

@larsoner
Copy link
Copy Markdown
Contributor Author

larsoner commented Dec 2, 2024

... this will conflict with the notes about bastibe/python-soundfile#288 but it will at least tell us if it's the problem.

@larsoner
Copy link
Copy Markdown
Contributor Author

larsoner commented Dec 2, 2024

Okay, looks like that's the problem.

Should this package really be noarch given that the wheels are platform-specific?

@bmcfee
Copy link
Copy Markdown
Contributor

bmcfee commented Dec 2, 2024

Should this package really be noarch given that the wheels are platform-specific?

Probably not - I don't know how that ended up being the case originally.

@conda-forge-admin
Copy link
Copy Markdown
Contributor

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

I wanted to let you know that I linted all conda-recipes in your PR (recipe/meta.yaml) and found some lint.

Here's what I've got...

For recipe/meta.yaml:

  • ❌ Non noarch packages should have python requirement without any version constraints.

This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/12166615165. Examine the logs at this URL for more detail.

@conda-forge-admin
Copy link
Copy Markdown
Contributor

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/meta.yaml) and found it was in an excellent condition.

@conda-forge-admin
Copy link
Copy Markdown
Contributor

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

I wanted to let you know that I linted all conda-recipes in your PR (recipe/meta.yaml) and found some lint.

Here's what I've got...

For recipe/meta.yaml:

  • ❌ The build section contained an unexpected subsection name. patches is not a valid subsection name.

This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/12168376014. Examine the logs at this URL for more detail.

@conda-forge-admin
Copy link
Copy Markdown
Contributor

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/meta.yaml) and found it was in an excellent condition.

@larsoner larsoner changed the title Check that pip check succeeds Make package arch-specific Dec 4, 2024
@larsoner larsoner force-pushed the check branch 5 times, most recently from 97cb82f to 08f20f5 Compare December 4, 2024 22:00
@larsoner
Copy link
Copy Markdown
Contributor Author

larsoner commented Dec 4, 2024

Okay, finally green 🚀 (after a few dozen failed attempts 😓)

The trick was to patch out the cmdclass stuff where the wheel name was changed. I tried (for a long time!) to use the PYSOUNDFILE_* env vars instead, and maybe could have gotten it working by patching the manylinux_ in setup.py to be high enough that it wasn't overwritten by pip (at least I think that's what was happening!) when creating the wheel name. But this seemed much more complicated and fragile than just letting pip do what it does by default.

Ready for review/merge from my end:

  1. First commit makes it arch-specific and improves the tests (tests lib access and pip checks), only real changes in five files in recipe/* and the root-level conda-forge.yml.
  2. Second commit is a local rerender and results in the other ~35 files changed.
  3. Third commit fixes the Windows build script to have the env var it needs (I had accidentally removed it), and adds a test that WAV file info can actually be read by the library from disk (might catch the need for the env var but I'm not sure really...)

@bmcfee
Copy link
Copy Markdown
Contributor

bmcfee commented Dec 5, 2024

Wow, thanks for the mega PR on this! It all looks good to me, but I also don't have too much experience with all the build systems involved. It's probably good if @wolfv can also give it a quick look over.

It looks like we have all the same platform coverage as the upstream libsndfile feedstock, so I don't anticipate moving from noarch to cause any dependency chain problems.

@mgeier
Copy link
Copy Markdown
Contributor

mgeier commented Dec 5, 2024

Should this package really be noarch given that the wheels are platform-specific?

Probably not - I don't know how that ended up being the case originally.

The wheels always have been platform-specific, because they contain platform-specific binaries for libsndfile.

However, the conda package didn't contain any binaries, therefore it didn't need to be platform-specific. It just depended on the libsndfile package, which contains the platform-specific binaries.

@larsoner
Copy link
Copy Markdown
Contributor Author

larsoner commented Dec 5, 2024

However, the conda package didn't contain any binaries, therefore it didn't need to be platform-specific. It just depended on the libsndfile package, which contains the platform-specific binaries.

... except that there is some code in there to add a definition for something that is windows-specific when you're on Windows. If you're okay having that in all the wheels (even where it's unusable) I could see if it's possible to keep it noarch (I guess that's what was being done before?). It's a pain to get the wheel naming right but I think it might be doable.

@larsoner larsoner changed the title Make package arch-specific Fix generated wheel naming and platform metadata as -any Dec 5, 2024
@mgeier
Copy link
Copy Markdown
Contributor

mgeier commented Dec 5, 2024

... except that there is some code in there to add a definition for something that is windows-specific when you're on Windows.

You mean this definition?

SNDFILE* sf_wchar_open (const wchar_t *wpath, int mode, SF_INFO *sfinfo) ;

I don't think that hurts on non-Windows systems. You can define whatever function signatures in there. As long as they are not used, they don't have to exist in the actual library.

That's why win32 is selected here:

# We are currently building a noarch package, but pysoundfile contains a
# install-time check for the OS and injects the sf_wchar_open function
# conditionally. If this variable is not set, opening files on Windows doesn't work
# Fix coming hopefully with https://github.com/bastibe/python-soundfile/pull/288
# Original issue on conda-forge: https://github.com/conda-forge/libsndfile-feedstock/issues/14
- export PYSOUNDFILE_PLATFORM=win32

There's even an old unmerged PR that would avoid defining that environment variable: bastibe/python-soundfile#288

@larsoner
Copy link
Copy Markdown
Contributor Author

larsoner commented Dec 5, 2024

Okay changed it so it's now still noarch but the metadata makes it clear that's the case as well. Should be good to go!

@larsoner
Copy link
Copy Markdown
Contributor Author

larsoner commented Dec 9, 2024

@bmcfee this one is a much smaller / less controversial change now, feel free to merge if you're happy

Copy link
Copy Markdown
Contributor

@bmcfee bmcfee left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@bmcfee bmcfee merged commit dc5de32 into conda-forge:main Dec 9, 2024
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.

4 participants