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

Build JavaScript files during python package build #697

Merged
merged 1 commit into from
Jan 4, 2022
Merged

Build JavaScript files during python package build #697

merged 1 commit into from
Jan 4, 2022

Conversation

stefangehn
Copy link
Contributor

Make package builds via setuptools automatically trigger building of
merged & minified JS assets.

This fixes installing isso from source when using pip install as long
as both the python and node requirements are met. It also ensures that
setup.py commands like build or bdist_wheel create archives that
contain generated JS code.

@ix5
Copy link
Member

ix5 commented Feb 3, 2021

That feels a bit too magic to me? Maybe just warn if the minified js files are not found.

@stefangehn
Copy link
Contributor Author

I wouldn't know what part should warn about these files missing.

Since this step is required for building a working redistributable package (may it be a wheel or just a tarball) this looks like a good place to me. It also makes it obvious that you will need npm if you use pip install on the github URL instead of failing more or less silently in the browser because the required files are missing.

The idea for this was actually taken from the readthedocs sphinx theme:
https://github.com/readthedocs/sphinx_rtd_theme/blob/master/setup.py#L16

@ix5
Copy link
Member

ix5 commented Feb 4, 2021

Again, just my personal preference. I can see that your approach would solve a lot of annoying newbie issues.

The primary reason for people to install from source is because the isso package in pypi has historically been outdated or even broken (werkzeug==1.0) for long stretches of time.

People installing directly via pip install git+https://github.com/posativ/isso would need to have node+npm installed already, which might be non-obvious. I'd also wager that these people don't really want to install all this js ecosystem stuff just to get an updated package.

My favourite way to resolve this whole issue would be to keep minified versions of the js assets in this repo, but sadly that might break the make-based change detection.


(I guess your approach wouldn't affect FTBFS issues since you'd invoke the make commands in the "prepare" stage anyway, and not use python to generate the minified js assets?)

@ix5
Copy link
Member

ix5 commented Feb 5, 2021

I've created #703 to clarify more issues that might pop up when installing from source.
But I'm growing more and more partial to your approach, considering e.g. the experience of users in #656.

setup.py Outdated

def run(self):
if 'TOX_ENV' not in os.environ:
subprocess.run(['make', 'init', 'js'], check=True)
Copy link
Member

Choose a reason for hiding this comment

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

check_call() ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed. Although subprocess.run(..., check=True) does the same I find subprocess.check_call() to be a bit more obvious and in fact know it from Python 2.7 already.

@stefangehn
Copy link
Contributor Author

Rebased and applied the check_call() suggestion.

Sorry for the long delay, work and health kept me busy lately.

@ix5 ix5 mentioned this pull request Dec 22, 2021
Copy link
Member

@jelmer jelmer left a comment

Choose a reason for hiding this comment

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

Please resolve the conflict?

@ix5 ix5 self-assigned this Dec 27, 2021
Make package builds via setuptools automatically trigger building of
merged & minified JS assets.

This fixes installing isso from source when using "pip install" as long
as both the python and node requirements are met. It also ensures that
setup.py commands like "build" or "bdist_wheel" create archives that
contain generated JS code.
@jelmer jelmer merged commit bbad14e into isso-comments:master Jan 4, 2022
@alexandru alexandru mentioned this pull request Jan 13, 2022
@stefangehn stefangehn deleted the python-build-js branch January 30, 2022 17:46
@ix5
Copy link
Member

ix5 commented Feb 13, 2022

Note that packages created by python3 setup.py sdist and installed via pip will attempt to run the NpmBuildCommand.
A way to specify that the command is only to be run when installing from source is needed.

@ix5 ix5 added bug server (Python) server code labels Feb 13, 2022
@ix5 ix5 added this to the 0.12.6 milestone Feb 13, 2022
ix5 added a commit to ix5/isso that referenced this pull request Feb 14, 2022
@stefangehn
Copy link
Contributor Author

Feel free to revert, I don't need it myself and I don't intend to dive any further into the sorry state of python packaging

@jelmer
Copy link
Member

jelmer commented Feb 16, 2022

Let's revert.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug server (Python) server code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants