Skip to content

build/bin/sage-spkg: Add options for checking, cleaning separately from build/install#36738

Merged
vbraun merged 9 commits intosagemath:developfrom
mkoeppe:sage_spkg_split_install
Dec 10, 2023
Merged

build/bin/sage-spkg: Add options for checking, cleaning separately from build/install#36738
vbraun merged 9 commits intosagemath:developfrom
mkoeppe:sage_spkg_split_install

Conversation

@mkoeppe
Copy link
Copy Markdown
Contributor

@mkoeppe mkoeppe commented Nov 19, 2023 •
edited
Loading

The sage-spkg script installs normal and wheel packages.
We create new options that make it possible to invoke the phases of the installation separately.

   -d: only download the package
   -b: build and install (stage) only, do not run post-install
       or check
   -p: post-install only
   -x: exclusively run the test suite; this may assume that:
        * the package has been installed already and/or that
        * the temporary build directory has not been deleted
   -e: erase (delete) the temporary build directory only

This is preparation for improvements in Makefile.in in combination with #36740

📝 Checklist

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

⌛ Dependencies

@orlitzky
Copy link
Copy Markdown
Contributor

orlitzky commented Nov 23, 2023

No major issues but I noticed a few small things that could be improved:

  1. sage-spkg -p doesn't work twice in a row because unload_destdir() removes the staged files itself when it's done. Maybe it could say "Moving package files..." instead of "Copying..."? Regardless, this is mainly confusing because the error message accuses you of deleting the build directory which still exists if you used -s.
  2. The "stage" vs "install" terminology is confusing throughout. There's no hope of fixing this, just mentioning it.
  3. Conflicting options should throw an error. Some combinations do nothing (-d -p, -d -x), while others choose one "randomly" based on whichever case comes first in the script (-p -x, -p -x -e).

@mkoeppe
Copy link
Copy Markdown
Contributor Author

mkoeppe commented Nov 23, 2023

3. Conflicting options should throw an error. Some combinations do nothing (-d -p, -d -x), while others choose one "randomly" based on whichever case comes first in the script (-p -x, -p -x -e).

Improved in d41c032

@mkoeppe
Copy link
Copy Markdown
Contributor Author

mkoeppe commented Nov 23, 2023

  • sage-spkg -p doesn't work twice in a row because unload_destdir() removes the staged files itself when it's done. Maybe it could say "Moving package files..." instead of "Copying..."? Regardless, this is mainly confusing because the error message accuses you of deleting the build directory which still exists if you used -s.

Thanks. I've made improvements in 756ae53.

  • The "stage" vs "install" terminology is confusing throughout. There's no hope of fixing this, just mentioning it.

Indeed. But we can perhaps revisit this after #36747, which unifies handling of "script" and "normal" packages. I also want to clean up the "clean" vs "uninstall" terminology.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Nov 23, 2023

Documentation preview for this PR (built with commit 756ae53; changes) is ready! 🎉

orlitzky
orlitzky approved these changes Nov 24, 2023
View reviewed changes
build/bin/sage-spkg
##################################################################
MODE_SWITCHES=${MODE_SWITCHES#, }
case "$MODE_SWITCHES" in
*,*)
Copy link
Copy Markdown
Contributor

@orlitzky orlitzky Nov 24, 2023

Choose a reason for hiding this comment

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

This makes e.g. -d -d an error, but it's good enough, thanks.

vbraun pushed a commit to vbraun/sage that referenced this pull request Dec 9, 2023
abb1a51 
sagemathgh-36743: Normal Python packages: postpone wheel installation to the post-install phase
    
<!-- ^^^^^
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 -->

Previously, on `sdh_pip_install`, the wheel file is staged in DESTDIR,
but the wheel is installed immediately.
Now we store a new script `spkg-pipinst`, which is run after unloading
DESTDIR (and before any `spkg-postinst` script), which does the actual
installation of the wheel.

- This resolves sagemath#30956 (fixing the wheel URLs shown in `sage -pip
freeze`) -- except when `SAGE_SUDO` is set.

Apart from this and some changes to the messages displayed during
package installation, this should make no difference for any of our
packages.

Just so that it is tested for at least one package in CI, we include a
small package update.

Together with
- sagemath#36738 and
- sagemath#36740,

this is preparation for requiring only the build dependencies ("build-
system requires") while building a wheel for the package, and to require
the runtime dependencies ("install-requires") only later, when the wheel
is to be installed.

<!-- 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#36743
Reported by: Matthias Köppe
Reviewer(s): John H. Palmieri
@vbraun vbraun merged commit 03fc821 into sagemath:develop Dec 10, 2023
vbraun pushed a commit to vbraun/sage that referenced this pull request Dec 13, 2023
6cadd84 
sagemathgh-36747: `build/bin/sage-spkg`: Add support for installing script packages
    
<!-- ^^^^^
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 -->
Split out and updated from sagemath#29386.

The section of `build/make/Makefile.in` that deals with script packages
can be consolidated with the section on normal packages later, in a
follow-up PR after sagemath#36740, sagemath#36738.

We switch the `spkg-install` scripts of the `sage*` script packages to
the templated versions (`spkg-install.in`), which is a simplification.
`spkg-check` remains as is because we still invoke it directly (changing
this will need sagemath#36738).

Resolves sagemath#29386.

<!-- 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: ...
-->
- Depends on sagemath#36737 (for testing)

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36747
Reported by: Matthias Köppe
Reviewer(s): Michael Orlitzky
vbraun pushed a commit to vbraun/sage that referenced this pull request Mar 30, 2024
dd6b9b4 
sagemathgh-37022: `make SPKG-check` for normal and script packages
    
<!-- ^^^^^
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 -->
`make SPKG-check` runs the testsuite of `SPKG` via sage-spkg. This
builds on
- sagemath#36738
- sagemath#36747

<!-- 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. -->

- [ ] The title is concise, informative, and self-explanatory.
- [ ] The description explains in detail what this PR is about.
- [ ] 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#37022
Reported by: Matthias Köppe
Reviewer(s): Kwankyu Lee, Matthias Köppe
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@orlitzky orlitzky orlitzky approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@mkoeppe mkoeppe

Labels

c: build

Projects

None yet

Milestone

sage-10.3

Development

Successfully merging this pull request may close these issues.

3 participants

@mkoeppe @orlitzky @vbraun