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

xar: 1.6.1 -> 498 #329721

Merged
merged 2 commits into from
Aug 27, 2024
Merged

xar: 1.6.1 -> 498 #329721

merged 2 commits into from
Aug 27, 2024

Conversation

tie
Copy link
Member

@tie tie commented Jul 24, 2024

Description of changes

This change switches the xar package from unmaintained fork of the original project to the Apple Open Source tarball. We also add some patches from Debian/Gentoo on top, but otherwise the patch set is original and fixes some longstanding bugs (i.e. I’ve spent a considerable amount of time to ensure that all tests provided in the tarball are passing). If merged, Nixpkgs would probably be the only distribution that actually runs xar tests…

See also https://repology.org/project/xar/versions

Things done

  • Built on → for platform(s)
    • x86_64-linux
    • aarch64-linux
    • aarch64-linux → aarch64-linux (pkgsMusl)
    • aarch64-linux → aarch64-linux (pkgsStatic)
    • aarch64-linux → x86_64-linux (pkgsCross.gnu64)
    • 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/)
    Built darwin.apple_sdk.MacOSX-SDK.
  • 24.11 Release Notes (or backporting 23.11 and 24.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.

@tie
Copy link
Member Author

tie commented Jul 28, 2024

Mostly ready for review, but there are some test failures because of mismatching mtime in pkgsMusl.xar.tests.integrationTest (that is, with musl in particular).

@tie tie marked this pull request as ready for review July 28, 2024 20:33
@tie tie requested a review from alyssais July 28, 2024 20:34
@github-actions github-actions bot removed the 6.topic: stdenv Standard environment label Aug 23, 2024
@tie tie force-pushed the xar-apple-oss branch 3 times, most recently from 02e9c01 to a12afe4 Compare August 25, 2024 12:51
@tie
Copy link
Member Author

tie commented Aug 25, 2024

Rebased, addressed previous review comments, should be ready for another round of reviews.

@tie tie requested review from reckenrode and emilazy August 25, 2024 13:09
This change switches the xar package from unmaintained fork of the
original project to the Apple Open Source tarball. See also
https://repology.org/project/xar/versions

Since the package is essentially rewritten from scratch, we take an
opportunity and move it to pkgs/by-name/xa/xar (formatted with nixfmt).

We also remove Windows from the supported platforms because even before
this change pkgsCross.mingwW64.xar failed with
xar> configure: error: can not detect the size of your system's uid_t type
@tie tie requested a review from philiptaron as a code owner August 25, 2024 15:10
@github-actions github-actions bot added the 6.topic: stdenv Standard environment label Aug 25, 2024
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.

Diff looks good, reproduced the build and tests (pure and impure) on aarch64-linux and aarch64-linux. Happy to merge once @reckenrode has taken another look and once someone reproduces the build on x86-64.

@reckenrode
Copy link
Contributor

I haven’t run the tests or tried to build it, but the changes look good to me.

Copy link
Contributor

@JohnRTitor JohnRTitor left a comment

Choose a reason for hiding this comment

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

Built on x64-linux.

@emilazy
Copy link
Member

emilazy commented Aug 26, 2024

Lots of warnings like this on x86_64-darwin:

xar> src/xar.c:810:23: warning: 'xar_get_safe_path' is only available on macOS 12.0 or newer [-Wunguarded-availability-new]
xar>                         const char *file = xar_get_safe_path(f);
xar>                                            ^~~~~~~~~~~~~~~~~
xar> include/xar.h:269:7: note: 'xar_get_safe_path' has been marked as being introduced in macOS 12.0 here, but the deployment target is macOS 10.12.0
xar> char *xar_get_safe_path(xar_file_t f) API_AVAILABLE(macos(12.0));
xar>       ^
xar> src/xar.c:810:23: note: enclose 'xar_get_safe_path' in a __builtin_available check to silence this warning
xar>                         const char *file = xar_get_safe_path(f);
xar>                                            ^~~~~~~~~~~~~~~~~

I can’t tell if this is using a system library that actually won’t be available, or if the xar code itself is defining that function so it’ll be okay?

@reckenrode
Copy link
Contributor

It’s defined at https://github.com/apple-oss-distributions/xar/blob/eaeb03d085970f94938dc6785cd86d22eaa54b47/xar/include/xar.h.in#L268. The availability annotation could be removed since nixpkgs will be shipping the same dylib regardless of deployment target.

@emilazy
Copy link
Member

emilazy commented Aug 27, 2024

Seems reasonable. It shouldn’t cause anything worse than warnings to leave it in, though, right?

@reckenrode
Copy link
Contributor

Probably shouldn’t cause any issues other than warnings unless one uses -Werror.

@emilazy
Copy link
Member

emilazy commented Aug 27, 2024

Let’s merge this as‐is for now and we can make a decision about whether to patch the availability stuff out of the headers in another PR. Thank you for the great work as always, @tie!

@emilazy emilazy merged commit e1c3167 into NixOS:staging Aug 27, 2024
25 of 26 checks passed
@tie tie deleted the xar-apple-oss branch August 27, 2024 07:12
emilazy added a commit to emilazy/nixpkgs that referenced this pull request Sep 4, 2024
This change, while fine in isolation, breaks evaluation in combination
with <NixOS#329721>, as `xar` depends
on `e2fsprogs` which now depends on `macfuse-stubs` which depends on
`xar`. This broke `staging-next`.

A couple possible solutions are to disable the `e2fsprogs` dependency
in the version of `xar` used for the bootstrap, or to build
`macfuse-stubs` from source to avoid the `xar` dependency.

This reverts commit 0dfc820.
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.

5 participants