Reimplement "sage -p SPKG" as "cd $SAGE_ROOT && make SPKG-no-deps"
Closed this issue · 55 comments
This option installs a package without its dependencies, enabling experiments by experienced developers.
After #29013 (Support installation of Python packages into separate venvs depending on the python version), sage-spkg requires an installation-tree argument. As a result, currently sage -p is broken, as reported in #32751.
We remove this direct use of sage-spkg.
Making this change also removes the last use of SAGE_LOGS from src/
Since this now goes through a make target instead of using sage-spkg directly, after switching to a branch that adds a package, developers may need to manually invoke bootstrap and configure before using this option.
Depends on #33127
CC: @jhpalmieri @orlitzky @kliem @vbraun
Component: build
Author: Matthias Koeppe, John Palmieri
Branch/Commit: 9ed0d60
Reviewer: John Palmieri, Matthias Koeppe
Issue created by migration from https://trac.sagemath.org/ticket/30649
Author: Matthias Koeppe
Description changed:
---
+++
@@ -1,3 +1,9 @@
-Making this change will allow us to remove the last use of `SAGE_LOGS` from `src/`.
+This option installs a package without its dependencies, enabling experiments by experienced developers.
+
+Making this change
+- removes the last use of `SAGE_LOGS` from `src/`
+- is preparation for #29013 (Support installation of Python packages into separate venvs depending on the python version)
+
+Since this now goes through a `make` target instead of using `sage-spkg` directly, after switching to a branch that adds a package, developers may need to manually invoke `bootstrap` and `configure` before using this option.
I think that this does not behave according to the documentation: "Options are the same as for the -i command." For example ./sage -p -c bzip2 does not run the test suite, and ./sage -p -s biopython does not preserve the build directory.
You are right, this needs more thought
Setting new milestone based on a cursory review of ticket status, priority, and last modification date.
Setting a new milestone for this ticket based on a cursory review.
Description changed:
---
+++
@@ -1,8 +1,9 @@
This option installs a package without its dependencies, enabling experiments by experienced developers.
+
+After #29013 (Support installation of Python packages into separate venvs depending on the python version), `sage-spkg` requires an installation-tree argument. As a result, currently `sage -p` is broken, as reported in #32751.
Making this change
- removes the last use of `SAGE_LOGS` from `src/`
-- is preparation for #29013 (Support installation of Python packages into separate venvs depending on the python version)
Since this now goes through a `make` target instead of using `sage-spkg` directly, after switching to a branch that adds a package, developers may need to manually invoke `bootstrap` and `configure` before using this option.
Description changed:
---
+++
@@ -2,8 +2,9 @@
After #29013 (Support installation of Python packages into separate venvs depending on the python version), `sage-spkg` requires an installation-tree argument. As a result, currently `sage -p` is broken, as reported in #32751.
-Making this change
-- removes the last use of `SAGE_LOGS` from `src/`
+We remove this direct use of `sage-spkg`.
+
+Making this change also removes the last use of `SAGE_LOGS` from `src/`
Since this now goes through a `make` target instead of using `sage-spkg` directly, after switching to a branch that adds a package, developers may need to manually invoke `bootstrap` and `configure` before using this option.
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
bf40b1d | src/bin/sage (-p): Use build/make/install SPKG-no-deps |
Branch pushed to git repo; I updated commit sha1. New commits:
c9aa68c | src/bin/sage (-p): Use 'make' to install the package and pass on INSTALL_OPTIONS |
2d852ad | build/bin/sage-dist-helpers (sdh_pip_uninstall): Use build/bin/sage-pip-uninstall, resurrected in simplified form |
b5a89ad | build/bin/sage-dist-helpers (sdh_store_and_pip_install_wheel): Uninstall before installing, to ensure the wheel is installed even if the version is the same |
30a272e | build/pkgs/sage_setup/dependencies: Add interpreter specs as dependencies |
d435781 | build/bin/sage-dist-helpers (sdh_store_and_pip_install_wheel): Use sage-pip-uninstall |
11c47d8 | Merge #32659 |
The code looks simple and it works for me. ./sage -p -s ... and ./sage -p -c ... work. One possible issue I can see is the one mentioned in the description: users may need to use ./bootstrap and/or ./configure first. But I think this is okay.
One real issue: src/sage/tests/cmdline.py fails doctests. I think the test is kind of stupid, though (the second one):
Test ``sage --info [packages]`` and the equivalent
``sage -p --info --info [packages]`` (the doubling of ``--info``
is intentional, that option should be idempotent)::
Why do we care whether ./sage -p --info --info PKG works? ./sage --info --info PKG doesn't work, so why should we care whether it works if we insert -p?
Branch pushed to git repo; I updated commit sha1. New commits:
59b576e | Merge tag '9.5.beta5' into t/30649/reimplement__sage__p_spkg__as__cd__sage_root_build_make____install_spkg_no_deps_ |
a320416 | src/bin/sage: Unify code for -i and -p |
6eba164 | build/bin/sage-site: Move handling of -i, -f, -p here from src/bin/sage |
140d1c0 | build/bin/sage-spkg: Do not depend on sage-env for printing usage |
60a2d99 | build/bin/sage-site: Do not depend on PATH being set for running --info |
efaadfa | build/bin/sage-site: On -p, also do not run then final 'make all-build' |
I've cleaned this up a bit more, it's now all in build/bin/sage-site.
Needs a bit more work though
Maybe this will resolve #22062: currently sage -p --info sqlite writes info to a log file as well as printing on the screen, and I think it should just print to the screen.
What work needs to be done here? This does indeed resolve #22062 because it disables the use of sage -p --arg.
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
7e11ede | src/bin/sage (-p): Use build/make/install SPKG-no-deps |
d90eeae | src/bin/sage (-p): Use 'make' to install the package and pass on INSTALL_OPTIONS |
fd4ffee | src/bin/sage: Unify code for -i and -p |
7e8c0fa | build/bin/sage-site: Move handling of -i, -f, -p here from src/bin/sage |
3325107 | build/bin/sage-spkg: Do not depend on sage-env for printing usage |
1b796bd | build/bin/sage-site: Do not depend on PATH being set for running --info |
0aa7233 | build/bin/sage-site: On -p, also do not run then final 'make all-build' |
Rebased on current beta.
I unfortunately don't recall what I meant in comment:19
Branch pushed to git repo; I updated commit sha1. New commits:
ac9f302 | src/bin/sage, src/bin/sage-env: Treat SAGE_ROOT = nonexisting directory like SAGE_ROOT unset |
5ee6fba | src/bin/sage-env: Suppress warning 'overwriting SAGE_ROOT' for empty NEW_SAGE_ROOT |
5b6e7f8 | Merge #33127 |
d42fabe | src/bin/sage: Another 'test -d SAGE_ROOT' instead of 'test -n SAGE_ROOT |
A (stupid) doctest fails, and we could just delete it:
diff --git a/src/sage/tests/cmdline.py b/src/sage/tests/cmdline.py
index 681ff703e2..fccba3f12f 100644
--- a/src/sage/tests/cmdline.py
+++ b/src/sage/tests/cmdline.py
@@ -210,9 +210,7 @@ def test_executable(args, input="", timeout=100.0, pydebug_ignore_warnings=False
sage: ret # optional - sage_spkg
0
- Test ``sage --info [packages]`` and the equivalent
- ``sage -p --info --info [packages]`` (the doubling of ``--info``
- is intentional, that option should be idempotent)::
+ Test ``sage --info [packages]``::
sage: out, err, ret = test_executable(["sage", "--info", "sqlite"]) # optional - sage_spkg
sage: print(out) # optional - sage_spkg
@@ -225,17 +223,6 @@ def test_executable(args, input="", timeout=100.0, pydebug_ignore_warnings=False
sage: ret # optional - sage_spkg
0
- sage: out, err, ret = test_executable(["sage", "-p", "--info", "--info", "sqlite"]) # optional - sage_spkg
- sage: print(out) # optional - sage_spkg
- sqlite...
- SQLite is a software library that implements a self-contained,
- serverless, zero-configuration, transactional SQL database engine.
- ...
- sage: err # optional - sage_spkg
- ''
- sage: ret # optional - sage_spkg
- 0
-
Test ``sage-run`` on a Python file, both with an absolute and with a relative path::
sage: dir = tmp_dir(); name = 'python_test_file.py'Unless there is some script somewhere that routinely produces commands like sage --info --info ...?
Yes, I'm in favor of deleting this nonsensical test
Changed author from Matthias Koeppe to Matthias Koeppe, John Palmieri
Any ideas on what else needs to be done with this?
New commits:
33d3d1c | src/bin/sage-env: Merge code that sets SAGE_ROOT |
fdada1c | src/bin/sage-env: Update comments on SAGE_ENV_SOURCED |
7828a79 | src/bin/sage-env: Fixup: Move setting of SAGE_ROOT out of the inner-most if-block |
2bd0742 | src/bin/sage-env: Suppress error message from probing for NEW_SAGE_ROOT |
702b779 | src/bin/sage-env: Do not set SAGE_ROOT to empty; sage-location does not like it |
c9fe2ed | Merge #33127 |
I think it is ready
The change from comment:30 and comment:31 seems to have been lost, and I am getting an error when I try to push. I don't want to mess up the rest of the branch, so it might be better if you do it:
diff --git a/src/sage/tests/cmdline.py b/src/sage/tests/cmdline.py
index 681ff703e25..fccba3f12fb 100644
--- a/src/sage/tests/cmdline.py
+++ b/src/sage/tests/cmdline.py
@@ -210,9 +210,7 @@ def test_executable(args, input="", timeout=100.0, pydebug_ignore_warnings=False
sage: ret # optional - sage_spkg
0
- Test ``sage --info [packages]`` and the equivalent
- ``sage -p --info --info [packages]`` (the doubling of ``--info``
- is intentional, that option should be idempotent)::
+ Test ``sage --info [packages]``::
sage: out, err, ret = test_executable(["sage", "--info", "sqlite"]) # optional - sage_spkg
sage: print(out) # optional - sage_spkg
@@ -225,17 +223,6 @@ def test_executable(args, input="", timeout=100.0, pydebug_ignore_warnings=False
sage: ret # optional - sage_spkg
0
- sage: out, err, ret = test_executable(["sage", "-p", "--info", "--info", "sqlite"]) # optional - sage_spkg
- sage: print(out) # optional - sage_spkg
- sqlite...
- SQLite is a software library that implements a self-contained,
- serverless, zero-configuration, transactional SQL database engine.
- ...
- sage: err # optional - sage_spkg
- ''
- sage: ret # optional - sage_spkg
- 0
-
Test ``sage-run`` on a Python file, both with an absolute and with a relative path::
sage: dir = tmp_dir(); name = 'python_test_file.py'Branch pushed to git repo; I updated commit sha1. New commits:
65989cc | trac 30649: remove "sage -p --info --info ..." test |
Sorry for dropping this! Here it is
I propose the following two changes (the first one because this is already in sage-site and the option is handled twenty lines later, the second is just a typo not related to the changes here but one that you see when you run ./sage -p):
diff --git a/build/bin/sage-site b/build/bin/sage-site
index 78848c389e..f7164503b7 100755
--- a/build/bin/sage-site
+++ b/build/bin/sage-site
@@ -129,7 +129,7 @@ if [ "$1" = '-standard' -o "$1" = "--standard" ]; then
fi
if [ "$1" = '-p' ]; then
- # If there are no further arguments, display usage help (via an option handled by sage-site)
+ # If there are no further arguments, display usage help
if [ $# -eq 1 ]; then
set -- --info
else
diff --git a/build/bin/sage-spkg b/build/bin/sage-spkg
index 1af55f6671..4478bc7f2d 100755
--- a/build/bin/sage-spkg
+++ b/build/bin/sage-spkg
@@ -99,7 +99,7 @@ Options:
-y: automatically reply "y" for all prompts regarding
experimental and old-style packages; warning: there
is no guarantee that these packages will build correctly;
- use at your own risk"
+ use at your own risk
-n: automatically reply "n" for all prompts regarding
experimental and old-style packages
-o: allow fetching the package from its upstream URLAlso for a future ticket: should ./sage -i and ./sage -p (with no further arguments) behave similarly? ./sage -i builds some stuff, while ./sage -p just prints a usage message and exits.
Branch pushed to git repo; I updated commit sha1. New commits:
9ed0d60 | build/bin/sage-site: Edits to comments, help text |
Replying to @jhpalmieri:
should
./sage -iand./sage -p(with no further arguments) behave similarly?./sage -ibuilds some stuff, while./sage -pjust prints a usage message and exits.
I suppose sage -p could be reduced to just saying "Nothing to do." instead of showing usage.
And it's not clear if sage -i running make all-build is really useful, so we could consider changing this behavior too.
But as you say, that would be a future ticket.
Reviewer: ..., Matthias Koeppe
Changed reviewer from ..., Matthias Koeppe to John Palmieri, Matthias Koeppe
Looks good to me. It has some advantages that I didn't expect. Without this branch:
% ./sage -p pytest
Error: Installing old-style SPKGs is no longer supported
whereas with this branch, ./sage -p pytest works fine (although it says "Successfully installed iniconfig-1.1.1 pytest-7.0.1" so it is still doing some dependency checking — probably pip or similar, not Sage's build system).
Thanks!