fix building from source on macos by branchv · Pull Request #2593 · pygments/pygments

@branchv

Since #2573 switched to hatchling, macOS users cannot build from source:

$ python3 -m venv .venv
$ .venv/bin/pip install --no-binary=pygments pygments
$ .venv/bin/python3 -c 'import pygments'
Traceback (most recent call last):
  File "<string>", line 1, in <module>
ModuleNotFoundError: No module named 'pygments'

This is because the wheel gets packaged as Pygments (prob due to macOS's case-insensitive filesystem). This explicitly tells hatchling that the package is pygments.

@branchv

@jeanas

I find it hard to imagine that installing hatchling-built packages would be broken for everyone on macOS so I suspect there is something else going on. Let me investigate.

@branchv

It's not broken for everyone, just macOS users building from source (i.e. using --no-binary, which is very few people but does include Homebrew. See Homebrew/homebrew-core#154759, and our failing CI here).

@Anteru

Time for a macOS CI builder? Also, this is weird, because the file generated is pygments now, no longer Pygments (at least on Linux) and I noticed the case change. Why would macOS hatchling suddenly produce an upper-case Pygments?

@branchv

Why would macOS hatchling suddenly produce an upper-case Pygments?

So the sdist is correctly cased as pygments, but when hatchling builds a wheel from that sdist it infers the package name by first checking if Pygments/__init__.py is a file and then later pygments/__init__.py here. That works on Linux but on a case-insensitive filesystem like macOS, the former is "found" and thus hatchling uses it

@birkenfeld

Sounds like a bug in hatchling...

@jeanas

If you ask me, any case-insensitive file system is a bug 😉 More seriously, yes, it does, I've just filed it as pypa/hatch#1054

@jeanas

@jeanas

After SSHing into a macOS VM that I have access to for another project, I can confirm that this fixes the problem. The wheel contents seem correct, including the RECORD and the capitalized name in METADATA. Merging, thank you! And it looks like we're going to need another bugfix release :(

@jeanas

Time for a macOS CI builder?

Yes, probably.

Also, this is weird, because the file generated is pygments now, no longer Pygments (at least on Linux) and I noticed the case change. Why would macOS hatchling suddenly produce an upper-case Pygments?

Just to be sure we're on the same page: it's not about the wheel file name, but the contents of the wheel.

In a wheel, you need to trim everything that's not needed: docs, tests, ... In our case, basically just keep everything in pygments/. This is hatchling's default behavior because it detects the very common case of having an import package that matches the project name. However, it tries the unnormalized project name (Pygments) first and then the normalized name (pygments). On macOS, a Pygments/ directory "exists", so it thinks that there is a Pygments/ package, and it adds all the files under a Pygments/ directory in the ZIP archive.

I'm pretty surprised that we're the first project to hit this (or maybe it was introduced recently in hatchling?).

@birkenfeld

Time for a macOS CI builder?

Yes, probably.

Are they still unproportionally slow? If yes, I'd vote to only use it selectively before release.

@Anteru

We could configure one to only run on "tagged" builds. I think GitHub allows for that.

@Anteru

@jeanas Do I get it right we need a 2.17.2 to fix the packaing issue on macOS as currently there's nothing working on macOS? Can't they use the package from PyPI? I don't have a mac so I don't understand the extent of the issue. Not being able to build Pygments on macOS doesn't seem to warrant a hotfix if they can still use it.

@jeanas

Are they still unproportionally slow? If yes, I'd vote to only use it selectively before release.

We can also default to testing macOS with just one Python version instead of all supported ones, to reduce the matrix? Should be enough for the CI on each PR, though we may want more before a release...

@jeanas Do I get it right we need a 2.17.2 to fix the packaing issue on macOS as currently there's nothing working on macOS? Can't they use the package from PyPI? I don't have a mac so I don't understand the extent of the issue. Not being able to build Pygments on macOS doesn't seem to warrant a hotfix if they can still use it.

On PyPI, there is an sdist and a wheel. I'll refer you to this recent not yet merged PR if you're not sure what the usefulness of each is (I opened it because I used to be pretty confused about it...).

pip defaults to selecting wheels, so pip install pygments does work for most macOS users right now, because the wheel is the one you built on your Linux machine while doing the release, which is in good shape.

What doesn't work is (much more rare) things like pip install --no-binary :all: where pip is forced to build from the sdist. A few people do that sort of thing, e.g., because of extension modules that they want to build from source against Homebrew-provided libraries (for example). Or, because they want to audit everything and many builds aren't reproducible (I'm a student who's never worked in a corporate environment, so I can't tell you how common that actually is, but I've heard it exists).

@birkenfeld

IMO this kind of bug is an annoyance that should be addressed by a release, especially since it's a regression caused by us switching the build system.

@birkenfeld

That said, I have no objections to just picking the single commit onto 2.17.1 for the release, instead of releasing from current master.

@jeanas

That said, I have no objections to just picking the single commit onto 2.17.1 for the release, instead of releasing from current master.

I think this is a good idea. Some of the recent changes are quite huge diffs, let's not fold them into a point release.

@Anteru

I do think it should be fixed in the next release, I'm arguing if this warrants 2.17.2 on its own if PyPI users can use Pygments using the recommended installation method. If it's "much more rare" -- is it rare enough to meet whatever arbitrary bar we have for a point release, or is it too rare to warrant a fix now? I understand it's a regression, but it seems a fairly minor one.

If we do agree that 2.17.2 it is, then yes, I'll branch it off 2.17.1, and we'll cherry-pick that commit over. I would argue though that we should have a CI test for this to make sure we're actually not breaking anything while doing so. I have no access to a mac machine to try myself, and I feel somewhat uncomfortable about this. Sure, I can merge this PR and believe everyone it's working, but having a way to actually verify this would be much better.

Can I get a quick vote on 2.17.2 yay/nay for this one fix only? My vote is no, fold into 2.18, but we strategically decided to have three maintainers so you can overrule me.

@birkenfeld

I vote for the 2.17.2. If it's such a chore to do a hotfix then we really need to change the process.

@jeanas

I vote for the 2.17.2. If it's such a chore to do a hotfix then we really need to change the process.

+1 on both points.

Anteru pushed a commit that referenced this pull request

Nov 21, 2023

@branchv @Anteru

@Anteru

Ok. Getting a 2.17.2 out now.

@Anteru

Something is broken. I'm building the branch release/2.17.x, the wheel size looks fine, but the tar.gz is now 983 KiB instead of the expected 4.8 MiB :( Seems to be missing all tests and everything. Is this another hatchling problem? @jeanas , did this remove all tests from the sdist as a drive-by?

(pygments) anteru@vm:~/pygments$ python -m build
* Creating virtualenv isolated environment...
* Installing packages in isolated environment... (hatchling)
* Getting build dependencies for sdist...
* Building sdist...
* Building wheel from sdist
* Creating virtualenv isolated environment...
* Installing packages in isolated environment... (hatchling)
* Getting build dependencies for wheel...
* Building wheel...
Successfully built pygments-2.17.2.tar.gz and pygments-2.17.2-py3-none-any.whl
(pygments) anteru@vm:~/pygments$ ls -lah dist/
total 2.1M
drwxr-xr-x  2 anteru anteru 4.0K Nov 21 21:13 .
drwxr-xr-x 10 anteru anteru 4.0K Nov 21 21:13 ..
-rw-------  1 anteru anteru 1.2M Nov 21 21:13 pygments-2.17.2-py3-none-any.whl
-rw-------  1 anteru anteru 983K Nov 21 21:13 pygments-2.17.2.tar.gz
(pygments) anteru@vm:~/pygments$ cd dist/
(pygments) anteru@vm:~/pygments/dist$ tar xzf pygments-2.17.2.tar.gz 
(pygments) anteru@vm:~/pygments/dist$ ls
pygments-2.17.2  pygments-2.17.2-py3-none-any.whl  pygments-2.17.2.tar.gz
(pygments) anteru@vm:~/pygments/dist$ cd pygments-2.17.2/
(pygments) anteru@vm:~/pygments/dist/pygments-2.17.2$ ls
AUTHORS  description.rst  LICENSE  PKG-INFO  pygments  pyproject.toml

@jeanas

jeanas commented

Nov 21, 2023

via email

Loading

@Anteru

Thanks. I added some more information. If I had to guess, the packages statement makes it only include the pygments folder.

@Anteru

@birkenfeld Not a chore per se, more like a wild ride with no safety belt and on a rusty looping designed by someone playing Rollercoaster Tycoon.

@jeanas

Oh, I see. I've pushed 2972a1c to master.

@Anteru

Ok. I'll cherry-pick that as well.

@jeanas

2.17.2 is out. Thank you @Anteru! @branchvincent Could you confirm it worked for Homebrew?

@branchv

can confirm it works for us, thanks everyone!

@ofek

@jeanas

This is now fixed in hatchling, thank you @ofek!