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

Remove direct use of "setup.py sdist", add targets "make SPKG-sdist" #35104

Merged

Conversation

mkoeppe
Copy link
Member

@mkoeppe mkoeppe commented Feb 13, 2023

πŸ“š Description

Fixes #34855

πŸ“ Checklist

  • I have made sure that the title is self-explanatory and the description concisely explains the PR.
  • I have linked an issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

βŒ› Dependencies

@codecov-commenter
Copy link

codecov-commenter commented Feb 13, 2023

Codecov Report

Base: 88.58% // Head: 88.59% // Increases project coverage by +0.00% πŸŽ‰

Coverage data is based on head (c0f6348) compared to base (293dd72).
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff            @@
##           develop   #35104    +/-   ##
=========================================
  Coverage    88.58%   88.59%            
=========================================
  Files         2140     2140            
  Lines       396961   397273   +312     
=========================================
+ Hits        351655   351953   +298     
- Misses       45306    45320    +14     
Impacted Files Coverage Ξ”
src/sage/groups/affine_gps/euclidean_group.py 88.88% <0.00%> (-11.12%) ⬇️
src/sage/quadratic_forms/random_quadraticform.py 90.00% <0.00%> (-6.00%) ⬇️
src/sage/plot/histogram.py 96.77% <0.00%> (-3.23%) ⬇️
src/sage/groups/affine_gps/affine_group.py 96.62% <0.00%> (-2.25%) ⬇️
src/sage/cpython/_py2_random.py 74.79% <0.00%> (-1.24%) ⬇️
src/sage/modular/hecke/algebra.py 94.65% <0.00%> (-1.07%) ⬇️
src/sage/modular/quasimodform/element.py 99.14% <0.00%> (-0.86%) ⬇️
src/sage/modular/modform/numerical.py 94.19% <0.00%> (-0.65%) ⬇️
src/sage/combinat/colored_permutations.py 95.46% <0.00%> (-0.60%) ⬇️
src/sage/rings/polynomial/ore_polynomial_ring.py 89.40% <0.00%> (-0.43%) ⬇️
... and 41 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

β˜” View full report at Codecov.
πŸ“’ Do you have feedback about the report comment? Let us know in this issue.

@mkoeppe mkoeppe self-assigned this Feb 18, 2023
@github-actions
Copy link

Documentation preview for this PR is ready! πŸŽ‰
Built with commit: c0f6348

@mkoeppe mkoeppe requested a review from dimpase February 26, 2023 20:39
@jhpalmieri
Copy link
Member

On a Mac, it looks like the current version of make pypi-sdists is broken: it doesn't produce tarballs for all nine of the listed packages, creating 5 of them and then "UNKNOWN-0.0.0.tar.gz". With the changes here, it looks like it actually works.

I don't know the details of the python3 -m build --sdist ... command. Can we remove the various setup.py files, or are they still used?

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 15, 2023

I think the UNKNOWN-... is a symptom of not running ./bootstrap. Related:

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 15, 2023

I don't know the details of the python3 -m build --sdist ... command. Can we remove the various setup.py files, or are they still used?

No, it still goes through setuptools, so setup.py is still needed. But setuptools has deprecated its entire command-line interface (and issues deprecation warnings); going forward, it will only provide a build backend (this goes by "PEP-517"). build is a new frontend for PEP-517 builds that uses the build backend declared in pyproject.toml; in our case, setuptools.

@jhpalmieri
Copy link
Member

Re #35268, running ./bootstrap fixes the problem. This is with a brand new tarball so I think that shouldn't be necessary, which presumably is what #35268 is about.

Copy link
Member

@jhpalmieri jhpalmieri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, works in my limited testing.

@jhpalmieri
Copy link
Member

jhpalmieri commented Mar 15, 2023

I wish I knew a way to list all make targets so that I could see very directly what targets were added by this change. I found https://stackoverflow.com/questions/4219255/how-do-you-get-the-list-of-targets-in-a-makefile, but for example the solution there by Marc.2377 only lists some of the targets, I'm guessing those created by the top-level Makefile.

@jhpalmieri
Copy link
Member

jhpalmieri commented Mar 15, 2023

I have a clunky way (make -pn and searching the output) of listing the make targets, and I can see lots of new ones that maybe shouldn't be there, like texlive-sdist. Would it be easy to only create those targets for the packages for which build/pkgs/SPKG/spkg-src exists?

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 15, 2023

Would it be easy to only create those targets for the packages for which build/pkgs/SPKG/spkg-src exists?

In our system, that would add more code in ./bootstrap and in m4/ and a new macro in build/make/Makefile.in; I think we shouldn't do that.

Overall, I don't think we should think of the targets of make as public API. Some certainly are, and we should probably document them (see for example #35276). But most are just implementation details.

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 16, 2023

Thanks for reviewing!

@vbraun vbraun merged commit df5d71a into sagemath:develop Mar 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove direct use of "setup.py sdist", add targets "make SPKG-sdist"
4 participants