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

Fix sagemath-standard sdist #34858

Closed
mkoeppe opened this issue Dec 18, 2022 · 17 comments
Closed

Fix sagemath-standard sdist #34858

mkoeppe opened this issue Dec 18, 2022 · 17 comments

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Dec 18, 2022

./bootstrap && ./sage -sh -c '(cd pkgs/sagemath-standard && tox -v -v -v -e sagepython-sagewheels-nopypi-norequirements)' fails in 9.8.beta5 because the sdist is broken


  cimport cython
  from sage.algebras.fusion_rings.poly_tup_engine cimport (
  ^
  ------------------------------------------------------------

  sage/algebras/fusion_rings/fast_parallel_fmats_methods.pyx:12:0: 'sage/algebras/fusion_rings/poly_tup_engine.pxd' not found

We fix this by

  • updating MANIFEST
  • unconditionally running the module finder in pkgs/sagemath-standard/setup.py

We make the same change also in pkgs/sagemath-objects/setup.py (by symlink, this also affects sagemath-categories).

Depends on #34859

CC: @kwankyu @kiwifb

Component: distribution

Author: Matthias Koeppe

Branch/Commit: e229436

Reviewer: François Bissey

Issue created by migration from https://trac.sagemath.org/ticket/34858

@mkoeppe mkoeppe added this to the sage-9.8 milestone Dec 18, 2022
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 18, 2022

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 18, 2022

Commit: 00deebe

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 18, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

00deebepkgs/sagemath-objects/setup.py: Run finder also for sdist, egg_info, dist_info

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 19, 2022

Author: Matthias Koeppe

@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 19, 2022

Dependencies: #34859

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 19, 2022

Changed commit from 00deebe to e229436

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 19, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

39bd748src/sage/dynamics/arithmetic_dynamics/projective_ds.py: Remove unused import of typing_extensions
e229436Merge #34859

@kiwifb
Copy link
Member

kiwifb commented Dec 20, 2022

comment:6

So no more sdist specific code in setup.py so simplification and make sure stuff is properly included in the manifest.

I am wondering if it is possible to first use a global include and then prune some of the files that are collected by that global include. I am just thinking we should explicitly list as few files as we can. It is a risk, because someone will forget to include them in the manifest when they create a new one - or delete one.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 20, 2022

comment:7

Yes, that's what the MANIFEST.in is doing. We do have to distinguish between the Cython-generated C/C++ files and actual C/C++ source files. Adding new such files is a relatively rare event

@kiwifb
Copy link
Member

kiwifb commented Dec 20, 2022

comment:8

I am very much in automation these days. While it is relatively short, maintaining this list by hand is a hazard. Which is why this ticket add 3 explicit files to MANIFEST.in. I am going to fill that under ideas to develop.

For the rest it looks good to me but I see #34859 has not been reviewed yet.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 20, 2022

comment:9

I hope to switch from setuptools to meson-python in #34630; then we can revisit this question.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 23, 2022

comment:10

Replying to François Bissey:

I see #34859 has not been reviewed yet.

Now the dependency has been reviewed

@kiwifb
Copy link
Member

kiwifb commented Dec 23, 2022

comment:11

Let's move on then.

@kiwifb
Copy link
Member

kiwifb commented Dec 23, 2022

Reviewer: François Bissey

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 24, 2022

comment:12

Thanks!

@vbraun
Copy link
Member

vbraun commented Jan 21, 2023

Changed branch from u/mkoeppe/fix_sagemath_standard_sdist to e229436

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

No branches or pull requests

3 participants