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

build/pkgs/nauty: Update to 2.8.8; use VERSION in all upstream_urls. #36774

Merged
merged 8 commits into from
Dec 19, 2023

Conversation

mkoeppe
Copy link
Contributor

@mkoeppe mkoeppe commented Nov 26, 2023

https://pallini.di.uniroma1.it/changes24-28.txt

Also, motivated by a mistake I made while preparing this PR (#36774 (comment)), and previously in #35380 (see #36781), and countless other times, I am changing all upstream_urls to use VERSION instead of the hardcoded version number. This makes it less error-prone to use sage -package update SPKG NEW_VERSION and sage -package update-latest. These commands now also display the URL from which they are downloading the tarball, and they warn if the upstream_url field does not contain the VERSION variable.

(Exceptions: ipykernel, which is fixed in #36129, and libbraiding, which is fixed in #36781)

Because some of the upstream_url need the version in a different format, a mechanism to refer to the components of a version is added: The strings VERSION_MAJOR, VERSION_MINOR, VERSION_MICRO can be used. For added readability, instead of the naked strings, also the notation ${VERSION_MAJOR} etc. can be used. For example, nauty, which previously had to use a hardcoded version number, now uses the upstream URL pattern
upstream_url=https://pallini.di.uniroma1.it/nauty${VERSION_MAJOR}_${VERSION_MINOR}_${VERSION_MICRO}.tar.gz

The syntax ${VERSION_MAJOR}, borrowed from Bourne shell, matches what sage-bootstrap already uses in https://github.com/sagemath/sage/blob/develop/.upstream.d/10-SAGE_SERVER

📝 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
Copy link
Contributor Author

mkoeppe commented Nov 26, 2023

File "sage/graphs/generators/families.py", line 3678, in sage.graphs.generators.families.nauty_gentreeg
Failed example:
    [len(list(graphs.nauty_gentreeg(str(i)))) for i in range(1, 15)]
Expected:
    [1, 1, 1, 2, 3, 6, 11, 23, 47, 106, 235, 551, 1301, 3159]
Got:
    [1, 0, 1, 2, 3, 6, 11, 23, 47, 106, 235, 551, 1301, 3159]
**********************************************************************

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 26, 2023

@orlitzky Is this what nauty-2.8.6-gentreeg-gentourng.patch was fixing? Do we have a newer version of this patch?

@orlitzky
Copy link
Contributor

That's an upstream patch from right after the v2.8.6 release. I only included it because the maintainer didn't think it was worth a new bugfix release, and, since v2.8.6 just happened, we might have been waiting a while.

It shouldn't be necessary in nauty 2.8.7 or newer.

@orlitzky
Copy link
Contributor

It does appear in the changelog for v2.8.8:

  • Fixed gentreeg output for n=2, also corrected the table of counts
    in the source file, which was shifted by 1. Also gentourng -c 2.
    Thanks to Gonne Kretschmer for reporting these.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 26, 2023

Sorry for the noise, I found my mistake.

@mkoeppe mkoeppe changed the title build/pkgs/nauty: Update to 2.8.8 build/pkgs/nauty: Update to 2.8.8; use VERSION in all upstream_urls. Nov 26, 2023
Copy link
Contributor

@dcoudert dcoudert left a comment

Choose a reason for hiding this comment

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

LGTM (passes all tests on Fedora 35) but some failures are reported by the CI on specific systems.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 27, 2023

The failures of the "sitepackages" runs can be ignored -- many fail systematically, and we are reducing them in #36697.

I've restarted the other CI Linux incremental runs, which got interrupted

@dimpase
Copy link
Member

dimpase commented Nov 28, 2023

Why do we need to delay these variable substitutions until after the run of ./configure ?

At ./configure time the versions are known, so there is no need to reimplement autotools in Python...

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 28, 2023

You're in the wrong phase. ./sage -package operates on unconfigured, unbootstrapped source trees.

@dimpase
Copy link
Member

dimpase commented Nov 28, 2023

there is no need for this extra complication.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 28, 2023

At least, Dima, you would need to say what exactly it is that you are against.

@dimpase
Copy link
Member

dimpase commented Nov 29, 2023

it's weird that on one hand each package directory has files with templates which are filled by configure, as well as with templates filled in by an ad hoc script. The latter in particular has data uniquely identifying the package (from checksums).

There is no reason not to rewrite the templates in the latter along with filling in the checksums.

There is also no reason to insist on the ability to fill in the latter templates in an unconfigured tree (and so they can be filled in by configure).
The advantage is that then there is no need for tricky custom code to do the filling in.
The only disadvantage is that the current templates don't adhere to the usual autoconf syntax, but this is easy to fix, once and for all.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 29, 2023

Do you understand that using VERSION is already our existing practice?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 29, 2023

it's weird that on one hand each package directory has files with templates which are filled by configure

It does not. Yes, there are .in files, but they have nothing to do with configure.

@dimpase
Copy link
Member

dimpase commented Nov 29, 2023

and all of our existing practices are absolutely the best possible, and any attempt to talk about changing them is a violation of CoC?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 29, 2023

Well, Dima, trying to mock me for holding you to the Code of Conduct, which applies to everyone, is certainly a Code of Conduct violation. Please do better.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 30, 2023

There is also no reason to insist on the ability to fill in the latter templates in an unconfigured tree (and so they can be filled in by configure).

Separation of phases is very important for maintainability.

Specifically, note that when bootstrap downloads a configure tarball, it goes through this very substitution mechanism for determining the download URL. So this cannot depend on running the configure script.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 14, 2023

I had to add one commit so that sage --package download understands the option --no-check-certificate, which is used in the CI.

Copy link

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

@jhpalmieri
Copy link
Member

Looks fine.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 15, 2023

Thanks!

@vbraun vbraun merged commit cd27965 into sagemath:develop Dec 19, 2023
23 of 30 checks passed
@mkoeppe mkoeppe added this to the sage-10.3 milestone Dec 19, 2023
@mkoeppe mkoeppe deleted the nauty_2_8_8 branch December 19, 2023 22:18
@mkoeppe mkoeppe added the disputed PR is waiting for community vote, see https://groups.google.com/g/sage-devel/c/IgBYUJl33SQ label Dec 23, 2023
vbraun pushed a commit to vbraun/sage that referenced this pull request Dec 27, 2023
…eck-certificate`

<!-- ^^^^^
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 -->
A mistake in sagemath#36774 broke the macOS CI in 10.3.beta3, see
https://github.com/sagemath/sage/actions/runs/7255417440/job/19765989346
#step:9:1425
```
  [patch-2.7.5] error installing, exit status 1. End of log file:
  [patch-2.7.5]   usage: sage --package [-h] [--log LOG] [--no-check-
certificate]
  [patch-2.7.5]
{config,list,name,tarball,apropos,update,update-
latest,download,upload,fix-checksum,create,clean}
  [patch-2.7.5]                         ...
  [patch-2.7.5]   sage --package: error: unrecognized arguments: --no-
check-certificate
  [patch-2.7.5]
************************************************************************
  [patch-2.7.5]   Error downloading tarball of patch
  [patch-2.7.5]
************************************************************************
```

We make the one-line fix for this here.

Tests (with other merged branches) at
https://github.com/mkoeppe/sage/actions/runs/7295997767/job/19883171237

<!-- 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#36947
Reported by: Matthias Köppe
Reviewer(s): Kwankyu Lee
vbraun pushed a commit to vbraun/sage that referenced this pull request Jan 2, 2024
…eck-certificate`

    
<!-- ^^^^^
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 -->
A mistake in sagemath#36774 broke the macOS CI in 10.3.beta3, see
https://github.com/sagemath/sage/actions/runs/7255417440/job/19765989346
#step:9:1425
```
  [patch-2.7.5] error installing, exit status 1. End of log file:
  [patch-2.7.5]   usage: sage --package [-h] [--log LOG] [--no-check-
certificate]
  [patch-2.7.5]
{config,list,name,tarball,apropos,update,update-
latest,download,upload,fix-checksum,create,clean}
  [patch-2.7.5]                         ...
  [patch-2.7.5]   sage --package: error: unrecognized arguments: --no-
check-certificate
  [patch-2.7.5]
************************************************************************
  [patch-2.7.5]   Error downloading tarball of patch
  [patch-2.7.5]
************************************************************************
```

We make the one-line fix for this here.

Tests (with other merged branches) at
https://github.com/mkoeppe/sage/actions/runs/7295997767/job/19883171237

<!-- 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#36947
Reported by: Matthias Köppe
Reviewer(s): Kwankyu Lee
vbraun pushed a commit to vbraun/sage that referenced this pull request Jan 5, 2024
…eck-certificate`

    
<!-- ^^^^^
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 -->
A mistake in sagemath#36774 broke the macOS CI in 10.3.beta3, see
https://github.com/sagemath/sage/actions/runs/7255417440/job/19765989346
#step:9:1425
```
  [patch-2.7.5] error installing, exit status 1. End of log file:
  [patch-2.7.5]   usage: sage --package [-h] [--log LOG] [--no-check-
certificate]
  [patch-2.7.5]
{config,list,name,tarball,apropos,update,update-
latest,download,upload,fix-checksum,create,clean}
  [patch-2.7.5]                         ...
  [patch-2.7.5]   sage --package: error: unrecognized arguments: --no-
check-certificate
  [patch-2.7.5]
************************************************************************
  [patch-2.7.5]   Error downloading tarball of patch
  [patch-2.7.5]
************************************************************************
```

We make the one-line fix for this here.

Tests (with other merged branches) at
https://github.com/mkoeppe/sage/actions/runs/7295997767/job/19883171237

<!-- 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#36947
Reported by: Matthias Köppe
Reviewer(s): Kwankyu Lee
vbraun pushed a commit to vbraun/sage that referenced this pull request Jan 13, 2024
…eck-certificate`

    
<!-- ^^^^^
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 -->
A mistake in sagemath#36774 broke the macOS CI in 10.3.beta3, see
https://github.com/sagemath/sage/actions/runs/7255417440/job/19765989346
#step:9:1425
```
  [patch-2.7.5] error installing, exit status 1. End of log file:
  [patch-2.7.5]   usage: sage --package [-h] [--log LOG] [--no-check-
certificate]
  [patch-2.7.5]
{config,list,name,tarball,apropos,update,update-
latest,download,upload,fix-checksum,create,clean}
  [patch-2.7.5]                         ...
  [patch-2.7.5]   sage --package: error: unrecognized arguments: --no-
check-certificate
  [patch-2.7.5]
************************************************************************
  [patch-2.7.5]   Error downloading tarball of patch
  [patch-2.7.5]
************************************************************************
```

We make the one-line fix for this here.

Tests (with other merged branches) at
https://github.com/mkoeppe/sage/actions/runs/7295997767/job/19883171237

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

mkoeppe commented Feb 3, 2024

@vbraun Apparently the tarball is missing on the mirrors.
https://github.com/sagemath/sage/actions/runs/7761817689/job/21171029322#step:4:3901

@vbraun
Copy link
Member

vbraun commented Feb 4, 2024

OK uploaded

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: distribution c: graph theory c: packages: standard disputed PR is waiting for community vote, see https://groups.google.com/g/sage-devel/c/IgBYUJl33SQ
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants