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.

Sign up for GitHub

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

Use jupyter-packaging for npm build #211

Merged
merged 2 commits into from
Sep 2, 2021
Merged

Use jupyter-packaging for npm build #211

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bcf9873
Use jupyter-packaging for npm build
manics Sep 1, 2021
1932cf3
Add package.json to MANIFEST.in to support a js rebuild
consideRatio Sep 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions MANIFEST.in
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
include *.md
include LICENSE
include package.json
include setup.cfg
recursive-include nbgitpuller/static *
recursive-include nbgitpuller/templates *
1 change: 1 addition & 0 deletions dev-requirements.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
jupyter_packaging>=0.10
six
pytest
pytest-cov
Expand Down
7 changes: 7 additions & 0 deletions pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[build-system]
requires = [
"build",
"jupyter_packaging>=0.10",
"setuptools",
]
build-backend = "setuptools.build_meta"
20 changes: 16 additions & 4 deletions setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,20 @@
from jupyter_packaging import wrap_installers, npm_builder
from setuptools import find_packages, setup
from distutils.util import convert_path
import subprocess
import os.path

HERE = os.path.abspath(os.path.dirname(__file__))

# Representative files that should exist after a successful build
jstargets = [
os.path.join(HERE, "nbgitpuller", "static", "dist", "bundle.js"),
]

# https://github.com/jupyter/jupyter-packaging/blob/0.10.4/README.md#as-a-build-requirement
jsdeps = npm_builder(build_cmd="webpack")
cmdclass = wrap_installers(
pre_develop=jsdeps, pre_dist=jsdeps,
ensured_targets=jstargets, skip_if_exists=jstargets)
Comment on lines +1 to +17
Copy link
Member

@consideRatio consideRatio Sep 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think you should wrap this in a try catch block, so that you don't assume jupyter_packaging to be available.

See this example from the jupyter_packaging readme file https://github.com/jupyter/jupyter-packaging#as-a-build-requirement

I'm thinking that if we have built the package so that the bundle.js stuff is available, then we are fine. If we on the other hand just want to install things from a package source distribution, we don't need pyproject.toml, package.json, or anything - we just need the bundle to be available and keep working as normal in setup.py.

Copy link
Member Author

@manics manics Sep 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I originally followed the example, but that means setup.py can silently ignores the js build which didn't seem particularly great.

It looks like pyproject.toml is included anyway in the sdist (at least it is when I tested locally, and pip install ~/path/to/nbgitpuller/dist/nbgitpuller-1.0.1.dev0.tar.gz works without errors).

What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

Long discussion here which I don't fully understand pypa/setuptools#1650
... but it looks like this project assumes pep517 anyway

nbgitpuller/.github/workflows/publish.yml

Line 27 in 4ffd009

python -m build --sdist --wheel .

Copy link
Member

Choose a reason for hiding this comment

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

Nice investigation @manics! Okay my understanding now:

  1. pyproject.toml will always ship with the source when we package the source with build --sdist --wheel . This means that jupyter_packaging can be assumed in setup.py, so we don't need to wrap the import statement etc in a try block.
  2. The nbgitpuller/static/dist content will also be shipped, and jupyter_packaging will not attempt to do a npm setup unless bundle.js isn't around anyhow because of skip_if_exists=jstargets. This means that we currently can do pip install . from the source file. If you on the other hand wants to rebuild the bundle.js file you won't be able to do so because then we miss package.json. I've verified this part.

I suggest we include package.json in our MANIFEST.in file as part of this PR.

I can't make a convenient code suggestion about that so I pushed a commit. If it LGTY also @manics then go for a merge! This LGTM now!


# Imports __version__, reference: https://stackoverflow.com/a/24517154/2220152
ns = {}
Expand All @@ -9,16 +23,14 @@
exec(ver_file.read(), ns)
__version__ = ns['__version__']

subprocess.check_call(['npm', 'install'])
subprocess.check_call(['npm', 'run', 'webpack'])

setup(
name='nbgitpuller',
version=__version__,
url='https://github.com/jupyterhub/nbgitpuller',
license='3-clause BSD',
author='Peter Veerman, YuviPanda',
author_email='[email protected]',
cmdclass=cmdclass,
description='Notebook Extension to do one-way synchronization of git repositories',
long_description=open('README.md').read(),
long_description_content_type='text/markdown',
Expand Down