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.

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

darwin: installBinaryPackage init and refactoring of darwin apps #293498

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
+123 −93
Draft

darwin: installBinaryPackage init and refactoring of darwin apps #293498

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5cf386b
darwin.installBinaryPackage: init
afh Mar 5, 2024
26d86e7
treewide: refactor to use darwin.installBinaryPackage (1password)
afh Mar 5, 2024
1624d72
treewide: refactor to use darwin.installBinaryPackage (wire-desktop)
afh Mar 5, 2024
abbdb9a
treewide: refactor to use darwin.installBinaryPackage (bartender)
afh Mar 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
52 changes: 27 additions & 25 deletions pkgs/applications/misc/1password/default.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{ lib, stdenv, fetchurl, fetchzip, autoPatchelfHook, installShellFiles, cpio, xar, _1password, testers }:
{ lib, stdenvNoCC, fetchurl, fetchzip, autoPatchelfHook, installShellFiles, _1password, testers, darwin }:

let
inherit (stdenv.hostPlatform) system;
inherit (stdenvNoCC.hostPlatform) system;
fetch = srcPlatform: hash: extension:
let
args = {
Expand All @@ -21,49 +21,36 @@ let
x86_64-darwin = aarch64-darwin;
};
platforms = builtins.attrNames sources;
mainProgram = "op";
in

stdenv.mkDerivation {
inherit pname version;
src =
if (builtins.elem system platforms) then
sources.${system}
else
throw "Source for ${pname} is not available for ${system}";

nativeBuildInputs = [ installShellFiles ] ++ lib.optional stdenv.isLinux autoPatchelfHook;

buildInputs = lib.optionals stdenv.isDarwin [ xar cpio ];

unpackPhase = lib.optionalString stdenv.isDarwin ''
xar -xf $src
zcat op.pkg/Payload | cpio -i
'';
nativeBuildInputs = [ installShellFiles ];

installPhase = ''
runHook preInstall
install -D ${mainProgram} $out/bin/${mainProgram}
install -D ${meta.mainProgram} $out/bin/${meta.mainProgram}
runHook postInstall
'';

postInstall = ''
HOME=$TMPDIR
installShellCompletion --cmd ${mainProgram} \
--bash <($out/bin/${mainProgram} completion bash) \
--fish <($out/bin/${mainProgram} completion fish) \
--zsh <($out/bin/${mainProgram} completion zsh)
installShellCompletion --cmd ${meta.mainProgram} \
--bash <($out/bin/${meta.mainProgram} completion bash) \
--fish <($out/bin/${meta.mainProgram} completion fish) \
--zsh <($out/bin/${meta.mainProgram} completion zsh)
'';

dontStrip = stdenv.isDarwin;

doInstallCheck = true;

installCheckPhase = ''
$out/bin/${mainProgram} --version
$out/bin/${meta.mainProgram} --version
'';

passthru.tests.version = testers.testVersion {
testers' = testers.testVersion {
package = _1password;
};

Expand All @@ -74,6 +61,21 @@ stdenv.mkDerivation {
maintainers = with maintainers; [ joelburget marsam ];
sourceProvenance = with sourceTypes; [ binaryNativeCode ];
license = licenses.unfree;
inherit mainProgram platforms;
mainProgram = "op";
inherit platforms;
};
}

linux = stdenvNoCC.mkDerivation {
inherit pname version src installPhase postInstall doInstallCheck installCheckPhase meta;
nativeBuildInputs = nativeBuildInputs ++ [ autoPatchelfHook ];
passthru.tests.version = testers';
};

darwin' = darwin.installBinaryPackage {
inherit pname version src nativeBuildInputs installPhase postInstall doInstallCheck installCheckPhase meta;
passthru.tests.version = testers';
};
in
if stdenvNoCC.isDarwin
then darwin'
else linux
46 changes: 8 additions & 38 deletions pkgs/applications/networking/instant-messengers/wire-desktop/default.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,23 +3,22 @@
, fetchurl
, makeDesktopItem
, makeWrapper
, stdenv
, stdenvNoCC
, lib
, udev
, wrapGAppsHook
, cpio
, xar
, libdbusmenu
, alsa-lib
, mesa
, nss
, nspr
, systemd
, darwin
}:

let

inherit (stdenv.hostPlatform) system;
inherit (stdenvNoCC.hostPlatform) system;

throwSystem = throw "Unsupported system: ${system}";

Expand Down Expand Up @@ -69,7 +68,7 @@ let
hydraPlatforms = [];
};

linux = stdenv.mkDerivation rec {
linux = stdenvNoCC.mkDerivation rec {
inherit pname version meta;

src = fetchurl {
Expand Down Expand Up @@ -143,47 +142,18 @@ let
'';
};

darwin = stdenv.mkDerivation {
darwin' = darwin.installBinaryPackage {
inherit pname version meta;

src = fetchurl {
url = "https://github.com/wireapp/wire-desktop/releases/download/macos%2F${version}/Wire.pkg";
inherit hash;
};

buildInputs = [
cpio
xar
];

unpackPhase = ''
runHook preUnpack

xar -xf $src
cd com.wearezeta.zclient.mac.pkg

runHook postUnpack
'';

buildPhase = ''
runHook preBuild

cat Payload | gunzip -dc | cpio -i

runHook postBuild
'';

installPhase = ''
runHook preInstall

mkdir -p $out/Applications
cp -r Wire.app $out/Applications

runHook postInstall
'';
appName = "Wire.app";
};

in
if stdenv.isDarwin
then darwin
if stdenvNoCC.isDarwin
then darwin'
else linux
39 changes: 11 additions & 28 deletions pkgs/os-specific/darwin/bartender/default.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,48 +1,31 @@
{ lib
, stdenvNoCC
, fetchurl
, _7zz
, darwin
}:

stdenvNoCC.mkDerivation (finalAttrs: {
darwin.installBinaryPackage rec {
Copy link
Contributor

Choose a reason for hiding this comment

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

thought it was preferred to use the finalAttrs pattern over rec, why switch?

Copy link
Contributor

Choose a reason for hiding this comment

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

Based on my understanding, mkDerivation takes either an attrset or a function (the finalAttrs pattern) as argument.

This PR introduces installBinaryPackage to simplify the process of packaging macOS apps and reduces the overuse of mkDerivation (code duplication in general). It takes an attrset as argument, i.e. the finalAttrs pattern is not applicable here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense, thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Based on my understanding, mkDerivation takes either an attrset or a function (the finalAttrs pattern) as argument.

This PR introduces installBinaryPackage to simplify the process of packaging macOS apps and reduces the overuse of mkDerivation (code duplication in general). It takes an attrset as argument, i.e. the finalAttrs pattern is not applicable here.

The reason why most build helpers in Nixpkgs don't support fixed-point arguments (finalAttrs: { ... }) yet, is because of the challenge to handle technical debts inside existing build helpers -- arguments that cannot be passed directly to stdenv.mkDerivation.

The goal of #234651 is to introduce a way (lib.extendMkDerivation) to handle such arguments and bring fixed-point arguments support to existing build helpers. Even then, there are still efforts to bring fixed-point arguments support to build helpers like buildRustPackage (#194475) and buildGoModule (#225051).

As a new build helper, installBinaryPackage could avoid such technical debt as a whole and support fixed-point attributes out of the box without the need of the WIP lib.extendMkDerivation. We just need to make sure that everything is passed properly into stdenv.mkDerivation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Update:

Sorry @stepbrobd for my misunderstanding your point. You're right about the current implementation of installBinaryPackage handling only a plain attribute set.

I just want to point out that such support is not impossible or unsuitable outside stdenv.mkDerivation. We only need to modify the implementation of installBinaryPackage a bit to support it.

pname = "bartender";
version = "5.0.49";

src = fetchurl {
name = "Bartender ${lib.versions.major finalAttrs.version}.dmg";
url = "https://www.macbartender.com/B2/updates/${builtins.replaceStrings [ "." ] [ "-" ] finalAttrs.version}/Bartender%20${lib.versions.major finalAttrs.version}.dmg";
url = "https://www.macbartender.com/B2/updates/${builtins.replaceStrings [ "." ] [ "-" ] version}/Bartender%20${lib.versions.major version}.dmg";
hash = "sha256-DOQLtdbwYFyRri3GBdjLfFNII65QJMvAQu9Be4ATBx0=";
};

dontPatch = true;
dontConfigure = true;
dontBuild = true;
dontFixup = true;

nativeBuildInputs = [ _7zz ];

sourceRoot = "Bartender ${lib.versions.major finalAttrs.version}.app";

installPhase = ''
runHook preInstall

mkdir -p "$out/Applications/${finalAttrs.sourceRoot}"
cp -R . "$out/Applications/${finalAttrs.sourceRoot}"

runHook postInstall
'';
appName = "Bartender ${lib.versions.major version}.app";

meta = with lib; {
description = "Take control of your menu bar";
longDescription = ''
Bartender is an award-winning app for macOS that superpowers your menu bar, giving you total control over your menu bar items, what's displayed, and when, with menu bar items only showing when you need them.
Bartender improves your workflow with quick reveal, search, custom hotkeys and triggers, and lots more.
Bartender is an award-winning app for macOS that superpowers your menu
bar, giving you total control over your menu bar items, what's displayed,
and when, with menu bar items only showing when you need them.
Bartender improves your workflow with quick reveal, search, custom
hotkeys and triggers, and lots more.
'';
homepage = "https://www.macbartender.com";
changelog = "https://www.macbartender.com/Bartender${lib.versions.major finalAttrs.version}/release_notes/";
changelog = "https://www.macbartender.com/Bartender${lib.versions.major version}/release_notes/";
license = with licenses; [ unfree ];
sourceProvenance = with sourceTypes; [ binaryNativeCode ];
maintainers = with maintainers; [ stepbrobd ];
platforms = [ "aarch64-darwin" "x86_64-darwin" ];
};
})
}
44 changes: 44 additions & 0 deletions pkgs/os-specific/darwin/install-binary-package/default.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
{ lib, stdenvNoCC, makeSetupHook, _7zz, libarchive }:

{ appName ? "."
, nativeBuildInputs ? []
, sourceRoot ? "."
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
, sourceRoot ? "."

If we do want one, it could be called subSourceRoot or something.

, ...
}@args:

let
unpackDmgPkg = makeSetupHook {
name = "unpack-dmg-pkg";
propagatedBuildInputs = [ _7zz libarchive ];
} ./unpack-dmg-pkg.sh;
in
stdenvNoCC.mkDerivation (args // {
Comment on lines +3 to +15
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{ appName ? "."
, nativeBuildInputs ? []
, sourceRoot ? "."
, ...
}@args:
let
unpackDmgPkg = makeSetupHook {
name = "unpack-dmg-pkg";
propagatedBuildInputs = [ _7zz libarchive ];
} ./unpack-dmg-pkg.sh;
in
stdenvNoCC.mkDerivation (args // {
let
unpackDmgPkg = makeSetupHook {
name = "unpack-dmg-pkg";
propagatedBuildInputs = [ _7zz libarchive ];
} ./unpack-dmg-pkg.sh;
in
fpArgs:
(stdenvNoCC.mkDerivation fpArgs).overrideAttrs (finalAttrs:
{ appName ? "."
, nativeBuildInputs ? []
, sourceRoot ? "."
, ...
}@args:
{

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for this, @ShamrockLee, I've included your suggestion locally, but need to refactor the treewide changes before pushing an update on this.
Also I'd like wait until sourceRoot conversation (see below) provides guidance on a more acceptable solution.

dontMakeSourcesWritable = args.dontMakeSourcesWritable or true;
dontPatch = args.dontPatch or true;
dontConfigure = args.dontConfigure or true;
dontBuild = args.dontBuild or true;
dontStrip = args.dontStrip or true;
dontFixup = args.dontFixup or true;
dontCheck = args.dontCheck or true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
dontCheck = args.dontCheck or true;

The attribute is called doCheck in stdenv.mkDerivation, and it's false by default.
We don't need to touch it, unless we need to use finalAttrs.doCheck.

If we do need finalAttrs.doCheck somewhere, though, we'll still have to do

  doCheck = args.doCheck or false;

to guarentee its availability. Attempt to determine if an attribute is available in finalAttrs will result in infinite recursion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For some reason I thought both existed, @ShamrockLee. Grep'ing through the packages around 80 use dontCheck (likely including some false positives). Is this something that could and should be cleaned up too at some point?

Copy link
Contributor

Choose a reason for hiding this comment

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

@afh This comment slipped out of my radar. Thank you for your patience.

For some reason I thought both existed, @ShamrockLee. Grep'ing through the packages around 80 use dontCheck (likely including some false positives). Is this something that could and should be cleaned up too at some point?

After some keyword searching inside VSCode, it seems most of the dontCheck uses refer to a Haskell trivial helper (function) haskell.lib.compose.dontCheck, which takes a derivation and returns one with { doCheck = false; } overriding.

Searching with dontCheck = returns only 7 matches. Two of which is the definition of haskell.lib.compose.dontCheck, and the other 5 are Python packages. Since pkgs/development/interpreters/python/mk-python-derivation.nix doesn't provide such implementation, they are likely mistakes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Once again an excellent analysis from you, @ShamrockLee 😃👍

pkgs/by-name/py/pyxel/package.nix seems to also use dontCheck and is built using buildPythonApplication (defined in pkgs/development/interpreters/python/python-packages-base.nix, which also does not seem to offer dontCheck).
Anything we can do to confirm that the dontCheck used in the python packages is indeed a mistake prior to creating a PR? Or should I (unless you prefer to do it yourself) create a PR to get more eyes on this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Anything we can do to confirm that the dontCheck used in the python packages is indeed a mistake prior to creating a PR?

The proof turns out to be quite simple:

nix-repl> pkgs = import ./. { }             
nix-repl> pkgs.pyxel
«derivation /nix/store/rr3d1w5j622sy9xw032wzp69dr8fz15j-pyxel-2.0.7.drv»

nix-repl> pkgs.pyxel.doInstallCheck
true

nix-repl> (pkgs.pyxel.overridePythonAttrs (_: { doCheck = false; }))
«derivation /nix/store/mycyaxdzwqgi6x0aylzmljh7zb0xr22w-pyxel-2.0.7.drv»

nix-repl> (pkgs.pyxel.overridePythonAttrs (_: { doCheck = false; })).doInstallCheck
false

Or should I (unless you prefer to do it yourself) create a PR to get more eyes on this?

It would be helpful for people who use and maintain these packages.

That said, I wonder whether those packages really need their install checks disabled if they build fine without. Feel free to tag the package maintainers in the PR description.

Copy link
Contributor Author

@afh afh Apr 29, 2024 •
edited

Choose a reason for hiding this comment

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

ℹ️ Here are the three PRs #307823(merged), #307835(merged), #307838 (draft) that remove dontCheck from the packages mentioned earlier. Some PRs also update and/or modernize some of the packages.

doInstallCheck = args.doInstallCheck or false;
Comment on lines +16 to +23
Copy link
Contributor Author

@afh afh Mar 5, 2024 •
edited

Choose a reason for hiding this comment

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

  • Is this the best way to set sensible defaults, but allow customisation for packages that need it?
  • Are there any other dont* flags that are relevant here?


sourceRoot = "${args.pname}/${sourceRoot}";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
sourceRoot = "${args.pname}/${sourceRoot}";
sourceRoot = args.sourceRoot or args.pname;

Just give it a sensible default.

(Replace args.pname with finalAttrs.pname if you would like to adopt the fixed-point-arguments style.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this might be a bit more involved than it seems and possibly a more elegant way exists to solve what the current approach solves.

Most of the binary sources unpack multiple files, a directory, or a one or more archive files (often named Payload). Yet some packages unpack a single binary, which made¹ the unpackPhase fail for me. Given the variety of binary source packages (dmg with file(s), pkg with file(s), dmg with archive(s), pkgs with archive(s)) it seems easier to unpack each "layer" and take it from there instead of trying to analyse what the "best" unpack strategy for a particular package is. Not that some packages also need multiple levels of unpacking, e.g. karabiner-elements.

So my idea was to simple put all unpacked files into a directory named args.pname to satisfy the directory-"requirement"—in which a directory must be present afterwards—of the unpackPhase.
So introducing subSourceRoot might not provide the solution that is needed…

Have I been able to explain my thinking around this well?
What are alternative approaches for this challenge?

────────────
¹ Maybe this has been addressed as I cannot not reproduce the issue; possibly my quick tests for this comment have been too simple to not trigger the error. I'll keep my 👀 peeled.

Copy link
Contributor

Choose a reason for hiding this comment

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

So my idea was to simple put all unpacked files into a directory named args.pname to satisfy the directory-"requirement"—in which a directory must be present afterwards—of the unpackPhase.
So introducing subSourceRoot might not provide the solution that is needed…

That makes sense. subSourceRoot doesn't seem to scratch the itch.

Then, how about setting sourceRoot = "${args.pname}" by default, and tell people to always set sourceRoot to "${pname}" or its sub-directory inside Nixpkgs manual or comment block?

The current implementation will result in developer setting sourceRoot = "subdir" and see sourceRoot is hello-bin/subdir in the build log. Setting sourceRoot = "${pname}/subdir" avoids such inconsistency.

From the search result inside the Nixpkgs code base with regular expression sourceRoot =.*sourceRoot, no build helpers provided in Nixpkgs change sourceRoot transparently like this. The only exception is buildIntegration defined locally inside a let ... in block.

I could only express my point of view instead of requesting a change, as I'm nither a committer nor a Darwin user. The design choice will be made by you and the committers.


nativeBuildInputs = [ unpackDmgPkg ] ++ nativeBuildInputs;

installPhase = args.installPhase or ''
runHook preInstall

mkdir -p $out/Applications
cp -R "${appName}" $out/Applications

runHook postInstall
'';

installCheckPhase = args.installCheckPhase or null;

meta = {
sourceProvenance = [ lib.sourceTypes.binaryNativeCode ];
platforms = lib.platforms.darwin;
} // (args.meta or {});
})
29 changes: 29 additions & 0 deletions pkgs/os-specific/darwin/install-binary-package/unpack-dmg-pkg.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
# shellcheck shell=bash
unpackCmdHooks+=(unpackDmgPkg)
unpackDmgPkg() {
case "$curSrc" in
*.dmg) unpackDmg "$curSrc";;
*.pkg) unpackPkg "$curSrc";;
*) return 1
esac
}

unpackDmg() {
_unpack "$1" "${2:-$pname}"
}

unpackPkg() {
_unpack "$1" "${2:-$pname}"
pushd "${2:-$pname}"
# Extract files from Payload or Payload~ file contained in given pkg if any
find . -name 'Payload*' -maxdepth 1 -print0 | xargs -0 -t -I % bsdtar -xf %
popd
}

_unpack() {
# Exclude */Applications files from extraction as they may
# contain a dangerous link path, causing 7zz to error.
# Exclude *com.apple.provenance xattr files from extraction as they
# break the an application's signature and notarization.
7zz x -bd -o"$2" -xr'!*/Applications' -xr'!*com.apple.provenance' "$1"
}
2 changes: 0 additions & 2 deletions pkgs/top-level/all-packages.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27323,8 +27323,6 @@ with pkgs;
ath9k-htc-blobless-firmware-unstable =
callPackage ../os-specific/linux/firmware/ath9k { enableUnstable = true; };

bartender = callPackage ../os-specific/darwin/bartender { };

b43Firmware_5_1_138 = callPackage ../os-specific/linux/firmware/b43-firmware/5.1.138.nix { };

b43Firmware_6_30_163_46 = callPackage ../os-specific/linux/firmware/b43-firmware/6.30.163.46.nix { };
Expand Down
4 changes: 4 additions & 0 deletions pkgs/top-level/darwin-packages.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,8 @@ impure-cmds // appleSourcePackages // chooseLibs // {
extraBuildInputs = [];
};

bartender = callPackage ../os-specific/darwin/bartender { };

binutils-unwrapped = callPackage ../os-specific/darwin/binutils {
inherit (pkgs) binutils-unwrapped;
inherit (pkgs.llvmPackages) llvm clang-unwrapped;
Expand Down Expand Up @@ -142,6 +144,8 @@ impure-cmds // appleSourcePackages // chooseLibs // {
propagatedBuildInputs = [ pkgs.darwin.print-reexports ];
} ../os-specific/darwin/print-reexports/setup-hook.sh;

installBinaryPackage = callPackage ../os-specific/darwin/install-binary-package { };

sigtool = callPackage ../os-specific/darwin/sigtool { };

signingUtils = callPackage ../os-specific/darwin/signing-utils { };
Expand Down