sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.44k stars 481 forks source link

Add build/pkgs/SPKG/install-requires.txt for all Python packages, remove some unneeded packages #30719

Closed mkoeppe closed 3 years ago

mkoeppe commented 4 years ago

These files:

CC: @tobiasdiez @isuruf @kiwifb @antonio-rojas @dimpase

Component: build

Author: Matthias Koeppe

Branch/Commit: 6ec00dd

Reviewer: Dima Pasechnik

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

mkoeppe commented 4 years ago

Branch: u/mkoeppe/add_build_pkgs_spkg_install_requires_txt_for_all_python_packages

mkoeppe commented 4 years ago
comment:2

Tobias, before I add more of these, do you think this format is useful for generating other files?


New commits:

a754e8cAdd some build/pkgs/*/install-requires.txt
mkoeppe commented 4 years ago

Commit: a754e8c

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

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

11ce883Add more build/pkgs/*/install-requires.txt
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Changed commit from a754e8c to 11ce883

tobiasdiez commented 4 years ago
comment:4

So the idea would be to concatenate all these install-requires.txt files to obtain the install_requires section of src/setup.cfg? That should work. Except that cython is a more a compile time dependency ([build-system] requires = ["cython"] in pyproject.toml), right?

I have to admit though that I don't see the benefit of having all these text files over having a single src/setup.cfg (or pyproject for that matter). It feels like it's the same information, just in a different location. In fact, it feels a bit like you are reinventing the wheel. pyproject etc also have support for different kinds of dependencies, e.g. optional, compile time (cython), test (pytest) and build dependencies (tox).

Maybe that's a bit too naïve, but I would approach this as follows: sagelib (everything in /src) is a python library and as such only declares minimum dependencies and their requirements, trying not to pin versions (except if it's really necessary). I think, the most modern way would be to do this via a pyproject.toml file. Then sage-the-application (the actual thing used by the enduser) is a python application and thus includes sagelib as a library and needs to satisfy its dependencies (this is the point where you have to specify in some way the exact version you want to use). sage-the library should be completely independent of sage-the-application (i.e. it should work to have them in different git repositories).

mkoeppe commented 4 years ago
comment:5

Replying to @tobiasdiez:

So the idea would be to concatenate all these install-requires.txt files to obtain the install_requires section of src/setup.cfg?

Yes.

That should work. Except that cython is a more a compile time dependency ([build-system] requires = ["cython"] in pyproject.toml), right?

Right; but sagelib also exposes cython at runtime for interactive use.

I have to admit though that I don't see the benefit of having all these text files over having a single src/setup.cfg (or pyproject for that matter). It feels like it's the same information, just in a different location.

With modularization, I expect to see dozens of setup.cfg files for the various subset distributions. Basically, I don't want to duplicate version ranges between these.

In fact, it feels a bit like you are reinventing the wheel. pyproject etc also have support for different kinds of dependencies, e.g. optional, compile time (cython), test (pytest) and build dependencies (tox).

What I want to do is define version ranges in one place for all of the Sage project.

Distinguishing the kinds of dependencies (build, test, ...) should be orthogonal -- and the mechanism in #29041 should be able to put these version ranges into whatever file/format is needed.

(I don't think it would add sufficient value to Sage to have different version ranges for a given package in different roles, like "we need Cython >= 0.29.21 for build, but any Cython >= 0.27 is fine for runtime.)

sage-the library should be completely independent of sage-the-application (i.e. it should work to have them in different git repositories).

I don't like to go into this direction because it would add a big hurdle to our developer community. Having one git repository is key.

tobiasdiez commented 4 years ago
comment:6

Replying to @mkoeppe:

I have to admit though that I don't see the benefit of having all these text files over having a single src/setup.cfg (or pyproject for that matter). It feels like it's the same information, just in a different location.

With modularization, I expect to see dozens of setup.cfg files for the various subset distributions. Basically, I don't want to duplicate version ranges between these.

Sounds like a valid point.

Having one git repository is key.

I agree! What I meant is that it might be helpful to approach it like this, i.e. how would you solve this problem if sage-library would be sitting in a different git repo? I think, this will be helpful e.g for distribution as pipy-installable package where it would be helpful if src/ works independently of the make and build scripts.

One more remark: I would try to make the version range specifications as general as possible, i.e. only add restrictions if its known that that this particular version doesn't work with sage (this in particular applies to the upper limit). Otherwise the sage-library (and its subdistributions) would be harder to use (e.g. if one cannot upgrade/downgrade a dependency because other code relies on it). In this context, it may actually turn out to be helpful if you could could control the dependency versions for each submodule sepereatly, e.g. if ModuleA breaks with a newer version but ModuleB doesn't have these problems.

mkoeppe commented 4 years ago
comment:7

Replying to @tobiasdiez:

What I meant is that it might be helpful to approach it like this, i.e. how would you solve this problem if sage-library would be sitting in a different git repo? I think, this will be helpful e.g for distribution as pipy-installable package where it would be helpful if src/ works independently of the make and build scripts.

Sounds like we are basically in agreement here. One detail, however, is the bootstrapping phase. The way I see it, after running ./bootstrap, we would have standard Python package source trees (which will not depend on make and build) -- right now, src or build/pkgs/sagelib/src/, but many more after modularization (such as build/pkgs/sage-ntl/src/). By using scripts that are executed during bootstrapping, we can avoid duplicating a lot of information and thus reduce the maintenance burden.

mkoeppe commented 4 years ago
comment:8

Replying to @tobiasdiez:

I would try to make the version range specifications as general as possible, i.e. only add restrictions if its known that that this particular version doesn't work with sage (this in particular applies to the upper limit). Otherwise the sage-library (and its subdistributions) would be harder to use (e.g. if one cannot upgrade/downgrade a dependency because other code relies on it).

Yes, this is tricky to get right and will probably need some detailed discussion for individual packages. But I do think that some upper bounds on the version are always needed because we do not want our package to stop working if a new major version of some package is released on pypi. For example, Cython has an upcoming 3.x version which we have not tested with at all.

In this context, it may actually turn out to be helpful if you could could control the dependency versions for each submodule separately, e.g. if ModuleA breaks with a newer version but ModuleB doesn't have these problems.

Yes, this is conceivable but I would hope that if this ever becomes necessary, it can be implemented by overriding the default rather than defaulting to duplicate maintenance of the version ranges.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

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

85b2dcffor a in *; do if [ -d $a -a ! -r $a/requirements.txt -a ! -r $a/install-requires.txt ]; then if grep -q -i sdh_pip $a/spkg-install.in >&/dev/null ; then ( if [ -r $a/package-version.txt ]; then echo "$a >=$(sed s/[.]p[0-9]$// $a/package-version.txt)"; else echo "$a"; fi) > $a/install-requires.txt && git add $a/install-requires.txt; fi; fi; done
f9ac70eAdd install-requires.txt for setuptools, pip
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Changed commit from 11ce883 to f9ac70e

mkoeppe commented 4 years ago
comment:10

Autogenerated for now, setting current version as lower bound

mkoeppe commented 4 years ago
comment:12

I'm hoping to gather version information for our Python packages here.

mkoeppe commented 4 years ago

Author: Matthias Koeppe

mkoeppe commented 4 years ago

Description changed:

--- 
+++ 
@@ -3,6 +3,7 @@
 - serve as a marker that an SPKG is a Python package (for #29013)
 - may include comments that explain why we reject certain versions
 - provide version constraints (lower and upper bounds)
+- as such, are preparation for #29023: Meta-ticket: In a python 3 build, optionally use system Python packages
 - can be used to generate other metadata such as `src/Pipfile`, `src/setup.cfg [install_requires]` (#29041) in the `bootstrap` phase, to avoid duplicating this information in various places
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

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

f6a2c56Merge tag '9.3.beta1' into t/30719/add_build_pkgs_spkg_install_requires_txt_for_all_python_packages
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Changed commit from f9ac70e to f6a2c56

kiwifb commented 4 years ago
comment:17

That's a lot of packages to parse :(

Some stuff I can notice from a cursory inspection. I use sphinx 3.2.1 so the restriction <3.2 seem useless. Similarly I currently use networkx 2.5. python_openid was a sagenb dependency, I don't think we need it anymore. I certainly do not have it anymore on my system. Same with itsdangerous.

tobiasdiez commented 4 years ago
comment:18

The pipenv.lock file added in #30371 is a good indication, too.

The <1.0 constraint for cython seems to be not necessary (at least they are very far off from version 1.0).

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Changed commit from f6a2c56 to ba1d913

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

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

e61929dbuild/pkgs/sphinx/install-requires.txt: Update from gentoo
8fe0e35build/pkgs/python_openid: Unused, remove
ba1d913build/pkgs/itsdangerous: Unused, remove
mkoeppe commented 4 years ago
comment:20

Replying to @tobiasdiez:

The <1.0 constraint for cython seems to be not necessary (at least they are very far off from version 1.0).

From comment 8: For example, Cython has an upcoming 3.x version which we have not tested with at all.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Changed commit from ba1d913 to bb6c4ae

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

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

bb6c4aebuild/pkgs/tox/install-requires.txt: New
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Changed commit from bb6c4ae to d507501

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

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

d507501build/pkgs/networkx/install-requires.txt: Update from gentoo
mkoeppe commented 4 years ago
comment:23

See also: Upgrade ticket #30611

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

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

82d0df3build/pkgs/p_group_cohomology/install-requires.txt: Not on PyPI, remove
d5e9840build/pkgs/pathpy/install-requires.txt: Package removed in #30611, remove
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Changed commit from d507501 to d5e9840

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

c3a9352build/pkgs/pathpy/install-requires.txt: Package removed in #30611, remove
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Changed commit from d5e9840 to c3a9352

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

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

ebd4610build/pkgs/pynac/install-requires.txt: New
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Changed commit from c3a9352 to ebd4610

mkoeppe commented 4 years ago
comment:28

Good enough as a first approximation?

mkoeppe commented 4 years ago
comment:29

Missing: numpy

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

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

6ec00ddbuild/pkgs/{numpy,pillow}/install-requires.txt: New
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Changed commit from ebd4610 to 6ec00dd

dimpase commented 4 years ago
comment:31

OK, good. (all tests of this branch pass on Fedora 30, too)

dimpase commented 4 years ago

Reviewer: Dima Pasechnik

mkoeppe commented 4 years ago
comment:32

Thanks!

vbraun commented 3 years ago

Changed branch from u/mkoeppe/add_build_pkgs_spkg_install_requires_txt_for_all_python_packages to 6ec00dd