signal-desktop: init darwin at 7.5.1 #286188
Conversation
ofborg
bot
requested review from
urandom2,
Mic92,
bkchr,
equirosa and
eclairevoyant
|
You can test the update script by first installing Let me know if this works for you and than I can merge this PR. |
|
Checking back in on this PR, I have rebased my changes onto master and did a small refactor. I have also tested the update script to ensure it works. |
pkgs/applications/networking/instant-messengers/signal-desktop/default.nix
Outdated
Show resolved
Hide resolved
pkgs/applications/networking/instant-messengers/signal-desktop/default.nix
Outdated
Show resolved
Hide resolved
|
Please rebase and reword your commit to |
|
Ah, that's my bad. Nothing a good 'ol |
| else callPackage ./signal-desktop.nix { }; | ||
| signal-desktop-beta = callPackage ./signal-desktop-beta.nix{ }; | ||
| { targetPlatform, callPackage }: { | ||
| signal-desktop = callPackage ./signal-desktop-${targetPlatform.system}.nix { }; |
There was a problem hiding this comment.
Mhm. This will cause eval errors in case we have a new unsupported platform.
This needs to be fixed. signal-desktop should always point to some derivation even if this derivation doesn't build on the current platform.
There was a problem hiding this comment.
How do you suggest it to be fixed? Perhaps we could pick a default derivation to fall back onto if one doesn't exist for an unsupported platform.
There was a problem hiding this comment.
Maybe builtins.pathExists with a fallback to x86_64-linux? But otherwise the previous if statement was also good enough, easier to read than some other complex logic.
There was a problem hiding this comment.
Perhaps we could do something like this?
| signal-desktop = callPackage ./signal-desktop-${targetPlatform.system}.nix { }; | |
| signal-desktop = | |
| let | |
| default = callPackage ./signal-desktop-x86_64-linux.nix { }; | |
| supportedPlatforms = default.meta.platforms; | |
| inherit (targetPlatform) system; | |
| in | |
| if builtins.elem system supportedPlatforms | |
| then callPackage ./signal-desktop-${system}.nix { } | |
| else default; |
Or we could use builtins.pathExists like this
| signal-desktop = callPackage ./signal-desktop-${targetPlatform.system}.nix { }; | |
| signal-desktop = | |
| let | |
| path = ./signal-desktop-${targetPlatform.system}.nix; | |
| default = ./signal-desktop-x86_64-linux.nix; | |
| in | |
| if builtins.pathExists path | |
| then callPackage path { } | |
| else callPackage default { }; |
I think I prefer the latter. Evaluating meta in the first one would work but it's harder to follow.
There was a problem hiding this comment.
@Mic92 Wouldn't it be better to just throw and say your platform is not supported? Since there isn't any guarantee the x86_64-linux will work properly on a new platform either?
There was a problem hiding this comment.
We already have quite a few eval errors here and we should not keep adding to it: https://hydra.nixos.org/jobset/nixpkgs/trunk#tabs-errors
There was a problem hiding this comment.
I think a throw statement is not quite same as saying that the platform is not supported in meta.platforms semantically speaking.
You're right, although what now? Do we default to building x86_64-linux? Wouldn't it be better to mark anything that isn't in meta.platforms as broken so we can entirely skip making a derivation and save resources?
Also, if we were to default to building for x86_64-linux, wouldn't it be an issue if we could build it on another platform but not run it?
There was a problem hiding this comment.
According to @wegank, targePlatform is only required when building a compiler #310053 (comment), see https://nix.dev/tutorials/cross-compilation.html#platforms.
There was a problem hiding this comment.
and run the final executable on the target platform.
If hostPlatform were used and you were to cross compile architectures, wouldn't you end up with a binary that's incompatible?
There was a problem hiding this comment.
The cross-compiled program, in that example the compiler, still runs on the host platform (and produces executables that run on the target platform).
|
Looking again at the commits, you're breaking the Commit Conventions.
Just squash everything into first commit (89b5d0b40b61c6dcf9a75b4532b7e229b3a9a24a) and then reword it to |
juuyokka
changed the title
There was a problem hiding this comment.
Do we want to add the beta version for darwin as well?
https://updates.signal.org/desktop/signal-desktop-beta-mac-universal-7.9.0-beta.1.dmg
|
@juuyokka Are you still interested in pursuing this PR? |
|
@DontEatOreo apologies for the late reply, I mostly am lost on what else needs to be done (other than rebasing off of master) to get this merged. |
|
Done in #348165 |
Description of changes
Add support for x86_64-darwin and aarch64-darwin. I unfortunately could not figure out whether or not the update script works. I also could not figure out how to run the tests. If someone could tell me how to do both of those things I'd be happy to test those as well.
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.