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

signal-desktop: init darwin at 7.5.1 #286188

Closed
wants to merge 1 commit into from
Closed

signal-desktop: init darwin at 7.5.1 #286188

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 3 additions & 5 deletions pkgs/applications/networking/instant-messengers/signal-desktop/default.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
{ hostPlatform, callPackage }: {
signal-desktop = if hostPlatform.system == "aarch64-linux"
then callPackage ./signal-desktop-aarch64.nix { }
else callPackage ./signal-desktop.nix { };
signal-desktop-beta = callPackage ./signal-desktop-beta.nix{ };
{ targetPlatform, callPackage }: {
signal-desktop = callPackage ./signal-desktop-${targetPlatform.system}.nix { };
Copy link
Member

@Mic92 Mic92 Apr 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

Perhaps we could do something like this?

Suggested change
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

Suggested change
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.

Copy link
Member

Choose a reason for hiding this comment

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

@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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

According to @wegank, targePlatform is only required when building a compiler #310053 (comment), see https://nix.dev/tutorials/cross-compilation.html#platforms.

Copy link
Member

@RossComputerGuy RossComputerGuy May 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

The cross-compiled program, in that example the compiler, still runs on the host platform (and produces executables that run on the target platform).

signal-desktop-beta = callPackage ./signal-desktop-beta.nix { };
}
25 changes: 17 additions & 8 deletions pkgs/applications/networking/instant-messengers/signal-desktop/generic.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
, fetchurl
, autoPatchelfHook
, dpkg
, undmg
, wrapGAppsHook
, makeWrapper
, nixosTests
Expand Down Expand Up @@ -72,13 +73,13 @@ in stdenv.mkDerivation rec {
inherit url hash;
};

nativeBuildInputs = [
nativeBuildInputs = lib.optionals stdenv.isLinux [
autoPatchelfHook
dpkg
(wrapGAppsHook.override { inherit makeWrapper; })
];
] ++ lib.optional stdenv.isDarwin undmg;

buildInputs = [
buildInputs = lib.optionals stdenv.isLinux [
alsa-lib
at-spi2-atk
at-spi2-core
Expand Down Expand Up @@ -115,7 +116,7 @@ in stdenv.mkDerivation rec {
xorg.libxshmfence
];

runtimeDependencies = [
runtimeDependencies = lib.optionals stdenv.isLinux [
(lib.getLib systemd)
libappindicator-gtk3
libnotify
Expand All @@ -126,12 +127,12 @@ in stdenv.mkDerivation rec {
wayland
];

unpackPhase = "dpkg-deb -x $src .";
unpackPhase = if stdenv.isLinux then "dpkg-deb -x $src ." else "undmg $src";

dontBuild = true;
dontConfigure = true;

installPhase = ''
installPhase = if stdenv.isLinux then ''
runHook preInstall

mkdir -p $out/lib
Expand All @@ -146,10 +147,18 @@ in stdenv.mkDerivation rec {
# Create required symlinks:
ln -s libGLESv2.so "$out/lib/${dir}/libGLESv2.so.2"

runHook postInstall
''
else ''
runHook preInstall

mkdir -p $out/Applications
mv Signal.app $out/Applications

runHook postInstall
'';

preFixup = ''
preFixup = lib.optionalString stdenv.isLinux ''
gappsWrapperArgs+=(
--add-flags "\''${NIXOS_OZONE_WL:+\''${WAYLAND_DISPLAY:+--ozone-platform-hint=auto --enable-features=WaylandWindowDecorations}}"
--suffix PATH : ${lib.makeBinPath [ xdg-utils ]}
Expand Down Expand Up @@ -182,7 +191,7 @@ in stdenv.mkDerivation rec {
license = lib.licenses.agpl3Only;
maintainers = with lib.maintainers; [ eclairevoyant mic92 equirosa urandom bkchr ];
mainProgram = pname;
platforms = [ "x86_64-linux" "aarch64-linux" ];
platforms = [ "x86_64-linux" "aarch64-linux" "x86_64-darwin" "aarch64-darwin" ];
sourceProvenance = with lib.sourceTypes; [ binaryNativeCode ];
};
}
8 changes: 8 additions & 0 deletions ...plications/networking/instant-messengers/signal-desktop/signal-desktop-aarch64-darwin.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{ callPackage }:
callPackage ./generic.nix { } rec {
pname = "signal-desktop";
dir = "Signal";
version = "7.5.1";
url = "https://updates.signal.org/desktop/signal-desktop-mac-arm64-${version}.dmg";
hash = "sha256-q3+v5u//niA+ortlGMsNuVSJaIM72PF97NgG0yaGHlI=";
}
0 ...signal-desktop/signal-desktop-aarch64.nix → ...-desktop/signal-desktop-aarch64-linux.nix
View file
Open in desktop
File renamed without changes.
8 changes: 8 additions & 0 deletions ...pplications/networking/instant-messengers/signal-desktop/signal-desktop-x86_64-darwin.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{ callPackage }:
callPackage ./generic.nix { } rec {
pname = "signal-desktop";
dir = "Signal";
version = "7.5.1";
url = "https://updates.signal.org/desktop/signal-desktop-mac-x64-${version}.dmg";
hash = "sha256-3GFGiMWYQSQX1EQPYPWikr+0iAo36KZUjsTGkR9MQdA=";
}
0 ...sengers/signal-desktop/signal-desktop.nix → ...l-desktop/signal-desktop-x86_64-linux.nix
View file
Open in desktop
File renamed without changes.
18 changes: 15 additions & 3 deletions pkgs/applications/networking/instant-messengers/signal-desktop/update.sh
100755 → 100644
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,25 @@ signal-desktop)
echo "Updating signal-desktop for x86_64-linux"
nix-update --version "$latestVersion" \
--system x86_64-linux \
--override-filename "$SCRIPT_DIR/signal-desktop.nix" \
signal-desktop
--override-filename "$SCRIPT_DIR/signal-desktop-x86_64-linux.nix" \
signal-desktop

echo "Updating signal-desktop for aarch64-linux"
nix-update --version "$latestVersionAarch64" \
--system aarch64-linux \
--override-filename "$SCRIPT_DIR/signal-desktop-aarch64.nix" \
--override-filename "$SCRIPT_DIR/signal-desktop-aarch64-linux.nix" \
signal-desktop

echo "Updating signal-desktop for x86_64-darwin"
nix-update --version "$latestVersion" \
--system x86_64-darwin \
--override-filename "$SCRIPT_DIR/signal-desktop-x86_64-darwin.nix" \
signal-desktop

echo "Updating signal-desktop for aarch64-darwin"
nix-update --version "$latestVersion" \
--system aarch64-darwin \
--override-filename "$SCRIPT_DIR/signal-desktop-aarch64-darwin.nix" \
signal-desktop
;;
signal-desktop-beta)
Expand Down