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.

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

sage --package properties #37018

Merged
merged 7 commits into from
Jan 22, 2024
Merged

Conversation

mkoeppe
Copy link
Contributor

@mkoeppe mkoeppe commented Jan 6, 2024

For example:

$ ./sage -package properties --format shell gap ecl                                                                                                                               git:sage_package_properties
path_ecl='/Users/mkoeppe/s/sage/sage-rebasing/worktree-pristine/build/pkgs/ecl'
version_with_patchlevel_ecl='21.2.1'
type_ecl='standard'
source_ecl='normal'
trees_ecl='SAGE_LOCAL'
path_gap='/Users/mkoeppe/s/sage/sage-rebasing/worktree-pristine/build/pkgs/gap'
version_with_patchlevel_gap='4.12.2'
type_gap='standard'
source_gap='normal'
trees_gap='SAGE_LOCAL'

The output is suitable as input for eval in Bourne shell. (The default output format is more human-friendly.)

Split out from:

where this new interface is used to eliminate direct references to build/pkgs/ from various scripts and reduce code duplication and complexity in reading SPKG metadata.

📝 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

@kwankyu
Copy link
Collaborator

kwankyu commented Jan 12, 2024

It would be natural to print like

$ ./sage -package properties gap                                                                                                                              
path='/Users/mkoeppe/s/sage/sage-rebasing/worktree-pristine/build/pkgs/gap'
version_with_patchlevel='4.12.2'
type='standard'
source='normal'
trees='SAGE_LOCAL'

Why accept multiple pac and add _pac?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jan 12, 2024

Why accept multiple pac and add _pac?

So that a shell script can request this information in bulk instead of invoking this script many times.
(In #36740, this gives a speedup in running ./configure)

@kwankyu
Copy link
Collaborator

kwankyu commented Jan 12, 2024

Would you point where the suffixed string is used?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jan 12, 2024

In https://github.com/sagemath/sage/pull/36740/files#diff-b1f0deab6b85906bd925c4fceb337406ebea20bdc199c63996977328d24cf5c5R347, take a look at:

  • m4/sage_spkg_collect.m4 line 155 SPKG_VERSION=$[version_with_patchlevel_]SPKG_NAME
  • bootstrap line 58 eval DIR=\$path_$pkgname pkgtype=\$type_$pkgname pkgsource=\$source_$pkgname and similar

@kwankyu
Copy link
Collaborator

kwankyu commented Jan 12, 2024

I see. You use the output directly to define variables...

I think the command should accommodate two uses: standalone and as pipeline.

@kwankyu
Copy link
Collaborator

kwankyu commented Jan 12, 2024

How about adding an option --output=list and --output=vars with default the former? Thus

$ ./sage -package properties gap
gap:                                                                                                                              
  path='/Users/mkoeppe/s/sage/sage-rebasing/worktree-pristine/build/pkgs/gap'
  version_with_patchlevel='4.12.2'
  type='standard'
  source='normal'
  trees='SAGE_LOCAL'
$ ./sage -package properties --output=vars gap                                                                                                                              
path_gap='/Users/mkoeppe/s/sage/sage-rebasing/worktree-pristine/build/pkgs/gap'
version_with_patchlevel_gap='4.12.2'
type_gap='standard'
source_gap='normal'
trees_gap='SAGE_LOCAL'

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jan 12, 2024

I'm open to creating output format options when needed (as you may have seen in #36740, sage --package dependencies has an option --format={plain,shell}), but I'm reluctant to add them without a specific use case

@kwankyu
Copy link
Collaborator

kwankyu commented Jan 12, 2024

--format is okay.

A use case is just to read the output of ./sage -package properties gap by human eyes. This use is not expected?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jan 12, 2024

We already have a tool that formats package info for human inspection, that's sage -info SPKG...

@kwankyu
Copy link
Collaborator

kwankyu commented Jan 12, 2024

But ./sage -package properties gap intends, as far as I understand, to provide other info (about installed packages) oriented for developers. That is a sage command, and developers may use them. If the command is only for shells, then why expose it as a sage command?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jan 12, 2024

./sage -package just delegates to sage-package, and ./sage -info just delegates to sage-spkg-info; both a provided by the Sage distribution. (But I feel I'm missing your point)

@kwankyu
Copy link
Collaborator

kwankyu commented Jan 12, 2024

If the command is internal and not exposed to developers, I won't care the output format. But it is exposed and developers may use it and read the output, and then the output should be in a readable format. Currently the format is only suitable to shells. (I wonder if I misunderstand something...)

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jan 12, 2024

OK, that makes sense, I'll add an option.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jan 12, 2024

How's this?

@kwankyu
Copy link
Collaborator

kwankyu commented Jan 12, 2024

Thanks. Looks good to me.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jan 12, 2024

Thank you!

@mkoeppe mkoeppe force-pushed the sage_package_properties branch from c665629 to 8ff9e85 Compare January 14, 2024 21:58
vbraun pushed a commit to vbraun/sage that referenced this pull request Jan 16, 2024
    
<!-- ^^^^^
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 -->

For example:
```
$ ./sage -package properties --format shell gap ecl
git:sage_package_properties
path_ecl='/Users/mkoeppe/s/sage/sage-rebasing/worktree-
pristine/build/pkgs/ecl'
version_with_patchlevel_ecl='21.2.1'
type_ecl='standard'
source_ecl='normal'
trees_ecl='SAGE_LOCAL'
path_gap='/Users/mkoeppe/s/sage/sage-rebasing/worktree-
pristine/build/pkgs/gap'
version_with_patchlevel_gap='4.12.2'
type_gap='standard'
source_gap='normal'
trees_gap='SAGE_LOCAL'
```
The output is suitable as input for `eval` in Bourne shell. (The default
output format is more human-friendly.)

<!-- Why is this change required? What problem does it solve? -->
Split out from:
- sagemath#36740

where this new interface is used to eliminate direct references to
`build/pkgs/` from various scripts and reduce code duplication and
complexity in reading SPKG metadata.

<!-- 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#37018
Reported by: Matthias Köppe
Reviewer(s): Kwankyu Lee
@vbraun
Copy link
Member

vbraun commented Jan 16, 2024

Do all supported platforms use python3 as the "default" python? I'm getting:

Traceback (most recent call last):
  File "/var/lib/buildbot/slave/sage_git/build/build/bin/sage-package", line 40, in <module>
    from sage_bootstrap.cmdline import run
  File "/var/lib/buildbot/slave/sage_git/build/build/bin/../sage_bootstrap/cmdline.py", line 31, in <module>
    from sage_bootstrap.app import Application
  File "/var/lib/buildbot/slave/sage_git/build/build/bin/../sage_bootstrap/app.py", line 80
    def properties(self, *package_classes, props=['path', 'version_with_patchlevel', 'type', 'source', 'trees'],
                                               ^
SyntaxError: invalid syntax
************************************************************************
Error downloading tarball of ecl
************************************************************************

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jan 16, 2024

Sorry about this. No, we still support python 2.7 as sage-bootstrap-python; see #35858.

vbraun pushed a commit to vbraun/sage that referenced this pull request Jan 16, 2024
    
<!-- ^^^^^
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 -->

For example:
```
$ ./sage -package properties --format shell gap ecl
git:sage_package_properties
path_ecl='/Users/mkoeppe/s/sage/sage-rebasing/worktree-
pristine/build/pkgs/ecl'
version_with_patchlevel_ecl='21.2.1'
type_ecl='standard'
source_ecl='normal'
trees_ecl='SAGE_LOCAL'
path_gap='/Users/mkoeppe/s/sage/sage-rebasing/worktree-
pristine/build/pkgs/gap'
version_with_patchlevel_gap='4.12.2'
type_gap='standard'
source_gap='normal'
trees_gap='SAGE_LOCAL'
```
The output is suitable as input for `eval` in Bourne shell. (The default
output format is more human-friendly.)

<!-- Why is this change required? What problem does it solve? -->
Split out from:
- sagemath#36740

where this new interface is used to eliminate direct references to
`build/pkgs/` from various scripts and reduce code duplication and
complexity in reading SPKG metadata.

<!-- 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#37018
Reported by: Matthias Köppe
Reviewer(s): Kwankyu Lee
Copy link

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

vbraun pushed a commit to vbraun/sage that referenced this pull request Jan 16, 2024
    
<!-- ^^^^^
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 -->

For example:
```
$ ./sage -package properties --format shell gap ecl
git:sage_package_properties
path_ecl='/Users/mkoeppe/s/sage/sage-rebasing/worktree-
pristine/build/pkgs/ecl'
version_with_patchlevel_ecl='21.2.1'
type_ecl='standard'
source_ecl='normal'
trees_ecl='SAGE_LOCAL'
path_gap='/Users/mkoeppe/s/sage/sage-rebasing/worktree-
pristine/build/pkgs/gap'
version_with_patchlevel_gap='4.12.2'
type_gap='standard'
source_gap='normal'
trees_gap='SAGE_LOCAL'
```
The output is suitable as input for `eval` in Bourne shell. (The default
output format is more human-friendly.)

<!-- Why is this change required? What problem does it solve? -->
Split out from:
- sagemath#36740

where this new interface is used to eliminate direct references to
`build/pkgs/` from various scripts and reduce code duplication and
complexity in reading SPKG metadata.

<!-- 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#37018
Reported by: Matthias Köppe
Reviewer(s): Kwankyu Lee
@vbraun vbraun merged commit a5b015c into sagemath:develop Jan 22, 2024
14 of 18 checks passed
@mkoeppe mkoeppe deleted the sage_package_properties branch January 22, 2024 00:45
vbraun pushed a commit to vbraun/sage that referenced this pull request Feb 22, 2024
    
<!-- ^^^^^
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 -->
We add a new command `sage --package dependencies`.
Like the command `sage --package properties` introduced in sagemath#37018, it
can be used to eliminate direct references to `build/pkgs/` from various
scripts and reduce code duplication in reading SPKG metadata.

Here we only make such changes in `sage-spkg-info`.
The only user-visible change is that the listed dependencies are now
sorted alphabetically; this hides [subtle nuances that may be expressed
in the sort order of some `dependencies` files](https://github.com/sagem
ath/sage/pull/36878#issuecomment-1876426258).

- Closes sagemath#33860

Split out from:
- sagemath#36740

<!-- 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#37350
Reported by: Matthias Köppe
Reviewer(s): Kwankyu Lee, Matthias Köppe
@mkoeppe mkoeppe added this to the sage-10.3 milestone Mar 7, 2024
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
vbraun pushed a commit to vbraun/sage that referenced this pull request Jun 17, 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants