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

🔧 MAINTAIN: Move to flit for PEP 621 package builds #1645

Merged
merged 7 commits into from
Feb 23, 2022

Conversation

choldgraf
Copy link
Collaborator

@choldgraf choldgraf commented Feb 22, 2022

This removes our setup.py file, which we had marked for removal once pypa/packaging-problems#256 was resolved (and it is now resolved!)

I also updated a couple of action jobs since we needed to update our build distribution action.

I tested pip install -e . locally without setup.py and it seemed to work as expected. Will leave this open for a few days in case others think this is not a good action to take yet.

@chrisjsewell
Copy link
Contributor

I would also just move to flit and remove setup.cfg and MANIFEST.in

@choldgraf
Copy link
Collaborator Author

choldgraf commented Feb 22, 2022

@chrisjsewell have you found any nice tutorials on that? I'm happy to give it a try but it'd be a first for me.

I guess the process would basically be:

  • step through the config in setup.cfg and MANIFEST.in and map that on to the appropriate parts of pyproject.toml.
  • update our action to do flit publish rather than using python -m build and then the PyPI upload action

is that right?

@codecov
Copy link

codecov bot commented Feb 22, 2022

Codecov Report

Merging #1645 (8693df3) into master (abf1183) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1645   +/-   ##
=======================================
  Coverage   91.30%   91.30%           
=======================================
  Files           7        7           
  Lines         690      690           
=======================================
  Hits          630      630           
  Misses         60       60           
Flag Coverage Δ
pytests 91.30% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update abf1183...8693df3. Read the comment docs.

@chrisjsewell
Copy link
Contributor

Well just the flit documentation should be mainly all you need, you can also try https://ini2toml.readthedocs.io/en/latest/readme.html, and I've already done this in e.g. aiidateam/aiida-core#5312

@chrisjsewell
Copy link
Contributor

the MANIFEST.in can basically just be deleted, since flit handles all that automatically
Plus this could be removed: https://github.com/executablebooks/jupyter-book/blob/abf118388834b2c97ec74aac4fae095e35481a19/.pre-commit-config.yaml#L22-L34

This would also need updating, but wouldn't be too hard: https://github.com/executablebooks/jupyter-book/blob/abf118388834b2c97ec74aac4fae095e35481a19/.pre-commit-config.yaml#L36-L49

@choldgraf
Copy link
Collaborator Author

will try to give it a shot tonight and ping you if anything comes up

pyproject.toml Outdated
"docutils>=0.15,<0.18",
"jsonschema<4",
# Include Jupytext to ensure users have the right version, even if not strictly necessary
"jupytext>=1.11.2,<1.12", # markdown-it-py~=1 required and support from 1.11.2
Copy link
Collaborator Author

@choldgraf choldgraf Feb 23, 2022

Choose a reason for hiding this comment

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

An aside - I noticed that we're pinning jupytext on a minor version rather than a major version. It's now on release 1.13.7 - do we still need to keep this pin?

Copy link
Contributor

@chrisjsewell chrisjsewell Feb 23, 2022

Choose a reason for hiding this comment

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

Well, we kind of don't need the jupytext dependency at all.
The lower pin was added when markdown-it-py was only an extra of jupytext, so you could end up with non-matching versions, and jupytext would just fail to convert properly.
But now, markdown-it-py is a first-class dependency of jupytext: https://github.com/mwouts/jupytext/blob/8cfa66bef380bf6d54479a06b092da30dbc9a7c4/setup.py#L38

@choldgraf
Copy link
Collaborator Author

OK I believe that I got a working flit configuration setup. I was able to successfully run flit build and it built without any errors. Looks reasonable to you @chrisjsewell ?

pyproject.toml Outdated
build-backend = "flit_core.buildapi"

[project]
name = "jupyter_book"
Copy link
Contributor

@chrisjsewell chrisjsewell Feb 23, 2022

Choose a reason for hiding this comment

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

The only "concern" of this name change, from jupyter-book to jupyter_book, is that although pip treats them the same (normalising - and _), conda for example does not. So I'm not sure if there would definitely not be any issue.

You can keep the name as jupyter-book, and set the module name separately, with:

[tool.flit.module]
name = "jupyter_book"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah perfect, will do this.

@chrisjsewell
Copy link
Contributor

Apart from two comments, looks good 👍

@choldgraf
Copy link
Collaborator Author

choldgraf commented Feb 23, 2022

OK I addressed both comments w/ the changes suggested. Jupytext is now in our [sphinx] and [testing] optional dependencies, since it's needed for building from an Rmd file. Also added a note for others to install it if they want to use RMD.

There's also some functionality that requires Jupytext within jupyter book, but we gave some nice warning messages about it:

https://github.com/executablebooks/jupyter-book/blob/18d52700b8636773f96631b3e187e38075557041/jupyter_book/utils.py#L66-L74

Copy link
Contributor

@chrisjsewell chrisjsewell left a comment

Choose a reason for hiding this comment

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

👍

@chrisjsewell chrisjsewell changed the title MAINT: Remove setup.py 🔧 MAINTAIN: Move to flit for PEP 621 package builds Feb 23, 2022
@choldgraf choldgraf merged commit d988a63 into jupyter-book:master Feb 23, 2022
@choldgraf choldgraf deleted the setup.py branch February 23, 2022 21:19
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