Skip to content

Conversation

mkoeppe
Copy link
Contributor

@mkoeppe mkoeppe commented Feb 22, 2024

Using the command sage --package properties added in #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)

This is:

📝 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

@@ -18,10 +18,11 @@ SAGELIB_PACKAGES=
SAGELIB_OPTIONAL_PACKAGES=
DEVELOP_PACKAGES=

eval $(sage-package properties --format=shell :all:)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nonsense

Copy link
Member

Choose a reason for hiding this comment

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

@mkoeppe could you elaborate why this is "nonsense" from your point of view?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@saraedum saraedum mentioned this pull request Mar 21, 2024
5 tasks
vbraun pushed a commit to vbraun/sage that referenced this pull request Apr 1, 2024
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
vbraun pushed a commit to vbraun/sage that referenced this pull request Apr 8, 2024
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
Copy link

github-actions bot commented Apr 9, 2024

Documentation preview for this PR (built with commit 2dcb369; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 14, 2024

@orlitzky If you have a moment, could you take a look if my changes to SAGE_PYTHON_PACKAGE_CHECK look OK to you?

@orlitzky
Copy link
Contributor

Yeah LGTM

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 17, 2024

@orlitzky Can I interest you in reviewing the whole PR?
It's the way to what you asked for in #37902 (comment)

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 6, 2024

@roed314 Would you perhaps be interested in reviewing this? See #37902 (comment)

@kwankyu
Copy link
Collaborator

kwankyu commented Jun 12, 2024

LGTM as far as I can understand. Most changes are simplifications using new sage --package and sage-get-system-packages commands.

But with this PR, build from scratch quickly fails for me:

brial-1.2.8.log

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 12, 2024

Thanks for testing. Dependencies are broken; I've fixed this in one of the follow-up PRs already, I'll try to find it.

Copy link
Collaborator

@kwankyu kwankyu left a comment

Choose a reason for hiding this comment

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

Thanks. Working well.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 13, 2024

Thank you!

vbraun pushed a commit to vbraun/sage that referenced this pull request Jun 16, 2024
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
@vbraun
Copy link
Member

vbraun commented Jun 16, 2024

Bootstrap download of the confball does not work (note the missing version):

$ ./bootstrap -u http://sagepad.org/spkg/upstream/configure
[...]
Error: downloading configure-.tar.gz from http://sagepad.org/spkg/upstream/configure failed

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 16, 2024

@vbraun Do you really use bootstrap -u or can we remove this option?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 16, 2024

(setting SAGE_SERVER=http://sagepad.org should have the same effect)

@vbraun
Copy link
Member

vbraun commented Jun 16, 2024

The buildbot uses it to download the confball that is being tested

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 16, 2024

The existing code there doesn't even unpack the downloaded tarball

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 16, 2024

The existing code there doesn't even unpack the downloaded tarball

Here's a repaired version, I've kept that as is

@vbraun vbraun merged commit ac51269 into sagemath:develop Jun 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants