Refactor bootstrap, bootstrap-conda, m4/sage_spkg_collect.m4, sage-spkg-info through sage --package properties, sage-get-system-packages
#37430
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tobiasdiez
suggested changes
Feb 23, 2024
| @@ -18,10 +18,11 @@ SAGELIB_PACKAGES= | |||
| SAGELIB_OPTIONAL_PACKAGES= | |||
| DEVELOP_PACKAGES= | |||
|
|
|||
| eval $(sage-package properties --format=shell :all:) | |||
There was a problem hiding this comment.
I don't think it's a good idea to tie bootstrap conda even closer to the sage-the-distribution scripts. Please revert the changes in the bootstrap-conda file.
There was a problem hiding this comment.
@mkoeppe could you elaborate why this is "nonsense" from your point of view?
There was a problem hiding this comment.
This PR replaces direct access to the files in build/pkgs (an implementation detail of the sage-bootstrap package) by access through a script of the sage-bootstrap package. Such refactoring does not "tie bootstrap closer to sage-the-distribution scripts".
mkoeppe
force-pushed
the
bootstrap_use_sage_package_properties
branch
from
455fdc5
to
fa008a0
Compare
mkoeppe
force-pushed
the
bootstrap_use_sage_package_properties
branch
from
fa008a0
to
4fb182c
Compare
mkoeppe
force-pushed
the
bootstrap_use_sage_package_properties
branch
from
0ef9680
to
c551a95
Compare
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
Apr 1, 2024
…nts.txt`
<!-- ^^^^^
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 -->
As discussed in sagemath#36982:
- the name "install-requires" has become outdated with the transition
to the new names set by the `pyproject.toml` format (see https://setupto
ols.pypa.io/en/latest/userguide/dependency_management.html#declaring-
required-dependency, compare the tabs "pyproject.toml" vs. "setup.cfg")
- this will help with sagemath#35890, as
GitHub will be able to just read the `version_requirements.txt` files
sagemath#36982 (comment)
Notes for reviewers:
- **This is a simple, limited-scope improvement and not intended as a
long-term commitment to the format of the files in build/pkgs/....**
- PRs sagemath#37430, sagemath#37350, sagemath#36740 remove direct access to build/pkgs in favor
of using the sage_bootstrap API (sage --package).
<!-- 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#37401
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
URL: sagemath#36999
Reported by: Matthias Köppe
Reviewer(s): Dima Pasechnik, Kwankyu Lee, Nathan Dunfield
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
Apr 8, 2024
…nts.txt`
<!-- ^^^^^
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 -->
As discussed in sagemath#36982:
- the name "install-requires" has become outdated with the transition
to the new names set by the `pyproject.toml` format (see https://setupto
ols.pypa.io/en/latest/userguide/dependency_management.html#declaring-
required-dependency, compare the tabs "pyproject.toml" vs. "setup.cfg")
- this will help with sagemath#35890, as
GitHub will be able to just read the `version_requirements.txt` files
sagemath#36982 (comment)
Notes for reviewers:
- **This is a simple, limited-scope improvement and not intended as a
long-term commitment to the format of the files in build/pkgs/....**
- PRs sagemath#37430, sagemath#37350, sagemath#36740 remove direct access to build/pkgs in favor
of using the sage_bootstrap API (sage --package).
<!-- 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#37401
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
URL: sagemath#36999
Reported by: Matthias Köppe
Reviewer(s): Dima Pasechnik, Kwankyu Lee, Nathan Dunfield
mkoeppe
force-pushed
the
bootstrap_use_sage_package_properties
branch
from
c551a95
to
541f27c
Compare
|
Documentation preview for this PR (built with commit 2dcb369; changes) is ready! 🎉 |
mkoeppe
force-pushed
the
bootstrap_use_sage_package_properties
branch
from
541f27c
to
96386ec
Compare
mkoeppe
force-pushed
the
bootstrap_use_sage_package_properties
branch
from
96386ec
to
90ffd4b
Compare
mkoeppe
force-pushed
the
bootstrap_use_sage_package_properties
branch
from
90ffd4b
to
c2594fb
Compare
mkoeppe
force-pushed
the
bootstrap_use_sage_package_properties
branch
from
fd888ae
to
048d891
Compare
mkoeppe
changed the title
bootstrap, bootstrap-conda, m4/sage_spkg_collect.m4 through sage --package propertiesbootstrap, bootstrap-conda, m4/sage_spkg_collect.m4, sage-spkg-info through sage --package properties, sage-get-system-packages
|
@orlitzky If you have a moment, could you take a look if my changes to |
|
Thanks for testing. Dependencies are broken; I've fixed this in one of the follow-up PRs already, I'll try to find it. |
mkoeppe
force-pushed
the
bootstrap_use_sage_package_properties
branch
from
297cb53
to
c13b3db
Compare
kwankyu
reviewed
Jun 13, 2024
|
Thank you! |
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
Jun 16, 2024
…pkg_collect.m4`, `sage-spkg-info` through `sage --package properties`, `sage-get-system-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 -->
Using the command `sage --package properties` added in
sagemath#37018, we eliminate some direct
references to `build/pkgs/` from various scripts and reduce code
duplication in reading SPKG metadata.
Using other `sage --package` commands, we also simplify the code for
`bootstrap -s` and `bootstrap -D`.
Review instructions: Note that there are changes to the
`bootstrap`-generated file `m4/sage_spkg_configures.m4`:
- The macro `SAGE_SPKG_COLLECT_INIT` is now called explicitly, with an
argument (the full list of packages).
- Lines such as `m4_define([SPKG_INSTALL_REQUIRES_exceptiongroup],
[['exceptiongroup; python_version<\"3.11\"',]])dnl` are added for use by
the macro `SAGE_PYTHON_PACKAGE_CHECK` (replacing direct access to
`version_requirements.txt` files)
<!-- 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". -->
This is:
- Split out from sagemath#36740
<!-- 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#37430
Reported by: Matthias Köppe
Reviewer(s): Julian Rüth, Kwankyu Lee, Matthias Köppe, Tobias Diez
|
Bootstrap download of the confball does not work (note the missing version): |
5 tasks
|
@vbraun Do you really use |
|
(setting |
|
The buildbot uses it to download the confball that is being tested |
|
The existing code there doesn't even unpack the downloaded tarball |
Here's a repaired version, I've kept that as is |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Using the command
sage --package propertiesadded in #37018, we eliminate some direct references tobuild/pkgs/from various scripts and reduce code duplication in reading SPKG metadata.Using other
sage --packagecommands, we also simplify the code forbootstrap -sandbootstrap -D.Review instructions: Note that there are changes to the
bootstrap-generated filem4/sage_spkg_configures.m4:SAGE_SPKG_COLLECT_INITis now called explicitly, with an argument (the full list of packages).m4_define([SPKG_INSTALL_REQUIRES_exceptiongroup], [['exceptiongroup; python_version<\"3.11\"',]])dnlare added for use by the macroSAGE_PYTHON_PACKAGE_CHECK(replacing direct access toversion_requirements.txtfiles)This is:
bootstrap-conda,m4/sage_spkg_collect.m4through sage-package; handledependencies_build#36740📝 Checklist
⌛ Dependencies