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 xrst (extract rst files and run sphinx) #25198

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

bradbell
Copy link

@bradbell bradbell commented Feb 1, 2024

Purpose of xrst:
https://xrst.readthedocs.io/latest/purpose.html

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-webservices
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/xrst) and found some lint.

Here's what I've got...

For recipes/xrst:

  • Jinja2 variable definitions are suggested to take a {%<one space>set<one space><variable name><one space>=<one space><expression><one space>%} form. See lines [27, 29]

@bradbell
Copy link
Author

bradbell commented Feb 2, 2024

There seems to be a problem with the conda-forge version of pyenchant that is caussing some of the test failures; see
conda-forge/pyenchant-feedstock#1
We will try changing the test of xrst to use pyspellchecker instead of pyenchant.

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

@bradbell
Copy link
Author

bradbell commented Feb 6, 2024

I am convinced that the following commit truly passed the test on all the target systems: 0adf204

Waiting for feed back from the review team as per the instructions on
https://conda-forge.org/docs/maintainer/adding_pkgs.html
Once you finished your PR, all you have to do is wait for feedback from our review team.

@bradbell
Copy link
Author

bradbell commented Feb 6, 2024

@conda-forge/help-python, ready for review.

@bradbell
Copy link
Author

bradbell commented Feb 20, 2024

@conda-forge/help-python
It has been 2 weeks since this package passed all its tests and it does not yet have a review.

Copy link

To help direct your pull request to the best reviewers, please mention a topic-specifc team if your recipe matches any of the following: conda-forge/help-c-cpp, conda-forge/help-cdts, conda-forge/help-go, conda-forge/help-java, conda-forge/help-julia, conda-forge/help-nodejs, conda-forge/help-perl, conda-forge/help-python, conda-forge/help-python-c, conda-forge/help-r, conda-forge/help-ruby,or conda-forge/help-rust. Thanks!

'sphinx', 'toml', 'sphinx-copybutton', # required
'pytest', # need for testing xrst
- 'pyenchant', 'pyspellchecker', # need the one you use
+ 'pyspellchecker', # need the one you use
Copy link
Member

Choose a reason for hiding this comment

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

Since deps are specified in the meta.yaml it's not necessary to patch them in the setup.py... unless it's to enable pip check to work.

Copy link
Member

@dhirschfeld dhirschfeld left a comment

Choose a reason for hiding this comment

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

I don't think there are any issues which should prevent merging, however I don't have merge rights myself...

@bradbell
Copy link
Author

bradbell commented Mar 23, 2024

The following text appears in meta.yaml:

build:
  entry_points:
    - xrst = xrst:run_xrst
  noarch: python
  script: {{ PYTHON }} -m pip install . -vv --no-deps --no-build-isolation
  number: 0

As I understand, the commands above instruct conda-forge to use pip to install the python files.
I therefore thought that I should make sure that pyproject.toml and setup.py do not require pyenchant because pip uses these files.

@dhirschfeld
Copy link
Member

As I understand, the commands above instruct conda-forge to use pip to install the python files.

It uses pip to install just the package itself (it passes --no-deps automatically, even if you don't specify it https://github.com/conda/conda-build/blob/061063f10c51162707c7c5253daa39b2752fcfcc/conda_build/build.py#L3169-L3189)

The dependencies used are those specifiedin the meta.yaml. If they do happen to be different to what's in the upstream setup.py then pip check won't work so, sometimes it can be preferable to patch upstream dependencies but it's not necessary.

@bradbell
Copy link
Author

Thanks for the clarification. I made the suggested changes; see
db29691

@bradbell
Copy link
Author

bradbell commented Apr 3, 2024

I have been looking into fixing the pyenchant problem so that xrst can use it for a spell checker; see
conda-forge/hunspell-feedstock#12 (comment)

In any event, xrst will not be able to use pyenhant on windows because enchant is not available on windows; see
https://github.com/conda-forge/enchant-feedstock/blob/main/recipe/meta.yaml
where the following text appears:

build:
  number: 0
  skip: true  # [not unix]

So the solution here (not using pyenchant in the conda-forge install of xrst) may be the simplest because it makes xrst consistent for all platforms.

@BastianZim
Copy link
Member

Can you please create your recipe with grayskull once and see if that passes? If it fails on windows because there is a missing dependency, you can just ignore that.

https://github.com/conda/grayskull
https://conda-forge.org/docs/maintainer/knowledge_base/#noarch-python

@bradbell
Copy link
Author

bradbell commented May 11, 2024

I created a recipe for xrst using the command

grayskull pypi xrst

I then ran the command:

conda build xrst

and got the following error message:

ValueError: License file given in about/license_file (/home/bradbell/trash/conda/xrst/PLEASE_ADD_LICENSE_FILE) does not exist in source root dir or in recipe root dir (with meta.yaml)

Note that
https://github.com/conda-forge/staged-recipes/blob/main/recipes/example
suggests that one use SPDX identifiers for licenses.

Looking at
https://pypi.org/project/xrst/
you will see
License: OSI Approved (GPL-3.0-or-later)

As noted in at the top of xrst/meta.yaml, in the pull request:

# This project uses the SPDX identifier GPL-3.0-or-later; see
# https://spdx.org/licenses/GPL-3.0-or-later.html

@BastianZim
Copy link
Member

PLEASE_ADD_LICENSE_FILE is not the actual license, this means that you need to add the license file into the folder. In practise, that means that you need to manually download the license file from the project website and add it to the folder where the meta.yaml is.

@bradbell
Copy link
Author

In xrst/meta.yaml (generaged by grayskull) I changed
license_file: PLEASE_ADD_LICENSE_FILE to license_file: gpl-3.0.txt
and then executed

conda build xrst >& temp

If I execute grep ERROR temp I get

  ERROR setuptools_scm._file_finders.git listing git files failed - pretending there aren't any
  ERROR setuptools_scm._file_finders.git listing git files failed - pretending there aren't any

If I execute grep WARNING temp I get

WARNING: No numpy version specified in conda_build_config.yaml.  Falling back to default numpy value of 1.22

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