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 recipe for python-soxr #19495

Merged
merged 3 commits into from
Aug 18, 2022
Merged

Add recipe for python-soxr #19495

merged 3 commits into from
Aug 18, 2022

Conversation

dofuuz
Copy link
Contributor

@dofuuz dofuuz commented Jun 30, 2022

Checklist

  • Title of this PR is meaningful: e.g. "Adding my_nifty_package", not "updated meta.yaml".
  • License file is packaged (see here for an example).
  • Source is from official source.
  • Package does not vendor other packages. (If a package uses the source of another package, they should be separate packages or the licenses of all packages need to be packaged).
  • If static libraries are linked in, the license of the static library is packaged.
  • Build number is 0.
  • A tarball (url) rather than a repo (e.g. git_url) is used in your recipe (see here for more details).
  • GitHub users listed in the maintainer section have posted a comment confirming they are willing to be listed there.
  • When in trouble, please check our knowledge base documentation before pinging a team.

@conda-forge-linter
Copy link

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 (recipes/python-soxr) and found some lint.

Here's what I've got...

For recipes/python-soxr:

  • The top level meta keys are in an unexpected order. Expecting ['package', 'source', 'build', 'requirements', 'test', 'about', 'extra'].
  • Feedstock with the same name exists in conda-forge.

For recipes/python-soxr:

  • License is not an SPDX identifier (or a custom LicenseRef) nor an SPDX license expression.

Documentation on acceptable licenses can be found here.

@conda-forge-linter
Copy link

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 (recipes/python-soxr) and found some lint.

Here's what I've got...

For recipes/python-soxr:

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

@conda-forge-linter
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 (recipes/python-soxr) and found it was in an excellent condition.

@dofuuz
Copy link
Contributor Author

dofuuz commented Jul 21, 2022

@conda-forge-admin, please ping team

Hello. It seems like my package is missed 😭

Python-SoXR is one of most accurate and fastest samplerate converter for Python, mostly used for audio resampling.
Now it's being used by audio data scientists and AI researchers, have to resample massive amount of audio data.
It's optional dependency of librosa.

soxr conda-forge package already exists for c libsoxr, so I named this package python-soxr

Please review my PR and merge it! Thank you.

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-webservice.

I was asked to ping @conda-forge/staged-recipes and so here I am doing that.

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-webservice.

I was asked to ping @conda-forge/staged-recipes and so here I am doing that.

Copy link
Member

@carterbox carterbox left a comment

Choose a reason for hiding this comment

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

Package does not vendor other packages. (If a package uses the source of another package, they should be separate packages or the licenses of all packages need to be packaged).

This package is vending libsoxr because it uses the source code from the soxr package when compiling. libsoxr already exists on conda-forge, please patch this build to use/dynamically link against the soxr package that we already have or explain why static linking is needed.

Please name the package soxr-python (just this package, the github repo doesn't need to be rename). This follows our naming scheme and doesn't affect compatibility with PYPI because the name there is just soxr.

- numpy
- pip
- python
- setuptools_scm
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- setuptools_scm
- setuptools_scm
- soxr

Add soxr to the host requirements section, then patch your build to dynamically link against the pre-compiled libsoxr instead of building it again.

@conda-forge-linter
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 (recipes/soxr-python) and found it was in an excellent condition.

@dofuuz
Copy link
Contributor Author

dofuuz commented Jul 21, 2022

Python-SoXR uses static link by intention. So that it can be easily installed with pip, without any additional action.
Dynamic linking needs external library installed separately. Anaconda has own c library repository so it's fine, but pip doesn't.
Especially for Windows, getting and installing right libsoxr binary is not trivial job.

If this is not acceptable for conda-forge, another Python libsoxr wrapper should be made for it.

I renamed package to soxr-python.

@dofuuz dofuuz requested a review from carterbox July 21, 2022 06:34
@carterbox
Copy link
Member

easily installed with pip, without any additional action. Dynamic linking needs external library installed separately. Anaconda has own c library repository so it's fine, but pip doesn't.

As you have noted, conda packages don't have the same limitations as pip packages because they can depend on non-python packages.

Especially for Windows, getting and installing right libsoxr binary is not trivial job.

We have soxr already precompiled for Windows so getting and installing is not a problem. As for the "right" one, version 0.1.3 is available (the latest stable release).

If patching the build to optionally link libsoxr is not something you are willing to do, then maybe ask @bmcfee for help? Otherwise, feel free to abandon this PR.

Copy link
Member

@carterbox carterbox left a comment

Choose a reason for hiding this comment

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

There is not sufficient justification to revendor soxr.

@dofuuz dofuuz requested a review from carterbox August 11, 2022 13:16
@dofuuz
Copy link
Contributor Author

dofuuz commented Aug 17, 2022

@carterbox
I changed to dynamically link libsoxr. Can you review the package again?

Copy link
Member

@carterbox carterbox left a comment

Choose a reason for hiding this comment

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

It is not dynamically linking to libsoxr. If it was, the link inspector would be able to detect a link between cysoxr instead of reporting libsoxr is unused.

@dofuuz
Copy link
Contributor Author

dofuuz commented Aug 18, 2022

That's strange...... I'll take a look into it.

When using PEP 517 building, --install-option and --global-option are ignored.
@dofuuz
Copy link
Contributor Author

dofuuz commented Aug 18, 2022

@carterbox It seems like the problem is fixed. Can you check it again?
Sorry for bothering you.

@dofuuz dofuuz requested a review from carterbox August 18, 2022 15:11
@dofuuz
Copy link
Contributor Author

dofuuz commented Aug 18, 2022

Some note for commit 1335c33:
When using pip install with --use-pep517, --install-option and --global-option are ignored. pypa/pip#5771
Instead, use --config-settings

@carterbox carterbox merged commit 2718098 into conda-forge:main Aug 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants