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

configure --enable-system-site-packages via spkg-configure.m4 for cvxopt and other python packages #29665

Closed
thierry-FreeBSD opened this issue May 8, 2020 · 170 comments · Fixed by #36141

Comments

@thierry-FreeBSD
Copy link

This ticket adds an spkg-configure.m4 for cvxopt and various other python packages, to allow them to be used from the system (part of #27330).

These python packages are special because we currently use a python "venv" that is isolated from the system. Thus this ticket proceeds in several steps:

  1. Add an --enable-system-site-packages flag to the top-level ./configure script. When present, this will tell sage-venv that the venv it creates should not be isolated from the system.
  2. Create a new macro that can detect python packages on the system. It should ensure that the version bounds in each python package's install-requires.txt are met.
  3. Create the spkg-configure.m4 files themselves, which should be relatively simple at that point.
  4. Test everything thoroughly with various combinations of system and non-system python packages.

Platform-specific testing:

  • for example TARGETS_PRE="build-local" EXTRA_CONFIGURE_ARGS="--enable-system-site-packages" tox -e docker-ubuntu-focal-standard -- build ptest

Depends on #32703
Depends on #33067

Component: build: configure

Keywords: cvxopt, system packages, Python library

Author: Michael Orlitzky

Branch/Commit: u/mjo/ticket/29665 @ baf6301

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

@thierry-FreeBSD
Copy link
Author

spkg-configure.m4 to be copied under build/pkgs/cvxopt

@mkoeppe
Copy link
Contributor

mkoeppe commented May 8, 2020

comment:1

Attachment: spkg-configure.m4.gz

I don't see a branch

@mkoeppe
Copy link
Contributor

mkoeppe commented May 8, 2020

comment:2

See comments in #29023

@thierry-FreeBSD
Copy link
Author

@mkoeppe
Copy link
Contributor

mkoeppe commented Aug 11, 2020

comment:4

"wontfix" because of #29023


New commits:

ac66801Experimental spkg-configure.m4 for cvxopt

@mkoeppe
Copy link
Contributor

mkoeppe commented Aug 11, 2020

Commit: ac66801

@mkoeppe
Copy link
Contributor

mkoeppe commented Aug 11, 2020

Changed author from gh-thierry-FreeBSD to none

@mkoeppe mkoeppe removed this from the sage-9.2 milestone Aug 11, 2020
@dimpase
Copy link
Member

dimpase commented Aug 13, 2020

comment:5

There is per se nothing wrong with having spkg-configure.m4 for Python packages.

@mkoeppe
Copy link
Contributor

mkoeppe commented Aug 13, 2020

comment:6

Not per se, only per the big picture.

@orlitzky
Copy link
Contributor

comment:7

Regardless of what we eventually wind up with... does this work? Does it hurt anything? The solution in #29023 could be years away. If we can avoid building e.g. numpy, scipy, matplotlib, and cvxopt with relatively trivial spkg-configures until then, why not do it?

@mkoeppe
Copy link
Contributor

mkoeppe commented Sep 18, 2021

comment:8

No, it does not work.

@orlitzky
Copy link
Contributor

comment:9

Replying to @mkoeppe:

No, it does not work.

What goes wrong? I just did a quick test (without this branch):

  1. make cvxopt-clean
  2. install system cvxopt
  3. touch any file in sagelib names *cvxopt*
  4. sage -b
  5. run some tests on said cvxopt files

Behavior is unchanged as far as I can tell.

@mkoeppe
Copy link
Contributor

mkoeppe commented Sep 19, 2021

comment:10

Sound like you forgot to test with --optional=sage,cvxopt.

Sage runs in a venv that does not provide access to system site packages.

@orlitzky
Copy link
Contributor

comment:11

Replying to @mkoeppe:

Sound like you forgot to test with --optional=sage,cvxopt.

Sage runs in a venv that does not provide access to system site packages.

Lucky guess! I forgot we made it possible to disable cvxopt even though it's type=standard. On the surface, setting include-system-site-packages = true in local/pyvenv.cfg looks like it solves that, and is one of the first steps mentioned on #29023. What necessitates the next step B.2. in #29023? In other words, what's the next thing that's going to go wrong for me?

@mkoeppe
Copy link
Contributor

mkoeppe commented Sep 19, 2021

comment:12

Let's take the discussion of #29023 to #29023. I think it would be fine if you want to go ahead with step B.1 (via a new option ./configure --enable-system-site-packages) in a ticket, for users to experiment.

@orlitzky
Copy link
Contributor

Changed commit from ac66801 to faed999

@orlitzky
Copy link
Contributor

comment:13

Matthias, does something like this fit with your master plan?


Last 10 new commits:

17860f7Trac #29665: spkg-configure.m4 for requests.
67db9efTrac #29665: spkg-configure.m4 for six.
ef80d1aTrac #29665: spkg-configure.m4 for packaging.
310e56bTrac #29665: spkg-configure.m4 for numpy.
8d6a740Trac #29665: spkg-configure.m4 for scipy.
fe16a90Trac #29665: spkg-configure.m4 for pluggy.
57cb9beTrac #29665: spkg-configure.m4 for toml.
f976afaTrac #29665: spkg-configure.m4 for pickleshare.
4137218Trac #29665: spkg-configure.m4 for html5lib.
faed999Trac #29665: spkg-configure.m4 for scandir.

@orlitzky
Copy link
Contributor

@mkoeppe
Copy link
Contributor

mkoeppe commented Oct 11, 2021

comment:14

I wouldn't object to doing something like this, as it is clearly marked experimental.

However, be prepared for a long-term burden of maintenance regarding the declared version ranges. Lots of things can go wrong because the individual packages often have complicated version constraints.

Also the version constraints shouldn't be hardcoded in the spkg-configure.m4 file; instead, they should really be taken from install-requires.txt.
Likewise, don't use distutils, it's on its way out. Use a higher-level library that can check the requirements

Also, note scandir is gone after #32626

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 12, 2021

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

b7a0dcbTrac #29665: new spkg-configure.m4 for pillow.
b4252edTrac #29665: standard python spkg-configure.m4 for mpmath.
1b20db3Trac #29665: standard python spkg-configure.m4 for sympy.
37c7732Trac #29665: standard python spkg-configure.m4 for pygments.
c0dce08Trac #29665: standard python spkg-configure.m4 for jinja2.
492471aTrac #29665: standard python spkg-configure.m4 for pandocfilters.
9d70ed5Trac #29665: standard python spkg-configure.m4 for webencodings.
71cc574Trac #29665: standard python spkg-configure.m4 for pytz.
29e95b2Trac #29665: standard python spkg-configure.m4 for texttable.
6a5246dTrac #29665: standard python spkg-configure.m4 for babel.

orlitzky added a commit to orlitzky/sage that referenced this issue Aug 26, 2023
orlitzky added a commit to orlitzky/sage that referenced this issue Aug 26, 2023
orlitzky added a commit to orlitzky/sage that referenced this issue Aug 26, 2023
orlitzky added a commit to orlitzky/sage that referenced this issue Aug 26, 2023
orlitzky added a commit to orlitzky/sage that referenced this issue Aug 26, 2023
orlitzky added a commit to orlitzky/sage that referenced this issue Aug 26, 2023
orlitzky added a commit to orlitzky/sage that referenced this issue Aug 26, 2023
orlitzky added a commit to orlitzky/sage that referenced this issue Aug 26, 2023
orlitzky added a commit to orlitzky/sage that referenced this issue Aug 26, 2023
orlitzky added a commit to orlitzky/sage that referenced this issue Aug 26, 2023
orlitzky added a commit to orlitzky/sage that referenced this issue Aug 26, 2023
orlitzky added a commit to orlitzky/sage that referenced this issue Aug 26, 2023
orlitzky added a commit to orlitzky/sage that referenced this issue Aug 26, 2023
orlitzky added a commit to orlitzky/sage that referenced this issue Aug 26, 2023
@orlitzky
Copy link
Contributor

what's needing work here, besides rebasing?

Nothing, and I've rebased it at #36141

There's one test failure due to a deprecation warning in ipython somewhere, but I don't want to spend too much time chasing down minor test failures again. This is opt-in and I think it's a better use of time to get it merged and then deal with the issues rather than try to make it perfect before merging, given how many commits there are and how easily everything goes out of date.

vbraun pushed a commit to vbraun/sage that referenced this issue Sep 4, 2023
…e-packages

    
Rebased branch of sagemath#29665

Platform-specific testing:
- for example TARGETS_PRE="build-local" tox -e docker-ubuntu-focal-
standard-sitepackages -- build ptest

Fixes sagemath#29665
    
URL: sagemath#36141
Reported by: Michael Orlitzky
Reviewer(s): Dima Pasechnik, Matthias Köppe, Michael Orlitzky
vbraun pushed a commit to vbraun/sage that referenced this issue Sep 5, 2023
…e-packages

    
Rebased branch of sagemath#29665

Platform-specific testing:
- for example TARGETS_PRE="build-local" tox -e docker-ubuntu-focal-
standard-sitepackages -- build ptest

Fixes sagemath#29665
    
URL: sagemath#36141
Reported by: Michael Orlitzky
Reviewer(s): Dima Pasechnik, Matthias Köppe, Michael Orlitzky
vbraun pushed a commit to vbraun/sage that referenced this issue Sep 10, 2023
…e-packages

    
Rebased branch of sagemath#29665

Platform-specific testing:
- for example TARGETS_PRE="build-local" tox -e docker-ubuntu-focal-
standard-sitepackages -- build ptest

Fixes sagemath#29665
    
URL: sagemath#36141
Reported by: Michael Orlitzky
Reviewer(s): Dima Pasechnik, Matthias Köppe, Michael Orlitzky
@mkoeppe mkoeppe added this to the sage-10.2 milestone Sep 10, 2023
vbraun pushed a commit to vbraun/sage that referenced this issue Jan 2, 2024
sagemath#29665

    
Currently broken (since sagemath#29665):
```
$ build/bin/sage-get-system-packages install-requires zipp
$ build/bin/sage-get-system-packages install-requires-toml zipp
```
This causes `pkgs/sagemath-standard/pyproject.toml` to be broken; as
noted in
sagemath#36964 (comment)

The correct output:
```
$  build/bin/sage-get-system-packages install-requires zipp
zipp >=0.5.2
$ build/bin/sage-get-system-packages install-requires-toml zipp
'zipp >=0.5.2',
```

<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->

<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36979
Reported by: Matthias Köppe
Reviewer(s): Michael Orlitzky
vbraun pushed a commit to vbraun/sage that referenced this issue Jan 5, 2024
sagemath#29665

    
Currently broken (since sagemath#29665):
```
$ build/bin/sage-get-system-packages install-requires zipp
$ build/bin/sage-get-system-packages install-requires-toml zipp
```
This causes `pkgs/sagemath-standard/pyproject.toml` to be broken; as
noted in
sagemath#36964 (comment)

The correct output:
```
$  build/bin/sage-get-system-packages install-requires zipp
zipp >=0.5.2
$ build/bin/sage-get-system-packages install-requires-toml zipp
'zipp >=0.5.2',
```

<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->

<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36979
Reported by: Matthias Köppe
Reviewer(s): Michael Orlitzky
vbraun pushed a commit to vbraun/sage that referenced this issue Jan 13, 2024
sagemath#29665

    
Currently broken (since sagemath#29665):
```
$ build/bin/sage-get-system-packages install-requires zipp
$ build/bin/sage-get-system-packages install-requires-toml zipp
```
This causes `pkgs/sagemath-standard/pyproject.toml` to be broken; as
noted in
sagemath#36964 (comment)

The correct output:
```
$  build/bin/sage-get-system-packages install-requires zipp
zipp >=0.5.2
$ build/bin/sage-get-system-packages install-requires-toml zipp
'zipp >=0.5.2',
```

<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->

<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36979
Reported by: Matthias Köppe
Reviewer(s): Michael Orlitzky
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants