Skip to content

move zlib to prereqs, remove zlib spkg #40460

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
Merged
merged 3 commits into from
Aug 27, 2025
Merged

move zlib to prereqs, remove zlib spkg #40460

merged 3 commits into from
Aug 27, 2025

Conversation

dimpase
Copy link
Member

@dimpase dimpase commented Jul 21, 2025

move zlib to prereqs, remove zlib spkg. Provide macos-specific zlib.pc which is used to point at the system zlib,
which is always there.

📝 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

@dimpase dimpase requested a review from vbraun July 21, 2025 05:05
@dimpase
Copy link
Member Author

dimpase commented Jul 21, 2025

let's just drop zlib - macOS provides a perfectly functional zlib, what's missing is zlib.pc, but this is fixed here.

@dimpase dimpase mentioned this pull request Jul 21, 2025
Merged
@dimpase dimpase requested a review from orlitzky July 21, 2025 05:08
@dimpase dimpase requested a review from tobiasdiez August 2, 2025 19:10
@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 2, 2025 •
edited
Loading

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

tobiasdiez
tobiasdiez approved these changes Aug 3, 2025
View reviewed changes
Copy link
Contributor

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

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

I don't have a mac, so cannot test it there. But codewise it looks good, and I trust you (and CI) that it's okay.

vbraun pushed a commit to vbraun/sage that referenced this pull request Aug 10, 2025
d660ea1 
sagemathgh-40460: move zlib to prereqs, remove zlib spkg
    
move zlib to prereqs, remove zlib spkg. Provide macos-specific zlib.pc
which is used to point at the system zlib,
which is always there.

### 📝 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#40460
Reported by: Dima Pasechnik
Reviewer(s): Tobias Diez
vbraun pushed a commit to vbraun/sage that referenced this pull request Aug 12, 2025
170e1b6 
sagemathgh-40460: move zlib to prereqs, remove zlib spkg
    
move zlib to prereqs, remove zlib spkg. Provide macos-specific zlib.pc
which is used to point at the system zlib,
which is always there.

### 📝 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#40460
Reported by: Dima Pasechnik
Reviewer(s): Tobias Diez
vbraun pushed a commit to vbraun/sage that referenced this pull request Aug 13, 2025
02a7859 
sagemathgh-40460: move zlib to prereqs, remove zlib spkg
    
move zlib to prereqs, remove zlib spkg. Provide macos-specific zlib.pc
which is used to point at the system zlib,
which is always there.

### 📝 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#40460
Reported by: Dima Pasechnik
Reviewer(s): Tobias Diez
vbraun pushed a commit to vbraun/sage that referenced this pull request Aug 14, 2025
1b00148 
sagemathgh-40460: move zlib to prereqs, remove zlib spkg
    
move zlib to prereqs, remove zlib spkg. Provide macos-specific zlib.pc
which is used to point at the system zlib,
which is always there.

### 📝 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#40460
Reported by: Dima Pasechnik
Reviewer(s): Tobias Diez
@vbraun
Copy link
Member

vbraun commented Aug 15, 2025

On macos:

[sagelib-10.7] [spkg-install]   pkgconfig.pkgconfig.PackageNotFoundError: libpng not found

@dimpase
Copy link
Member Author

dimpase commented Aug 15, 2025

@vbraun - at what moment does this happen? It's too cryptic one-liner.

@vbraun
Copy link
Member

vbraun commented Aug 15, 2025

http://build.sagemath.org/#/builders/71/builds/8

@tobiasdiez
Copy link
Contributor

That error looks quite unrelated to the changes in this PR...

May I suggest to just delete the cython_alias business? It's bound to be unstable, and may pick up different compilation flags than what is used for sage's compilation (with meson).

@dimpase
Copy link
Member Author

dimpase commented Aug 15, 2025

@vbraun - my own testing wasn't correct, sorry.
This error should be fixed by the latest commit, 428a388.

@tobiasdiez - libpng is apparently the 1st python module to be build with calling pkgconfig. So with PKG_CONFIG_PATH shot, it all broke.

vbraun pushed a commit to vbraun/sage that referenced this pull request Aug 17, 2025
sagemathgh-40460: move zlib to prereqs, remove zlib spkg
a7ae8a2 
move zlib to prereqs, remove zlib spkg. Provide macos-specific zlib.pc
which is used to point at the system zlib,
which is always there.

### 📝 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#40460
Reported by: Dima Pasechnik
Reviewer(s): Tobias Diez
vbraun pushed a commit to vbraun/sage that referenced this pull request Aug 17, 2025
6ab19e0 
sagemathgh-40460: move zlib to prereqs, remove zlib spkg
    
move zlib to prereqs, remove zlib spkg. Provide macos-specific zlib.pc
which is used to point at the system zlib,
which is always there.

### 📝 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#40460
Reported by: Dima Pasechnik
Reviewer(s): Tobias Diez
vbraun pushed a commit to vbraun/sage that referenced this pull request Aug 21, 2025
5e226ef 
sagemathgh-40460: move zlib to prereqs, remove zlib spkg
    
move zlib to prereqs, remove zlib spkg. Provide macos-specific zlib.pc
which is used to point at the system zlib,
which is always there.

### 📝 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#40460
Reported by: Dima Pasechnik
Reviewer(s): Tobias Diez
@dimpase dimpase mentioned this pull request Aug 25, 2025
Open
5 tasks
@vbraun vbraun merged commit fcc7a8c into sagemath:develop Aug 27, 2025
30 of 35 checks passed
@tobiasdiez
Copy link
Contributor

The docker build fails after this PR: https://github.com/sagemath/sage/actions/runs/17272511489/job/49020811731#step:8:11361

could you have a look? probably just doesn't install the prerequisites there.

@dimpase dimpase mentioned this pull request Aug 28, 2025
Merged
5 tasks
@dimpase
Copy link
Member Author

dimpase commented Aug 28, 2025 •
edited
Loading

The docker build fails after this PR: https://github.com/sagemath/sage/actions/runs/17272511489/job/49020811731#step:8:11361

could you have a look? probably just doesn't install the prerequisites there.

yes, I guess this is it. in Dockerfile one does not install _prereqs, but does some manual lists of packages, I don't know why. While removing pkgconf I did

a53a2f4fd0e8 (Dima Pasechnik  2025-07-15 21:13:36 -0500  93)     && apt-get -qq install -y --no-install-recommends gfortran gcc g++ libstdc++-10-dev sudo openssl bzip2 libbz2-dev pkg-config

but forgot about it.

To be fixed by #40708

@dimpase dimpase deleted the removezlib branch August 28, 2025 02:59
vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 4, 2025
1cfd52c 
sagemathgh-40708: make sure _prereq etc are installed in the docker images
    
<!-- ^ 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". -->

This is a follow-up to sagemath#40460

Docker images need to get _prereqs installed, as noticed in
sagemath#40460 (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 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#40708
Reported by: Dima Pasechnik
Reviewer(s): Sebastian Oehms, Tobias Diez
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vbraun vbraun Awaiting requested review from vbraun

@orlitzky orlitzky Awaiting requested review from orlitzky

1 more reviewer

@tobiasdiez tobiasdiez tobiasdiez approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dimpase @vbraun @tobiasdiez