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

Allow lzma compressed data file #4

Closed
wants to merge 2 commits into from
Closed

Conversation

tornaria
Copy link
Contributor

An attempt at #3.

This will use CPimport.txt.lzma if available, else fallback to CPimport.txt as before.

Note that this doesn't attempt to compress the file at install time, since I don't know how to do that. But one could e.g. compress the file when packaging for a distro, and it would work transparently. I do this for pari data, which support replacing text files by gzipped files.

@orlitzky
Copy link
Collaborator

I'd like this too, but I'd prefer to just do it as part of the pip wheel or python -m build process so (a) the name will be predictable, (b) pip users will get the benefits, (c) distro packaging can remain uninteresting.

@orlitzky
Copy link
Collaborator

I should mention: I don't know how to do it with this week's python build tooling or else I would just do it. I'll ask around.

@mkoeppe
Copy link

mkoeppe commented Jan 11, 2024

Since you are using setuptools, it would just be done by customizing setuptools.command.build_py; there hasn't really been a change to this since the distutils era.

@tornaria
Copy link
Contributor Author

I don't know how to fix this. It works if one builds a wheel (python -m build -w) but it doesn't work if one builds via sdist (python -m build, which is equivalent to what CI is doing here). It seems the sdist doesn't contain either CPimport.txt nor CPimport.txt.xz and so build_py cannot proceed. But if I keep CPimport.txt in the manifest, then the sdist contains CPimport.txt but the wheel contains both CPimport.txt and CPimport.txt.xz.

@tornaria
Copy link
Contributor Author

Question: the sdist should contain the original txt file, or the compressed file?

@tornaria
Copy link
Contributor Author

Ok, this seems to work: the CPimport.txt file is kept in the manifest, so it will be included in the sdist. And the build_py step replaces the installed file by the compressed one.

@orlitzky
Copy link
Collaborator

I spent at least an hour trying to find out what I needed to add to pyproject.toml to override build_py, and you're telling me it just works??

I think you did the right thing with the sdist. I pushed the changes, added some types, and tagged version 0.9. Matthias has to do the pypi release and then I guess we'll see how it all works in real life.

@orlitzky orlitzky closed this Jan 13, 2024
@mkoeppe
Copy link

mkoeppe commented Jan 13, 2024

Matthias has to do the pypi release

Actually it's all automated. It ran here when you pushed the tag: https://github.com/sagemath/conway-polynomials/actions/runs/7509952087/job/20447737402#step:7:82

@orlitzky
Copy link
Collaborator

Oh, even better. The first time you had to re-push my tag, I assumed it was a permissions issue, and not just a you had to push a tag issue.

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.

3 participants