darwin: installBinaryPackage init and refactoring of darwin apps #293498
Conversation
| 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; | ||
| doInstallCheck = args.doInstallCheck or false; |
There was a problem hiding this comment.
- 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?
There was a problem hiding this comment.
Does this implementation allow for enough flexibility for uncommon use-cases (see karabiner-elements) while still making "good" assumptions about the general layout/content of DMGs and PKGs, e.g. unpack any Payload* using bsdtar contained in a .pkg file?
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/proposal-for-installing-prebuilt-binaries-on-darwin/40843/1 |
afh
requested review from
emilytrau,
Enzime and
DataHearth
and removed request for
emilytrau and
Enzime
|
I like the abstraction made by the dynamic unpack hook for DMG and PKG. Package complexity will drop (IMO) and package's lines reduced by a good margin. But I think we should make sure every possible option is covered. As for some specific behavior, this might even add more complexity inside the package and possibly inside the hook. If this solution is chosen, we should create a detailed documentation to avoid looking everywhere to find options, default behaviors, etc. Love it 👍 |
|
Thank you for taking the time to have a look at this and your comment, @DataHearth, very much appreciated! In your mind what is a good way to ensure that "every possible option is covered"? My first approach would be to refactor the packages in Where would be a good place for such documentation? Somewhere in |
|
Looking at |
afh
force-pushed
the
init-darwin-install-binary
branch
from
38620a5
to
9dd6490
Compare
|
I love the idea of having a unified way to package binaries for MacOS. Regarding the treewide change,
|
|
Thanks for your input, @ShamrockLee, much appreciated!
Any opinion or advice you may have on the "💭 A Few Thoughts" I listed in the initial description? |
|
Hey! Regarding creating a fetcher, I'm not a big fan of. As the "universal" way seems to fetch then unpack for most scenarios, I would keep it as a new package helper. I think the best way to find out if you missed something, it's to check other packages which are using an unpack like I agree with @ShamrockLee , I think this would me much easier to review for a code maintainer with only 1 or 2 packages as examples. |
|
Thanks for the feedback, @DataHearth, that's helpful. I'll do some code level analysis and prepare some notes. In addition to that I'll split this PR into two: one for the install binary package changes and one for the treewide package changes (with a possible follow-up). |
afh
force-pushed
the
init-darwin-install-binary
branch
from
9dd6490
to
c424494
Compare
|
As requested this PR now affects fewer files and the majority of the actual package rewrites to use the changes proposed in this PR will be in #293961. |
afh
force-pushed
the
init-darwin-install-binary
branch
from
c424494
to
abbdb9a
Compare
| }: | ||
|
|
||
| stdenvNoCC.mkDerivation (finalAttrs: { | ||
| darwin.installBinaryPackage rec { |
There was a problem hiding this comment.
thought it was preferred to use the finalAttrs pattern over rec, why switch?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Based on my understanding,
mkDerivationtakes either an attrset or a function (thefinalAttrspattern) as argument.This PR introduces
installBinaryPackageto simplify the process of packaging macOS apps and reduces the overuse ofmkDerivation(code duplication in general). It takes an attrset as argument, i.e. thefinalAttrspattern 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.
There was a problem hiding this comment.
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.
| { appName ? "." | ||
| , nativeBuildInputs ? [] | ||
| , sourceRoot ? "." | ||
| , ... | ||
| }@args: | ||
|
|
||
| let | ||
| unpackDmgPkg = makeSetupHook { | ||
| name = "unpack-dmg-pkg"; | ||
| propagatedBuildInputs = [ _7zz libarchive ]; | ||
| } ./unpack-dmg-pkg.sh; | ||
| in | ||
| stdenvNoCC.mkDerivation (args // { |
There was a problem hiding this comment.
| { 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: | |
| { |
There was a problem hiding this comment.
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.
| dontBuild = args.dontBuild or true; | ||
| dontStrip = args.dontStrip or true; | ||
| dontFixup = args.dontFixup or true; | ||
| dontCheck = args.dontCheck or true; |
There was a problem hiding this comment.
| 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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
sourceRoot corresponds to the shell variable/environment variable $sourceRoot inside the build script. It might be surprising if sourceRoot gets implicitly prefixed.
Developers who set sourceRoot is responsible to make sure it goes right, and we just need to document how it should be set. The documentation can be placed under the Languages and Frameworks chapter of Nixpkgs Manual.
Such input argument is also an example of "arguments that cannot be passed to stdenv.mkDeriaviton". Such arguments are not only the main obstacle of adding fixed-point arguments support, but also the motivation behind custom overriders like overridePythonAttrs, overrideLispAttrs and overrideNimAttrs, despite all the issues they have.
If it is necessary to specify the sub-path through an attribute, I hope it could be something like subSourceRoot. If so, we could specify
inherit subSourceRoot; # Simpler version of subSourceRoot = args.subSourceRoot or "."
sourceRoot = finalAttrs.pname
+ lib.optionalString (finalAttrs.subSourceRoot != ".") "/${finalAttrs.subSourceRoot}";|
|
||
| { appName ? "." | ||
| , nativeBuildInputs ? [] | ||
| , sourceRoot ? "." |
There was a problem hiding this comment.
| , sourceRoot ? "." |
If we do want one, it could be called subSourceRoot or something.
| dontCheck = args.dontCheck or true; | ||
| doInstallCheck = args.doInstallCheck or false; | ||
|
|
||
| sourceRoot = "${args.pname}/${sourceRoot}"; |
There was a problem hiding this comment.
| 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.)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
So my idea was to simple put all unpacked files into a directory named
args.pnameto satisfy the directory-"requirement"—in which a directory must be present afterwards—of theunpackPhase.
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.
|
Wow! Thanks for your detailed comments and suggestions, @ShamrockLee, very much appreciated!! I'm a bit busy until the middle of calendar week 13, so please allow me some time to get back to you on this 🙂 |
Description of changes
darwin.installBinaryPackageunifying and simplifying how binary darwin applications are installed.darwin.installBinaryPackagepre- andpost-hooks are runNOTA BENE: This is a draft PR to get the conversation going on whether this is headed into the right direction and folks see this as beneficial and helpful.
💭 A Few Thoughts
../os-specific/darwin/packages declared inpkgs/top-level/all-packages.nixdo folks agree that these are better declared inpkgs/top-level/darwin-packages.nix?os-specific/darwinpackages fromall-packages.nixtodarwin-packages.nixif the general sentiment is positive on the previous question.grep os-specific/darwin pkgs/top-level/all-packages.nix | awk -F'[=]' '/ *#/{next} {print $1}'undmgor_7zzas theirnativeBuildInputto unpack a prebuilt darwin binary, if this PR is generally viewed positively.nix run nixpkgs#silver-searcher -- -G '\.nix' -l '(undmg|_7zz)' | rev | cut -d/ -f2 | revdarwin.installBinaryPackageand learn about its details?pkgs/os-specific/install-binary-package/*ℹ️ All
treewide: refactor to use darwin.installBinaryPackage*commits will be squashed into one. For now it's easier for me to work on this with individual commits.Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.