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

sage --package list: Sort output, add switches --{in,ex}clude-dependencies #36393

Merged
merged 2 commits into from
Oct 14, 2023
Merged

sage --package list: Sort output, add switches --{in,ex}clude-dependencies #36393

merged 2 commits into from
Oct 14, 2023

Conversation

mkoeppe
Copy link
Contributor

@mkoeppe mkoeppe commented Oct 3, 2023

We enhance the command sage --package list:

  • New switches --{in,ex}clude-dependencies walk the tree of dependencies declared in dependencies files
  • Output is always (uniquified and) sorted, eliminating the use of sort in various scripts
  • Handling of --exclude is moved to the end, which improves its applicability.

The new parsing of the dependencies* files in sage_bootstrap is generic preparation for handling more of the bootstrapping phase in Python, as sketched in #33860 (part of #29146).

This PR is immediate preparation for #35593, where it is desired to create "minimized" lists of system packages that do not include all standard packages that are really only needed as dependencies of "top-level" packages.

📝 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 Oct 3, 2023
@github-actions
Copy link

github-actions bot commented Oct 3, 2023

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

@orlitzky
Copy link
Contributor

orlitzky commented Oct 4, 2023

Do we have a notion of "top level" package yet, or is that TBD? I ask because things like gengetopt and patch are listed in the output of sage --package list --exclude-dependencies :standard: and I'm fairly sure that those are only needed as dependencies of other things that sage needs.

Unrelated, this outputs a list of packages,

$ ./sage --package list --exclude-dependencies :all:

but this,

$ ./sage --package list --exclude-dependencies

outputs nothing. IIUC they should do the same thing?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 4, 2023

Do we have a notion of "top level" package yet, or is that TBD?

No, this cannot be declared yet (any thoughts?); for use in #35593 I just with --exclude-dependencies to eliminate packages that will be pulled in by others.

I ask because things like gengetopt and patch are listed in the output of sage --package list --exclude-dependencies :standard: and I'm fairly sure that those are only needed as dependencies of other things that sage needs.

gengetopt and patch are order-only dependencies, but the new switches only deal with proper dependencies.

Unrelated, this outputs a list of packages,

$ ./sage --package list --exclude-dependencies :all:

but this,

$ ./sage --package list --exclude-dependencies

outputs nothing. IIUC they should do the same thing?

Without the new switches, the default is indeed :all:, which is nice for interactive use. But with the new switches given, I have changed the default to empty, which works better for scripting uses.

@orlitzky
Copy link
Contributor

orlitzky commented Oct 5, 2023

Do we have a notion of "top level" package yet, or is that TBD?

No, this cannot be declared yet (any thoughts?); for use in #35593 I just with --exclude-dependencies to eliminate packages that will be pulled in by others.

It's hard to do properly without writing a package manager. The entries in build/pkgs/sagelib/dependencies are a good start, but there are lots of runtime dependencies missing, and the $(FOO) deps to worry about, and the things controlled by ./configure --disable-foo...

We may wind up with a hard-coded list for a while., but at least the distribution maintainers have already come up with that list (they should all be roughly the same).

Without the new switches, the default is indeed :all:, which is nice for interactive use. But with the new switches given, I have changed the default to empty, which works better for scripting uses.

Can the --help output mention this then? That's the only documentation we have.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 6, 2023

Without the new switches, the default is indeed :all:, which is nice for interactive use. But with the new switches given, I have changed the default to empty, which works better for scripting uses.

Can the --help output mention this then?

Good idea, done

orlitzky
orlitzky approved these changes Oct 7, 2023
View reviewed changes
Copy link
Contributor

@orlitzky orlitzky left a comment

Choose a reason for hiding this comment

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

I'm not sure that this in-between approach is how I'd do it: if we came up with that list of direct dependencies now, we might also be able to find a way to hide the other packages from ./configure and make. That would be nice for conda and the distros because the package manager install instructions would contain only a minimal list, and ./configure would be able to skip a few hundred pointless checks for non-direct dependencies.

Still, this doesn't hurt anything, so it's OK too.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 7, 2023

Thanks!

@vbraun
Copy link
Member

vbraun commented Oct 8, 2023

Merge conflict

@mkoeppe mkoeppe force-pushed the sage_package_list_dependencies branch from c4b242f to bc7d33e Compare October 8, 2023 20:04
@vbraun vbraun merged commit 699dbf1 into sagemath:develop Oct 14, 2023
8 of 20 checks passed
@mkoeppe mkoeppe added this to the sage-10.2 milestone Oct 14, 2023
vbraun pushed a commit to vbraun/sage that referenced this pull request Oct 19, 2023
sagemathgh-36476: Fix bootstrapping on centos-7, fedora-30, `ubun…
c47788b 
…tu-xenial`

<!-- ^^^^^
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 -->

<!-- Why is this change required? What problem does it solve? -->
On `centos-7`, still supported until June 2024, we have to use sage-
bootstrap-python = python2.
Support for Python 2 was broken accidentally in sagemath#36393, see https://gith
ub.com/sagemath/sage/actions/runs/6520548150/job/17767608828#step:10:192
7

Fixed here, as tested with `tox -e docker-centos-7-devtoolset-
gcc_11-standard -- config.status`.

Same issue also affects `fedora-30`, which is still tested by the CI as
a proxy for old RHEL, and `ubuntu-xenial`. See sagemath#32074

<!-- 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#36476
Reported by: Matthias Köppe
Reviewer(s): Tobias Diez
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

@dimpase dimpase Awaiting requested review from dimpase

@jhpalmieri jhpalmieri Awaiting requested review from jhpalmieri

Assignees

@mkoeppe mkoeppe

Labels
c: scripts
Projects
None yet
Milestone
sage-10.2
Development

Successfully merging this pull request may close these issues.
3 participants
@mkoeppe @orlitzky @vbraun