Skip to content

Conversation

@branchv
Copy link
Contributor

@branchv branchv commented Nov 21, 2023

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.

@jeanas
Copy link
Contributor

jeanas commented Nov 21, 2023

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
Copy link
Contributor Author

branchv commented Nov 21, 2023

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
Copy link
Collaborator

Anteru commented Nov 21, 2023

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
Copy link
Contributor Author

branchv commented Nov 21, 2023

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
Copy link
Member

Sounds like a bug in hatchling...

@jeanas
Copy link
Contributor

jeanas commented Nov 21, 2023

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
Copy link
Contributor

jeanas commented Nov 21, 2023

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 jeanas merged commit c178ba3 into pygments:master Nov 21, 2023
@jeanas
Copy link
Contributor

jeanas commented Nov 21, 2023

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
Copy link
Member

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
Copy link
Collaborator

Anteru commented Nov 21, 2023

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

@Anteru
Copy link
Collaborator

Anteru commented Nov 21, 2023

@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
Copy link
Contributor

jeanas commented Nov 21, 2023

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
Copy link
Member

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
Copy link
Member

birkenfeld commented Nov 21, 2023

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
Copy link
Contributor

jeanas commented Nov 21, 2023

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
Copy link
Collaborator

Anteru commented Nov 21, 2023

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
Copy link
Member

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
Copy link
Contributor

jeanas commented Nov 21, 2023

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.

@jeanas jeanas mentioned this pull request Nov 21, 2023
Anteru pushed a commit that referenced this pull request Nov 21, 2023
Add a workaround for pypa/hatch#1054

(cherry picked from commit c178ba3)
@Anteru Anteru added this to the 2.17.2 milestone Nov 21, 2023
@Anteru
Copy link
Collaborator

Anteru commented Nov 21, 2023

Ok. Getting a 2.17.2 out now.

@Anteru
Copy link
Collaborator

Anteru commented Nov 21, 2023

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
Copy link
Contributor

jeanas commented Nov 21, 2023 via email

@Anteru
Copy link
Collaborator

Anteru commented Nov 21, 2023

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

@Anteru
Copy link
Collaborator

Anteru commented Nov 21, 2023

@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
Copy link
Contributor

jeanas commented Nov 21, 2023

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

@Anteru
Copy link
Collaborator

Anteru commented Nov 21, 2023

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

@jeanas
Copy link
Contributor

jeanas commented Nov 21, 2023

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

@branchv
Copy link
Contributor Author

branchv commented Nov 21, 2023

can confirm it works for us, thanks everyone!

@branchv branchv deleted the macos-sdist branch November 21, 2023 23:42
@ofek
Copy link
Contributor

ofek commented Dec 3, 2023

Sorry for the trouble everyone! Here's an update: pypa/hatch#1054 (comment)

@jeanas
Copy link
Contributor

jeanas commented Dec 4, 2023

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

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.

5 participants