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

Generate .pyc files from .hy files on install #2299

Merged
merged 1 commit into from
Jun 7, 2022

Conversation

scauligi
Copy link
Member

@scauligi scauligi commented Jun 2, 2022

Change setup.py to generate .pyc files from .hy files on install (which will cause setup.py to include them in wheel distributions as well).

Fixes #1747; a similar patch to hyrule's setup.py will fix hylang/hyrule#42. I can't test rpm/deb/etc generation, but with the way this generates the source/wheel distributions it should fix the .pyc recompilation issue regardless of the end package manager.

Note that this now makes wheels specific to a python version (ie instead of hy-0.21-py3-none-any.whl, running setup.py bdist_wheel will now generate hy-0.21-cp310-none-any.whl)
So for Pypi we (really, @Kodiologist) will want to generate wheels for each version of python we support. The source distribution doesn't contain .pyc files so it's still version agnostic.

@scauligi scauligi force-pushed the packaged_hy_pycs branch 2 times, most recently from 08648bc to 4f725cf Compare June 2, 2022 02:06
@Kodiologist
Copy link
Member

I don't really know anything about wheels. Is the reason we have to produce version-specific wheels that the compilation has to be done before packaging into a wheel rather than while installing the wheel, because the wheel installation process doesn't allow running any code?

@Kodiologist
Copy link
Member

Also, does the import hy line in class install work even if the user doesn't already have some version of Hy installed?

@scauligi scauligi force-pushed the packaged_hy_pycs branch from 4f725cf to a5f8e43 Compare June 2, 2022 17:31
@scauligi
Copy link
Member Author

scauligi commented Jun 2, 2022

compilation has to be done before packaging into a wheel

Yup, there's no way to have post-install hooks with wheels, it's essentially just an archive that gets copied into your site-packages. So we have to package the .pyc files into the wheel itself, which means we'll have python-version-specific wheels (since the .pyc files are specific to each python version).

Frustratingly enough, pip will compile all .py files to .pyc after a wheel install, but it's a hardcoded check for filenames ending in ".py" and there's no way to hook into it (or anywhere afterwards).

Also, does the import hy line in class install work even if the user doesn't already have some version of Hy installed?

Yup, since the prior call to super().run() performs the actual installation, hy is locatable in the sys.path by that point. I've tested installing from clean virtualenvs (and managed to get a nix environment set up to test from there as well), and can confirm that installing from the sdist .tar.gz file works both with and without wheel already installed (pip will rebuild a source as a wheel before installing if it can), and that installing directly from the bdist_wheel .whl file also works.

@Kodiologist
Copy link
Member

Hmm. What if we just forget about wheels and only provide source distributions? That will simplify things, and there's little advantage to a wheel over a source distribution for a project with no C code like this one, right?

@scauligi
Copy link
Member Author

scauligi commented Jun 2, 2022

That also works! If we go that route, then I can just remove bdist_wheel stuff (just tested it still works even when pip generates its own wheel before install).

@Kodiologist
Copy link
Member

Kodiologist commented Jun 2, 2022

Okay, sounds good.

As I understand the bit with mode, we're making sure that when we call py_compile.compile, we use the same invalidation_mode as was used for the compilation of __init__.py. Why is that necessary? Is it related to the Nix issue?

@scauligi scauligi force-pushed the packaged_hy_pycs branch from a5f8e43 to eb3fcab Compare June 4, 2022 00:34
@scauligi
Copy link
Member Author

scauligi commented Jun 4, 2022

Why is that necessary?

It isn't strictly necessary; we can have mode always set to CHECKED_HASH and it will work. Nix builds (and I assume other packaging systems) compile the .py files with CHECKED_HASH.
I added the code to try to compile with TIMESTAMP mode if possible since it's what python usually generates default; it's faster to check timestamps than hash the whole source files, and this will only come into play for "normal" installations.

@Kodiologist
Copy link
Member

So why not just leave invalidation_mode at its default, allowing py_compile.compile to set it to CHECKED_HASH if the user sets the environment variable SOURCE_DATE_EPOCH, as usual? Am I missing something here?

@scauligi scauligi force-pushed the packaged_hy_pycs branch from eb3fcab to dadf2e0 Compare June 4, 2022 17:23
@scauligi
Copy link
Member Author

scauligi commented Jun 4, 2022

...no, no, you're right, that code was only there to make sure that we made hash-based .pycs when pre-packaging wheels. Since we're only supplying sdists from here on out, it's completely redundant (and I ran a couple installs just to double check).

@scauligi scauligi force-pushed the packaged_hy_pycs branch from dadf2e0 to de9d228 Compare June 4, 2022 17:27
@Kodiologist
Copy link
Member

Thanks. I think this generates and installs the bytecode, but the bytecode isn't being used, at least on my system. I'm on Ubuntu 22.04. Here's what it looks like for me:

$ git clone https://github.com/scauligi/hy.git
Cloning into 'hy'...
…
$ cd hy 
$ git checkout packaged_hy_pycs 
Branch 'packaged_hy_pycs' set up to track remote branch 'packaged_hy_pycs' from 'origin'.
Switched to a new branch 'packaged_hy_pycs'
$ python3 get_version.py
$ sudo -H pip3 install .
…
Successfully installed hy-0.19.0+834.gde9d2280
…
$ cd /tmp
$ HY_MESSAGE_WHEN_COMPILING=1 hy
Compiling /usr/local/lib/python3.10/dist-packages/hy/core/macros.hy
Compiling /usr/local/lib/python3.10/dist-packages/hy/core/hy_repr.hy
Hy 0.19.0+834.gde9d2280 using CPython(main) 3.10.4 on Linux
=> 
now exiting HyREPL...
$ HY_MESSAGE_WHEN_COMPILING=1 hy
Compiling /usr/local/lib/python3.10/dist-packages/hy/core/macros.hy
Compiling /usr/local/lib/python3.10/dist-packages/hy/core/hy_repr.hy
Hy 0.19.0+834.gde9d2280 using CPython(main) 3.10.4 on Linux
=> 
now exiting HyREPL...

Do you find that the files aren't recompiled on your system?

@scauligi
Copy link
Member Author

scauligi commented Jun 5, 2022

Ugh, I never tested using pip install .. The problem is:

  1. pip kicks off a wheel build
  2. setup.py compiles hy -> pyc (timestamps: source from current directory)
  3. pip installs the wheel (source files in sitepackages now have new timestamps; hy pyc timestamps are now mismatched)
  4. pip compiles sitepackages py -> pyc (so the py timestamps still work out)

Easy fix is to just compile with CHECKED_HASH always.

I also ran into an additional snafu where if I was installing using pip install . into a venv with --system-site-packages enabled, but without funcparserlib installed at the system level, then hy (during setup.py) failed to import funcparserlib and failed to compile the pyc files.

Easy fix is to add funcparserlib (and the other install_requires) as setup_requires as well.

I'll push these fixes, but I'm also trying to see if there's some not-gross way I can add all of these cases as tests, so CI can catch it if anything breaks down the line.

@scauligi scauligi force-pushed the packaged_hy_pycs branch from de9d228 to b66c2ba Compare June 5, 2022 19:03
@Kodiologist
Copy link
Member

The fact that source-file timestamps aren't preserved sounds like a bug in how wheels are implemented, but I guess that's not something we can fix.

@Kodiologist
Copy link
Member

Anyway, this seems to work. Can you update the commit message and add a line to NEWS? Also, you don't need import install as _install; you can say class install(install): … or class install(installsetuptools.command.install.install): … or name your new class something other than install.

@scauligi scauligi force-pushed the packaged_hy_pycs branch from b66c2ba to 5558a32 Compare June 5, 2022 19:20
@scauligi
Copy link
Member Author

scauligi commented Jun 5, 2022

name your new class something other than install

I'm worried that make break something arcane; sifting through the setuptools internals, there are a few cases where it uses the name of the command's class for various things. All of the sample code I've seen that overrides setuptools commands use the same import as _name ... class name(_name) pattern ¯\_(ツ)_/¯

@scauligi scauligi force-pushed the packaged_hy_pycs branch from 5558a32 to 3c3587c Compare June 5, 2022 19:23
@Kodiologist
Copy link
Member

Okay, class install(install): … should be fine, then. The examples with class name(_name): … are presumably due to confusion about name resolution in Python, or perhaps a reluctance to reassign names.

@scauligi scauligi force-pushed the packaged_hy_pycs branch 6 times, most recently from bb9df38 to 1fc85b5 Compare June 6, 2022 00:50
@scauligi scauligi force-pushed the packaged_hy_pycs branch from 1fc85b5 to 4b1b130 Compare June 6, 2022 00:58
@scauligi
Copy link
Member Author

scauligi commented Jun 6, 2022

Aight I added some tests for checking the generated pyc files after both an sdist install and an install directly from the source folder. They're... really slow, and also potentially require an internet connection (since they'll call pip to install packages). You can skip them locally using pytest -m "not online"

I should be able to get to a windows machine by later this week to figure out what's going on there, but everything else is working.

ugh I don't know why I keep attempting these weird invasive/internal changes, apparently I'm a glutton for punishment

@Kodiologist
Copy link
Member

I'd say let's not worry about testing. Trying to test this kind of thing automatically, with all the slightly different ways a Python package can be installed, is too messy.

@Kodiologist
Copy link
Member

@scauligi Do you mind if I remove the testing commit and merge this?

@scauligi
Copy link
Member Author

scauligi commented Jun 7, 2022

I don't mind; go ahead

@Kodiologist Kodiologist merged commit a581292 into hylang:master Jun 7, 2022
@scauligi scauligi deleted the packaged_hy_pycs branch July 16, 2022 22:23
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.

Hyrule isn't using bytecode files Slow startup when Hy is installed from a wheel
2 participants