Refactor bootstrap, bootstrap-conda, m4/sage_spkg_collect.m4, sage-spkg-info through sage --package properties, sage-get-system-packages#37430
Conversation
| 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".
sagemathgh-36999: Rename `install-requires.txt` to `version_requirements.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
sagemathgh-36999: Rename `install-requires.txt` to `version_requirements.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
|
Documentation preview for this PR (built with commit 2dcb369; changes) is ready! 🎉 |
|
@orlitzky If you have a moment, could you take a look if my changes to |
|
Yeah LGTM |
|
@orlitzky Can I interest you in reviewing the whole PR? |
|
@roed314 Would you perhaps be interested in reviewing this? See #37902 (comment) |
bootstrap: Remove last direct use of 'build/pkgs' bootstrap-conda: Use 'sage-package properties' and 'sage-package dependencies' Update shell uses of 'sage-package properties'
…'./bootstrap && rm -rf build/pkgs/cachetools; ./configure')
… version/checksums
…ystem-packages versions'
|
LGTM as far as I can understand. Most changes are simplifications using new But with this PR, build from scratch quickly fails for me: |
|
Thanks for testing. Dependencies are broken; I've fixed this in one of the follow-up PRs already, I'll try to find it. |
|
Thank you! |
sagemathgh-37430: Refactor `bootstrap`, `bootstrap-conda`, `m4/sage_spkg_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): |
|
@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 |
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