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

sagemath-standard: fix manifest #37286

Merged
merged 2 commits into from
Feb 25, 2024
Merged

Conversation

tornaria
Copy link
Contributor

This PR fixes two issues with the manifest for sagemath-standard:

  • src/MANIFEST.in: don't include stuff outside src/sage

The use of global-include <PATTERN> is inconvenient since it will
include any file matching the pattern anywhere; replace it with
recursive-include sage <PATTERN> which is what we really mean.

  • sagemath-standard/setup.py: workaround setuptools_scm

The workaround already there is not good for setuptools_scm >= 8.
Implement a more robust workaround (should work for any version of
setuptools_scm but I have not tested, so I did not remove the old
workaround -- it's harmless to apply both).

📝 Checklist

  • The title is concise, informative, and self-explanatory.
  • The description explains in detail what this PR is about.

Copy link

Documentation preview for this PR (built with commit ac629c5; changes) is ready! 🎉

@tornaria
Copy link
Contributor Author

@mkoeppe I think this is also easy and uncontroversial.

The workaround for setuptools_scm >= 8 is I think much better (and maybe we should also remove the previous workaround, but I'm not sure and it's certainly harmless to apply both).

@mkoeppe
Copy link
Contributor

mkoeppe commented Feb 11, 2024

I'll take a look

@mkoeppe
Copy link
Contributor

mkoeppe commented Feb 11, 2024

In several other setup.py files, we have another workaround that somehow didn't make it into pkgs/sagemath-standard. Take a look at https://github.com/sagemath/sage/blob/develop/pkgs/sage-docbuild/setup.py#L13
for example

@tornaria
Copy link
Contributor Author

In several other setup.py files, we have another workaround that somehow didn't make it into pkgs/sagemath-standard. Take a look at https://github.com/sagemath/sage/blob/develop/pkgs/sage-docbuild/setup.py#L13 for example

The workaround here just replaces with a noop the function walk_revctrl() in setuptools.command.egg_info which is used to list files from scm. The code that uses walk_revctrl() is 19 years old so it seems quite safe (https://github.com/pypa/setuptools/blame/main/setuptools/command/egg_info.py#L591)

The other seems a workaround for a different issue, I don't understand how it works and its implications. The warning "LookupError" seemed harmless.

@mkoeppe
Copy link
Contributor

mkoeppe commented Feb 19, 2024

This seems to work well, thanks!

The next time I touch the setup.py files in the other disitributions, I'll check if some of the other workarounds for setuptools_scm can be replaced by this.

@vbraun vbraun merged commit 4988157 into sagemath:develop Feb 25, 2024
27 checks passed
@mkoeppe mkoeppe added this to the sage-10.3 milestone Mar 7, 2024
@kiwifb
Copy link
Member

kiwifb commented Apr 15, 2024

Hi guys, this is the only ticket included in 10.4.beta4 that I think may be related to what I just found. When I create a sdist for sagemath-standard-10.4.beta3 there are no python files (.py) in the sage folder. Only cython files. The only python files present in the sdist are at the top level (setup.py) and inside bin (sage-num-threads.py and sage-startuptime.py).

Do you have any ideas?

@mkoeppe
Copy link
Contributor

mkoeppe commented Apr 15, 2024

Indeed the 10.4.beta3 sdist (also on PyPI) is broken in this way. I'll investigate.

@tornaria
Copy link
Contributor Author

How do you create the sdist? Is the repo bootstrapped? What version of setuptools?

Can you try reverting each one of the two changes by hand to see which one, if any, is the culprit?

@tornaria tornaria deleted the fix-manifest branch April 15, 2024 02:45
@mkoeppe
Copy link
Contributor

mkoeppe commented Apr 15, 2024

This change probably comes from #36951 or #36964, not here.

@mkoeppe
Copy link
Contributor

mkoeppe commented Apr 15, 2024

@kiwifb Please review #37434, which adds the missing automatic test for the sagemath-standard distribution built out of pkgs/sagemath-standard....

@kiwifb
Copy link
Member

kiwifb commented Apr 15, 2024

I will investigate further the origin, I have my doubts about #36951 but we never know. There could be something in #36964 but that will be difficult to pull out.

@mkoeppe
Copy link
Contributor

mkoeppe commented Apr 15, 2024

I have a simple fix in #37804

@kiwifb
Copy link
Member

kiwifb commented Apr 15, 2024

Looks like something in #36964 is at fault.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants