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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 9 commits into from
Dec 10, 2023
Merged

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

merged 9 commits into from
Dec 10, 2023

Conversation

mkoeppe
Copy link
Member

@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

@mkoeppe mkoeppe self-assigned this Nov 19, 2023
@mkoeppe mkoeppe force-pushed the sage_spkg_split_install branch 2 times, most recently from 03d7063 to 1a36540 Compare November 19, 2023 22:04
This was referenced Nov 20, 2023
Open
Merged
Merged
@orlitzky
Copy link
Contributor

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
Member 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
Member 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 GitHub Actions
Copy link

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
Contributor

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.

@mkoeppe mkoeppe mentioned this pull request Nov 25, 2023
Draft
5 tasks
vbraun pushed a commit to vbraun/sage that referenced this pull request Dec 9, 2023
sagemathgh-36743: Normal Python packages: postpone wheel installation…
abb1a51 
… 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
20 checks passed
@mkoeppe mkoeppe added this to the sage-10.3 milestone Dec 10, 2023
vbraun pushed a commit to vbraun/sage that referenced this pull request Dec 13, 2023
sagemathgh-36747: build/bin/sage-spkg: Add support for installing s…
6cadd84 
…cript 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
@mkoeppe mkoeppe mentioned this pull request Dec 31, 2023
Open
@mkoeppe mkoeppe mentioned this pull request Jan 7, 2024
Merged
5 tasks
@mkoeppe mkoeppe deleted the sage_spkg_split_install branch March 3, 2024 07:10
vbraun pushed a commit to vbraun/sage that referenced this pull request Mar 30, 2024
sagemathgh-37022: make SPKG-check for normal and script packages
dd6b9b4 
    
<!-- ^^^^^
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

@orlitzky orlitzky orlitzky approved these changes

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