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/python3: Update to 3.11.8, remove Cygwin patches, remove pointless rpaths on macOS #37914

Merged
merged 1 commit into from
May 12, 2024

Conversation

mkoeppe
Copy link
Member

@mkoeppe mkoeppe commented May 1, 2024

Author: @culler

Split out from:

📝 Checklist

  • The title is concise and informative.
  • 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 and checked the documentation preview.

⌛ Dependencies

@mkoeppe
Copy link
Member Author

mkoeppe commented May 1, 2024

Copy link

github-actions bot commented May 1, 2024

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

@mkoeppe
Copy link
Member Author

mkoeppe commented May 2, 2024

@jhpalmieri
Copy link
Member

I did ./configure --with-system-python3=no and I got a lot of doctest failures of the form ld: warning: duplicate -rpath '/Users/jpalmier/Sage/TESTING/sage-10.4.beta3/local/lib' ignored. I believe that I did not get the same failures with #37886.

@mkoeppe
Copy link
Member Author

mkoeppe commented May 2, 2024

Yes, this PR, split out from #37886, only takes care of one part of the rpaths issue.
I split it out because it is easier to review. I'll need more time to review the other changes in particular because I'm concerned about the effect it may have on packages installed from source using sage -pip install.

@jhpalmieri
Copy link
Member

Yes, this PR, split out from #37886, only takes care of one part of the rpaths issue. I split it out because it is easier to review. I'll need more time to review the other changes in particular because I'm concerned about the effect it may have on packages installed from source using sage -pip install.

It's hard to give this a positive review when it causes new doctest failures, though.

@mkoeppe
Copy link
Member Author

mkoeppe commented May 3, 2024

Wait, I missed that the failures that you were talking about are new!

@mkoeppe
Copy link
Member Author

mkoeppe commented May 3, 2024

I'll try to reproduce this.

@jhpalmieri
Copy link
Member

Wait, I missed that the failures that you were talking about are new!

I thought that they were, but I'll double-check.

@jhpalmieri
Copy link
Member

You're right, these seem to be pre-existing failures, at least according to my testing. Feel free to restore the positive review.

@mkoeppe
Copy link
Member Author

mkoeppe commented May 3, 2024

OK, thanks for checking!

vbraun pushed a commit to vbraun/sage that referenced this pull request May 4, 2024
…in patches, remove pointless rpaths on macOS

    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

Author: @culler

Split out from:
- sagemath#37886

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [ ] 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 and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#37914
Reported by: Matthias Köppe
Reviewer(s):
vbraun pushed a commit to vbraun/sage that referenced this pull request May 9, 2024
…in patches, remove pointless rpaths on macOS

    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

Author: @culler

Split out from:
- sagemath#37886

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [ ] 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 and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#37914
Reported by: Matthias Köppe
Reviewer(s):
vbraun pushed a commit to vbraun/sage that referenced this pull request May 11, 2024
…in patches, remove pointless rpaths on macOS

    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

Author: @culler

Split out from:
- sagemath#37886

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [ ] 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 and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#37914
Reported by: Matthias Köppe
Reviewer(s):
vbraun pushed a commit to vbraun/sage that referenced this pull request May 12, 2024
…in patches, remove pointless rpaths on macOS

    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

Author: @culler

Split out from:
- sagemath#37886

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [ ] 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 and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#37914
Reported by: Matthias Köppe
Reviewer(s):
@vbraun vbraun merged commit f5a724f into sagemath:develop May 12, 2024
21 of 40 checks passed
@mkoeppe mkoeppe added this to the sage-10.4 milestone May 12, 2024
vbraun pushed a commit to vbraun/sage that referenced this pull request May 12, 2024
    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

Update cryptographic hashes to use sha256 instead of sha1 due to
insecurity of sha1.

- Fixes sagemath#37691
- Fixes sagemath#37558, see also
sagemath#36677 (comment)

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [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. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->

- sagemath#37570
- sagemath#37249
- sagemath#37914
    
URL: sagemath#37726
Reported by: Faisal
Reviewer(s): Matthias Köppe, roed314
vbraun pushed a commit to vbraun/sage that referenced this pull request May 15, 2024
    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

Update cryptographic hashes to use sha256 instead of sha1 due to
insecurity of sha1.

- Fixes sagemath#37691
- Fixes sagemath#37558, see also
sagemath#36677 (comment)

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [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. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->

- sagemath#37570
- sagemath#37249
- sagemath#37914
    
URL: sagemath#37726
Reported by: Faisal
Reviewer(s): Matthias Köppe, roed314
vbraun pushed a commit to vbraun/sage that referenced this pull request May 15, 2024
    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

Update cryptographic hashes to use sha256 instead of sha1 due to
insecurity of sha1.

- Fixes sagemath#37691
- Fixes sagemath#37558, see also
sagemath#36677 (comment)

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [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. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->

- sagemath#37570
- sagemath#37249
- sagemath#37914
    
URL: sagemath#37726
Reported by: Faisal
Reviewer(s): Matthias Köppe, roed314
vbraun pushed a commit to vbraun/sage that referenced this pull request May 15, 2024
…ite when run on systems with no toolchain

    
<!-- ^ Please provide a concise and informative title. -->

This PR does the following (after splitting out the Python upgrade to
[sagemath#37914](sagemath#37914)):
1. Fix a comment in the Makefile about how to use the DEBUG flag so it
will actually work.
3. Edit sage-env so that it does not print errors to stderr and/or open
a dialog whenever it is run on a system which has neither XCode nor
command line tools installed.
4. Remove (almost) all uses of rpath on macOS (see below).
5. Fix the primecountpy spkg, which was the only one which actually
needed an rpath in its python extension module.

**About rpaths on macOS**
Sage has been misusing rpaths on macOS for a very long time.  As far as
I can tell, this was an error based on the incorrect assumption that
MACH binaries use rpath in the same way as ELF binaries.

The rpath in an ELF binary provides a search path, beyond the default
search path, where the loader looks for shared libraries needed by the
binary.

A MACH binary specifies the dynamic libraries it needs with load
commands of type LC_LOAD_DYLIB.  The value of a LC_LOAD_DYLIB load
command is a path to a specific library.  There is one such load command
for each library that the binary needs.  The path is an absolute path by
default.  However, the path is allowed to begin with the substring
`@rpath`.  When the binary is loaded that substring is replaced by the
value of each LC_RPATH load command in the binary to produce a list of
absolute paths (or relative paths if the value of the LC_RPATH begins
with `@loader_path`).  Each of the paths on that list is tried as a
possible location of the needed dynamic library.

An obvious consequence of this design is that if none of the
LC_LOAD_DYLIB paths contains the string `@rpath` then there is no need
for any LC_RPATH commands, since they won't be used.  With one exception
(primecountpy) every MACH binary produced when building Sage had
absolute paths for its LC_LOAD_DYLIB load commands.  Nonetheless Sage
was setting the rpath to $SAGE_LOCAL/lib in all of these binaries.  In
fact, it was doing that multiple times, producing many duplicate rpaths
all of which were useless.  (And duplicate rpaths are now deprecated by
Apple's linker.)

This PR removes all of those bogus rpaths.

**About xcode-select**
The sage-env script was calling gcc to find the names of its linker and
archiver in the unlikely case that either of those had a non-standard
name, and it was also calling xcode-select as part of a check to see
whether the selected XCode toolchain had an ld-classic executable.  That
executable was added in XCode 15 as a new name for the old linker when a
new linker was added.  Apple made it possible to use the old linker by
passing the linker option -ld_classic to the gcc command.  The new
linker was buggy in the beginning and was unable to link openblas.  So
numpy and scipy use the old linker.  Sage also wanted to use it.  While
the new linker is probably working well enough now as a linker, it
generates a warning when it encounters duplicate libraries in a link
command.  It also appears that the new linker, or XCode 15 itself,
somehow *creates* duplicate libraries.  While the warnings should be
innocuous, they were causing Sage doctest failures.  So this PR provides
code which adds -Wl,-ld_classic to LDFLAGS in sage-env to replace the
analogous code which was already there.  But the new code is careful to
redirect the error messages to /dev/mull so they do not get printed on
the users terminal whenever sage starts up on a machine which does not
have any XCode toolchain at all.  It also avoids calling gcc in that
case since doing so posts a dialog on the user's screen asking if they
would like to install command line tools.  The dialog is a nice touch,
but should only appear if the user is actually *using* gcc, not as part
of every invocation of Sage, especially since XCode uses the standard
names.

<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->



### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [ ] The title is concise and informative.
- [ ] 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 and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#37886
Reported by: Marc Culler
Reviewer(s): John H. Palmieri, Marc Culler, Matthias Köppe, Volker Braun
vbraun pushed a commit to vbraun/sage that referenced this pull request May 18, 2024
    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

Update cryptographic hashes to use sha256 instead of sha1 due to
insecurity of sha1.

- Fixes sagemath#37691
- Fixes sagemath#37558, see also
sagemath#36677 (comment)

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [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. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->

- sagemath#37570
- sagemath#37249
- sagemath#37914
    
URL: sagemath#37726
Reported by: Faisal
Reviewer(s): Matthias Köppe, roed314
vbraun pushed a commit to vbraun/sage that referenced this pull request May 18, 2024
    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

Update cryptographic hashes to use sha256 instead of sha1 due to
insecurity of sha1.

- Fixes sagemath#37691
- Fixes sagemath#37558, see also
sagemath#36677 (comment)

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [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. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->

- sagemath#37570
- sagemath#37249
- sagemath#37914
    
URL: sagemath#37726
Reported by: Faisal
Reviewer(s): Matthias Köppe, roed314
vbraun pushed a commit to vbraun/sage that referenced this pull request May 18, 2024
…ite when run on systems with no toolchain

    
<!-- ^ Please provide a concise and informative title. -->

This PR does the following (after splitting out the Python upgrade to
[sagemath#37914](sagemath#37914)):
1. Fix a comment in the Makefile about how to use the DEBUG flag so it
will actually work.
3. Edit sage-env so that it does not print errors to stderr and/or open
a dialog whenever it is run on a system which has neither XCode nor
command line tools installed.
4. Remove (almost) all uses of rpath on macOS (see below).
5. Fix the primecountpy spkg, which was the only one which actually
needed an rpath in its python extension module.

**About rpaths on macOS**
Sage has been misusing rpaths on macOS for a very long time.  As far as
I can tell, this was an error based on the incorrect assumption that
MACH binaries use rpath in the same way as ELF binaries.

The rpath in an ELF binary provides a search path, beyond the default
search path, where the loader looks for shared libraries needed by the
binary.

A MACH binary specifies the dynamic libraries it needs with load
commands of type LC_LOAD_DYLIB.  The value of a LC_LOAD_DYLIB load
command is a path to a specific library.  There is one such load command
for each library that the binary needs.  The path is an absolute path by
default.  However, the path is allowed to begin with the substring
`@rpath`.  When the binary is loaded that substring is replaced by the
value of each LC_RPATH load command in the binary to produce a list of
absolute paths (or relative paths if the value of the LC_RPATH begins
with `@loader_path`).  Each of the paths on that list is tried as a
possible location of the needed dynamic library.

An obvious consequence of this design is that if none of the
LC_LOAD_DYLIB paths contains the string `@rpath` then there is no need
for any LC_RPATH commands, since they won't be used.  With one exception
(primecountpy) every MACH binary produced when building Sage had
absolute paths for its LC_LOAD_DYLIB load commands.  Nonetheless Sage
was setting the rpath to $SAGE_LOCAL/lib in all of these binaries.  In
fact, it was doing that multiple times, producing many duplicate rpaths
all of which were useless.  (And duplicate rpaths are now deprecated by
Apple's linker.)

This PR removes all of those bogus rpaths.

**About xcode-select**
The sage-env script was calling gcc to find the names of its linker and
archiver in the unlikely case that either of those had a non-standard
name, and it was also calling xcode-select as part of a check to see
whether the selected XCode toolchain had an ld-classic executable.  That
executable was added in XCode 15 as a new name for the old linker when a
new linker was added.  Apple made it possible to use the old linker by
passing the linker option -ld_classic to the gcc command.  The new
linker was buggy in the beginning and was unable to link openblas.  So
numpy and scipy use the old linker.  Sage also wanted to use it.  While
the new linker is probably working well enough now as a linker, it
generates a warning when it encounters duplicate libraries in a link
command.  It also appears that the new linker, or XCode 15 itself,
somehow *creates* duplicate libraries.  While the warnings should be
innocuous, they were causing Sage doctest failures.  So this PR provides
code which adds -Wl,-ld_classic to LDFLAGS in sage-env to replace the
analogous code which was already there.  But the new code is careful to
redirect the error messages to /dev/mull so they do not get printed on
the users terminal whenever sage starts up on a machine which does not
have any XCode toolchain at all.  It also avoids calling gcc in that
case since doing so posts a dialog on the user's screen asking if they
would like to install command line tools.  The dialog is a nice touch,
but should only appear if the user is actually *using* gcc, not as part
of every invocation of Sage, especially since XCode uses the standard
names.

<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->



### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [ ] The title is concise and informative.
- [ ] 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 and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#37886
Reported by: Marc Culler
Reviewer(s): John H. Palmieri, Marc Culler, Matthias Köppe, Volker Braun
vbraun pushed a commit to vbraun/sage that referenced this pull request May 18, 2024
    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

Update cryptographic hashes to use sha256 instead of sha1 due to
insecurity of sha1.

- Fixes sagemath#37691
- Fixes sagemath#37558, see also
sagemath#36677 (comment)

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [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. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->

- sagemath#37570
- sagemath#37249
- sagemath#37914
    
URL: sagemath#37726
Reported by: Faisal
Reviewer(s): Matthias Köppe, roed314
vbraun pushed a commit to vbraun/sage that referenced this pull request May 18, 2024
    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

Update cryptographic hashes to use sha256 instead of sha1 due to
insecurity of sha1.

- Fixes sagemath#37691
- Fixes sagemath#37558, see also
sagemath#36677 (comment)

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [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. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->

- sagemath#37570
- sagemath#37249
- sagemath#37914
    
URL: sagemath#37726
Reported by: Faisal
Reviewer(s): Matthias Köppe, roed314
vbraun pushed a commit to vbraun/sage that referenced this pull request May 18, 2024
…ite when run on systems with no toolchain

    
<!-- ^ Please provide a concise and informative title. -->

This PR does the following (after splitting out the Python upgrade to
[sagemath#37914](sagemath#37914)):
1. Fix a comment in the Makefile about how to use the DEBUG flag so it
will actually work.
3. Edit sage-env so that it does not print errors to stderr and/or open
a dialog whenever it is run on a system which has neither XCode nor
command line tools installed.
4. Remove (almost) all uses of rpath on macOS (see below).
5. Fix the primecountpy spkg, which was the only one which actually
needed an rpath in its python extension module.

**About rpaths on macOS**
Sage has been misusing rpaths on macOS for a very long time.  As far as
I can tell, this was an error based on the incorrect assumption that
MACH binaries use rpath in the same way as ELF binaries.

The rpath in an ELF binary provides a search path, beyond the default
search path, where the loader looks for shared libraries needed by the
binary.

A MACH binary specifies the dynamic libraries it needs with load
commands of type LC_LOAD_DYLIB.  The value of a LC_LOAD_DYLIB load
command is a path to a specific library.  There is one such load command
for each library that the binary needs.  The path is an absolute path by
default.  However, the path is allowed to begin with the substring
`@rpath`.  When the binary is loaded that substring is replaced by the
value of each LC_RPATH load command in the binary to produce a list of
absolute paths (or relative paths if the value of the LC_RPATH begins
with `@loader_path`).  Each of the paths on that list is tried as a
possible location of the needed dynamic library.

An obvious consequence of this design is that if none of the
LC_LOAD_DYLIB paths contains the string `@rpath` then there is no need
for any LC_RPATH commands, since they won't be used.  With one exception
(primecountpy) every MACH binary produced when building Sage had
absolute paths for its LC_LOAD_DYLIB load commands.  Nonetheless Sage
was setting the rpath to $SAGE_LOCAL/lib in all of these binaries.  In
fact, it was doing that multiple times, producing many duplicate rpaths
all of which were useless.  (And duplicate rpaths are now deprecated by
Apple's linker.)

This PR removes all of those bogus rpaths.

**About xcode-select**
The sage-env script was calling gcc to find the names of its linker and
archiver in the unlikely case that either of those had a non-standard
name, and it was also calling xcode-select as part of a check to see
whether the selected XCode toolchain had an ld-classic executable.  That
executable was added in XCode 15 as a new name for the old linker when a
new linker was added.  Apple made it possible to use the old linker by
passing the linker option -ld_classic to the gcc command.  The new
linker was buggy in the beginning and was unable to link openblas.  So
numpy and scipy use the old linker.  Sage also wanted to use it.  While
the new linker is probably working well enough now as a linker, it
generates a warning when it encounters duplicate libraries in a link
command.  It also appears that the new linker, or XCode 15 itself,
somehow *creates* duplicate libraries.  While the warnings should be
innocuous, they were causing Sage doctest failures.  So this PR provides
code which adds -Wl,-ld_classic to LDFLAGS in sage-env to replace the
analogous code which was already there.  But the new code is careful to
redirect the error messages to /dev/mull so they do not get printed on
the users terminal whenever sage starts up on a machine which does not
have any XCode toolchain at all.  It also avoids calling gcc in that
case since doing so posts a dialog on the user's screen asking if they
would like to install command line tools.  The dialog is a nice touch,
but should only appear if the user is actually *using* gcc, not as part
of every invocation of Sage, especially since XCode uses the standard
names.

<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->



### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [ ] The title is concise and informative.
- [ ] 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 and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#37886
Reported by: Marc Culler
Reviewer(s): John H. Palmieri, Marc Culler, Matthias Köppe, Volker Braun
vbraun pushed a commit to vbraun/sage that referenced this pull request May 24, 2024
    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

Update cryptographic hashes to use sha256 instead of sha1 due to
insecurity of sha1.

- Fixes sagemath#37691
- Fixes sagemath#37558, see also
sagemath#36677 (comment)

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [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. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->

- sagemath#37570
- sagemath#37249
- sagemath#37914
    
URL: sagemath#37726
Reported by: Faisal
Reviewer(s): Matthias Köppe, roed314
vbraun pushed a commit to vbraun/sage that referenced this pull request May 24, 2024
    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

Update cryptographic hashes to use sha256 instead of sha1 due to
insecurity of sha1.

- Fixes sagemath#37691
- Fixes sagemath#37558, see also
sagemath#36677 (comment)

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [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. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->

- sagemath#37570
- sagemath#37249
- sagemath#37914
    
URL: sagemath#37726
Reported by: Faisal
Reviewer(s): Matthias Köppe, roed314
vbraun pushed a commit to vbraun/sage that referenced this pull request May 24, 2024
…ite when run on systems with no toolchain

    
<!-- ^ Please provide a concise and informative title. -->

This PR does the following (after splitting out the Python upgrade to
[sagemath#37914](sagemath#37914)):
1. Fix a comment in the Makefile about how to use the DEBUG flag so it
will actually work.
3. Edit sage-env so that it does not print errors to stderr and/or open
a dialog whenever it is run on a system which has neither XCode nor
command line tools installed.
4. Remove (almost) all uses of rpath on macOS (see below).
5. Fix the primecountpy spkg, which was the only one which actually
needed an rpath in its python extension module.

**About rpaths on macOS**
Sage has been misusing rpaths on macOS for a very long time.  As far as
I can tell, this was an error based on the incorrect assumption that
MACH binaries use rpath in the same way as ELF binaries.

The rpath in an ELF binary provides a search path, beyond the default
search path, where the loader looks for shared libraries needed by the
binary.

A MACH binary specifies the dynamic libraries it needs with load
commands of type LC_LOAD_DYLIB.  The value of a LC_LOAD_DYLIB load
command is a path to a specific library.  There is one such load command
for each library that the binary needs.  The path is an absolute path by
default.  However, the path is allowed to begin with the substring
`@rpath`.  When the binary is loaded that substring is replaced by the
value of each LC_RPATH load command in the binary to produce a list of
absolute paths (or relative paths if the value of the LC_RPATH begins
with `@loader_path`).  Each of the paths on that list is tried as a
possible location of the needed dynamic library.

An obvious consequence of this design is that if none of the
LC_LOAD_DYLIB paths contains the string `@rpath` then there is no need
for any LC_RPATH commands, since they won't be used.  With one exception
(primecountpy) every MACH binary produced when building Sage had
absolute paths for its LC_LOAD_DYLIB load commands.  Nonetheless Sage
was setting the rpath to $SAGE_LOCAL/lib in all of these binaries.  In
fact, it was doing that multiple times, producing many duplicate rpaths
all of which were useless.  (And duplicate rpaths are now deprecated by
Apple's linker.)

This PR removes all of those bogus rpaths.

**About xcode-select**
The sage-env script was calling gcc to find the names of its linker and
archiver in the unlikely case that either of those had a non-standard
name, and it was also calling xcode-select as part of a check to see
whether the selected XCode toolchain had an ld-classic executable.  That
executable was added in XCode 15 as a new name for the old linker when a
new linker was added.  Apple made it possible to use the old linker by
passing the linker option -ld_classic to the gcc command.  The new
linker was buggy in the beginning and was unable to link openblas.  So
numpy and scipy use the old linker.  Sage also wanted to use it.  While
the new linker is probably working well enough now as a linker, it
generates a warning when it encounters duplicate libraries in a link
command.  It also appears that the new linker, or XCode 15 itself,
somehow *creates* duplicate libraries.  While the warnings should be
innocuous, they were causing Sage doctest failures.  So this PR provides
code which adds -Wl,-ld_classic to LDFLAGS in sage-env to replace the
analogous code which was already there.  But the new code is careful to
redirect the error messages to /dev/mull so they do not get printed on
the users terminal whenever sage starts up on a machine which does not
have any XCode toolchain at all.  It also avoids calling gcc in that
case since doing so posts a dialog on the user's screen asking if they
would like to install command line tools.  The dialog is a nice touch,
but should only appear if the user is actually *using* gcc, not as part
of every invocation of Sage, especially since XCode uses the standard
names.

<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->



### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [ ] The title is concise and informative.
- [ ] 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 and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#37886
Reported by: Marc Culler
Reviewer(s): John H. Palmieri, Marc Culler, Matthias Köppe, Volker Braun
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.

None yet

4 participants