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

ghostscript: fix install names on Darwin #358079

Merged
merged 1 commit into from
Nov 25, 2024

Conversation

carlocab
Copy link

Fixes #355377.

This should avoid the need to mess around with install_name_tool entirely. This mirrors what is done by Homebrew1 and MacPorts2.

This should also make the changes in #355853 and #357951 unnecessary.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

Footnotes

  1. https://github.com/Homebrew/homebrew-core/blob/5ca4f8ce766c69d49321fb7da1d297b8232f40cf/Formula/g/ghostscript.rb#L76

  2. https://github.com/macports/macports-ports/blob/d8a05520fa6a81fa5b0365068590aff184976b69/print/ghostscript/Portfile#L114

@carlocab
Copy link
Author

Also wasn't sure which branch this is supposed to target, so just went with master.

@NixOSInfra NixOSInfra added the 12. first-time contribution This PR is the author's first one; please be gentle! label Nov 22, 2024
@nolith
Copy link

nolith commented Nov 22, 2024

@carlocab I think you fixed this problem on the wrong branch, as far as I can see here on master the change I made in #357951 is already present. So the existing package derivation should work.

On the other hand, in 24.05 it does not work.

In the 24.05 branch, as far as I can see, there is extensive use of install_name_tool.

So I think this PR indeed improves the future compatibility of this derivation, but is not solving #355377

Fixes NixOS#355377.

This should avoid the need to mess around with `install_name_tool`
entirely. This mirrors what is done by Homebrew[^1] and MacPorts[^2].

This should also make the changes in NixOS#355853 and NixOS#357951 unnecessary.

[^1]: https://github.com/Homebrew/homebrew-core/blob/5ca4f8ce766c69d49321fb7da1d297b8232f40cf/Formula/g/ghostscript.rb#L76
[^2]: https://github.com/macports/macports-ports/blob/d8a05520fa6a81fa5b0365068590aff184976b69/print/ghostscript/Portfile#L114
@carlocab carlocab changed the base branch from master to staging-24.05 November 22, 2024 08:14
@carlocab
Copy link
Author

@carlocab I think you fixed this problem on the wrong branch, as far as I can see here on master the change I made in #357951 is already present. So the existing package derivation should work.

Thanks, switched the branch to staging-24.05.

@emilazy
Copy link
Member

emilazy commented Nov 25, 2024

We’re still doing this install_name_tool stuff unnecessarily on master, right? So I guess we’ll want a master/24.11 version of this PR afterwards, too, or am I misunderstanding?

@carlocab
Copy link
Author

We’re still doing this install_name_tool stuff unnecessarily on master, right?

Yes.

So I guess we’ll want a master/24.11 version of this PR afterwards, too, or am I misunderstanding?

I guess? I don't really understand the branching strategy here, sorry 😅 But if the install_name_tool stuff will stay unless there is a master and/or 24.11 version of this PR, then yes. Just let me know if they're needed -- I'll open the necessary PRs.

Copy link
Member

@emilazy emilazy left a comment

Choose a reason for hiding this comment

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

No worries, it’s pretty elaborate :) master is the new work‐in‐progress release that will become 25.05 in half a year; 24.11 is the upcoming release about to be cut, and 24.05 is the current stable release with about a month of support remaining. All of those have main branches for general work (release-YY.MM or master) and staging branches for things that cause mass rebuilds (staging-YY.MM or staging).

As I understand it, staging-24.05 is where ghostscript is broken and needs a fix. So that’s where this PR is critical. However, you can see that we are still doing slightly different install_name_tool stuff on master and release-24.11:

So we’d want a PR that applies the same basic change to staging (because of the number of rebuilds), and then we can automatically backport that to staging-24.11. (In general, we do a backporting workflow where we target the latest release with a problem, add a label for a backport to previous releases, and then get annoyed at the bot reporting a merge conflict and do it manually anyway. We don’t forward‐port changes, but in this case the backport would have had to have been manual anyway due to the divergence between the branches, so this is basically a preemptive backport.)

Hopefully that makes sense, and please ask if anything is still confusing! I’ve successfully built this on aarch64-darwin and aarch64-linux, so I’m going to merge this.

@emilazy emilazy merged commit df75a12 into NixOS:staging-24.05 Nov 25, 2024
33 of 34 checks passed
@carlocab carlocab deleted the fix-ghostscript branch November 25, 2024 15:06
@carlocab
Copy link
Author

Thanks; submitted against staging at #359025. I think staging is merged into master too, so I'm not sure whether a separate master PR is needed. If so let me know.

@emilazy
Copy link
Member

emilazy commented Nov 25, 2024

Right, the purpose of staging is to batch up mass‐rebuild changes to merge into staging-next, where we build everything and triage regressions, and then merge into master. (And also master merges back into staging-next merges back into staging, to keep them from drifting! The diagram in https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md#staging may make this more or less confusing.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment