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

igir: 2.11.0 -> 3.0.1-unstable-2024-09-27, adding support for darwin #372184

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

docbobo
Copy link
Contributor

@docbobo docbobo commented Jan 8, 2025

Pronounced "eager," Igir is a zero-setup ROM collection manager that sorts, filters, extracts or archives, patches, and reports on collections of any size on any OS.

This PR achieves two things that are described in more detail below: upgrading to version 3.0.1, and enabling platform support for Darwin.

2.11.0 -> 3.0.1-unstable-2024-09-27

Version 2.11.0 of igir already contains some pre-built binaries that require their dependencies to be updated via autoPatchelfHook. Version 3.0.0 (and thus 3.0.1) adds two more binaries that require this treatment: chdman and maxcso.

During testing of the proposed changes it was determined that the build for aarch64-linux did not include the maxcso binary. Further investigation showed that this is an upstream problem: a subsequent commit to v3.0.1 updates the maxcso to a version that fixes "Linux x86 and ARM64".

As a result, this PR picks up a commit for igir that comes shortly after the v3.0.1 tag. Changes between the two relevant commits are minor dependency updates, mostly for development packages and documentation.

Support for Darwin

Similar to Linux, the Darwin build requires patching of binaries since the bundled ones are compiled against homebrew libraries found under /opt/homebrew. However, no hook comparable to autoPatchelfHook seems to exist (fixDarwinDylibNames does not apply in this scenario) at this point in time, and patching is done "manually" via install_name_tool on a case by case basis.

Tests

Since the whole process of manually patching binaries is somewhat more fragile than using autoPatchelfHook, this PR also introduces two test cases that execute chdman and maxcso to determine if they start correctly. As a nice side-effect, the maxcso test case also breaks with the stock v3.0.1 because the maxcso binary is missing there (at least on aarch64-linux).

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.

pkgs/by-name/ig/igir/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ig/igir/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ig/igir/package.nix Outdated Show resolved Hide resolved
@eljamm
Copy link
Contributor

eljamm commented Jan 8, 2025

Please format this with nixfmt-rfc-style.

pkgs/by-name/ig/igir/package.nix Show resolved Hide resolved
pkgs/by-name/ig/igir/package.nix Outdated Show resolved Hide resolved
@eljamm
Copy link
Contributor

eljamm commented Jan 9, 2025

Looks good to me. @docbobo, can you squash the commits with git rebase -i?

@docbobo docbobo force-pushed the docbobo/igir-darwin-final branch from 0b05804 to 0e22b0f Compare January 9, 2025 09:39
@ofborg ofborg bot added the 6.topic: darwin Running or building packages on Darwin label Jan 9, 2025
Copy link
Contributor

@eljamm eljamm left a comment

Choose a reason for hiding this comment

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

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 372184


x86_64-linux

✅ 1 package built:
  • igir

x86_64-darwin

✅ 1 package built:
  • igir

aarch64-darwin

✅ 1 package built:
  • igir

@eljamm eljamm added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Jan 9, 2025
@keenanweaver
Copy link
Member

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 372184


x86_64-linux

✅ 1 package built:
  • igir

@keenanweaver keenanweaver added 12.approvals: 2 This PR was reviewed and approved by two reputable people and removed 12.approvals: 1 This PR was reviewed and approved by one reputable person labels Jan 9, 2025
@docbobo
Copy link
Contributor Author

docbobo commented Jan 10, 2025

Not sure how many approvals this needs, but can you wait with merging this (in case 2 is already sufficient)? I just noticed something that should be fixed.

@docbobo
Copy link
Contributor Author

docbobo commented Jan 11, 2025

Just to give a quick update: I realized that there is another binary that requires patchelf/install_name_tool patching: maxcso. While the patching itself is no big deal, I noticed that the binary itself seems to be missing on linux-aarch64. It is present on Darwin and I also believe that it's present on Linux-x86_64 - its presence was probably the reason for the build failure that was observed before.

After some digging and banging my head against the wall, I figured out that this is fixed in HEAD by picking up a slightly newer version of maxcso-js. According to the commit log, this version fixes "Linux x86 and ARM64".

At this point, the only option to get v3.0.1 consistently running on all platforms is to cherry-pick the updated maxcso-js which I am doing with a patch to package-lock.json and package.json. When igir gets a newer version in the future, that patch needs to be removed.

In order to ensure future correctness, I am also including tests.

@docbobo
Copy link
Contributor Author

docbobo commented Jan 11, 2025

I'll happily squash into one commit at the end, but for now I'd like you to see the difference between now and the previous state.

@wegank wegank removed the 12.approvals: 2 This PR was reviewed and approved by two reputable people label Jan 11, 2025
@docbobo
Copy link
Contributor Author

docbobo commented Jan 11, 2025

BTW, I could've just picked a newer igir commit, the one that introduces the update to v0.1380.8, but that would've included a few more dependency updates and I wanted to keep the difference to the tagged version as minimal as possible.

@github-actions github-actions bot added the 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` label Jan 12, 2025
@docbobo docbobo force-pushed the docbobo/igir-darwin-final branch 2 times, most recently from bf9e823 to b51aeeb Compare January 12, 2025 06:51
Copy link
Member

@keenanweaver keenanweaver left a comment

Choose a reason for hiding this comment

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

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 372184


x86_64-linux

✅ 1 package built:
  • igir

@keenanweaver keenanweaver added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Jan 12, 2025
@wegank wegank added 12.approvals: 2 This PR was reviewed and approved by two reputable people and removed 12.approvals: 1 This PR was reviewed and approved by one reputable person labels Jan 12, 2025
pkgs/by-name/ig/igir/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ig/igir/package.nix Show resolved Hide resolved
@eljamm eljamm added 12.approvals: 1 This PR was reviewed and approved by one reputable person and removed 12.approvals: 2 This PR was reviewed and approved by two reputable people labels Jan 12, 2025
@docbobo docbobo force-pushed the docbobo/igir-darwin-final branch from b51aeeb to 952874e Compare January 12, 2025 19:50
pkgs/by-name/ig/igir/package.nix Show resolved Hide resolved
pkgs/by-name/ig/igir/package.nix Outdated Show resolved Hide resolved
@docbobo docbobo force-pushed the docbobo/igir-darwin-final branch from 952874e to 2a7f376 Compare January 12, 2025 19:55
@eljamm eljamm removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Jan 12, 2025
@docbobo
Copy link
Contributor Author

docbobo commented Jan 12, 2025

Okay, so I guess this has reached its final state for now then?

@eljamm
Copy link
Contributor

eljamm commented Jan 12, 2025

Yes, but it would be great if you can also change the commit and PR titles and use the unstable version.

@eljamm
Copy link
Contributor

eljamm commented Jan 12, 2025

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 372184


x86_64-linux

✅ 1 package built:
  • igir

x86_64-darwin

✅ 1 package built:
  • igir

aarch64-darwin

✅ 1 package built:
  • igir

@docbobo docbobo force-pushed the docbobo/igir-darwin-final branch from 2a7f376 to af78e38 Compare January 12, 2025 21:29
@docbobo docbobo changed the title igir: 2.11.0 -> 3.0.1, adding support for darwin igir: 2.11.0 -> 3.0.1-unstable-2024-09-27, adding support for darwin Jan 12, 2025
Copy link
Contributor

@eljamm eljamm left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks a lot for your efforts and for maintaining this!

@eljamm eljamm added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Jan 12, 2025
@keenanweaver keenanweaver added 12.approvals: 2 This PR was reviewed and approved by two reputable people and removed 12.approvals: 1 This PR was reviewed and approved by one reputable person labels Jan 13, 2025
@docbobo
Copy link
Contributor Author

docbobo commented Jan 13, 2025

So what's needed to get this over the actual finish line (aka. merge)? More approvals? Just curious.

@keenanweaver
Copy link
Member

Time, patience, and the attention of a committer. More info here. It's a pain point that hopefully will be resolved in time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: darwin Running or building packages on Darwin 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 12.approvals: 2 This PR was reviewed and approved by two reputable people
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants