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 sphobjinv recipe to conda-forge #21285

Merged
merged 5 commits into from
Nov 28, 2022
Merged

Add sphobjinv recipe to conda-forge #21285

merged 5 commits into from
Nov 28, 2022

Conversation

anjos
Copy link
Contributor

@anjos anjos commented Nov 25, 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.
  • Package does not ship static libraries. If static libraries are needed, follow CFEP-18.
  • 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 just wanted to let you know that I linted all conda-recipes in your PR (recipes/sphobjinv) and found it was in an excellent condition.

@anjos
Copy link
Contributor Author

anjos commented Nov 25, 2022

@bskinn: please confirm your willingness to become a co-maintainer of this package as per conda-forge rules.

@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/sphobjinv) and found some lint.

Here's what I've got...

For recipes/sphobjinv:

  • noarch packages can't have skips with selectors. If the selectors are necessary, please remove noarch: python.

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

@anjos
Copy link
Contributor Author

anjos commented Nov 25, 2022

@ocefpaf: this PR concludes the work started on #21238, so we'd appreciate your advice. This is a noarch package, but it fails on Windows. I do not understand why it is tested on windows in the first place, nor why it fails. @bskinn - any ideas?

@anjos
Copy link
Contributor Author

anjos commented Nov 25, 2022

@conda-forge/help-python I'm tagging this for review.

@bskinn
Copy link
Contributor

bskinn commented Nov 25, 2022

@anjos:

@ocefpaf: this PR concludes the work started on #21238, so we'd appreciate your advice. This is a noarch package, but it fails on Windows. I do not understand why it is tested on windows in the first place, nor why it fails. @bskinn - any ideas?

It's tested on Windows because, if used on Windows, it needs to correctly handle EOLs when working with decompressed inventories. (This is less strictly true than it used to be, with Windows tools getting better at tolerating Unix EOLs, but is still necessary IMO.)

I'm not sure exactly which rev you're testing now, so I can't speak precisely to the Win test failures without more info. I think I've got everything chased down, the Azure Pipelines are looking good in this most recent CI run on sphobjinv#268. Not sure why the GH Actions job failed, now, though

@bskinn
Copy link
Contributor

bskinn commented Nov 25, 2022

Ah. Newly broken link in my Sphinx docs. Yay.

Definitely not an issue for the conda-forge packaging.

@anjos , new sdist coming shortly

@anjos
Copy link
Contributor Author

anjos commented Nov 25, 2022

It's tested on Windows because, if used on Windows, it needs to correctly handle EOLs when working with decompressed inventories. (This is less strictly true than it used to be, with Windows tools getting better at tolerating Unix EOLs, but is still necessary IMO.)

@bskinn: I meant the conda package itself, not your upstream. The so-called noarch conda-forge packages only build/test on linux. I'm not sure if there is any conda-forge provision to generate a single noarch package, but still test it across various OSes. @ocefpaf may be able to confirm this.

@bskinn
Copy link
Contributor

bskinn commented Nov 25, 2022

bskinn: I meant the conda package itself, not your upstream.

Aha, that's what I get for trying to diagnose on my phone, misread.

Change the pytest invocation to use double-quotes around "not readme". Single quotes apparently don't mean "treat this as a single argument" to Windows... sigh.

EDIT: I added a suggested edit to that spot in the YAML file.

@bskinn
Copy link
Contributor

bskinn commented Nov 25, 2022

Full disclosure: sphobjinv does currently vendor an old version of fuzzywuzzy, tweaked so that the pieces that sphobjinv needs are Python 3 compatible.

This old version is required due to a licensing conflict with newer fuzzywuzzy (details at bskinn/sphobjinv#264 (comment)). Simply pinning to this license-compatible old version is not feasible because of the lack of Python 3 compatibility available at that version. Vendoring and adapting seemed the least-worst option. The vendored code will not be updated with new fuzzywuzzy code over time, and my goal is to eventually be able to remove the vendored code, as part of bskinn/sphobjinv#178 and following work.

The sdist does include a copy of the appropriate license file alongside the vendored code, and my modifications to that code are marked in the vendored source.

@ocefpaf
Copy link
Member

ocefpaf commented Nov 28, 2022

but still test it across various OSes. @ocefpaf may be able to confirm this.

Nope. It would be nice but the whole idea of noarch is that the test is equivalent on the OSes and platforms. What you can do is to try the "brand new" some-arch packages. Those are packages that have platform specific stuff, build and test on each platform but not all all Pythons.

@bskinn
Copy link
Contributor

bskinn commented Nov 28, 2022

I test sphobjinv aggressively cross-platform (Linux/MacOS/Win), cross-Python-version, and cross-dependency-version on all my releases, so I'm pretty comfortable with the conda-forge recipe only testing on POSIX. The odds of a platform-specific bug emerging between my tests and the conda-forge build seem awfully small.

@ocefpaf
Copy link
Member

ocefpaf commented Nov 28, 2022

so I'm pretty comfortable with the conda-forge recipe only testing on POSIX

That is what I needed to hear (read).

@ocefpaf ocefpaf merged commit 19eb271 into conda-forge:main Nov 28, 2022
@anjos anjos deleted the add-sphobjinv branch November 28, 2022 20:05
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.

4 participants