-
-
Notifications
You must be signed in to change notification settings - Fork 15.3k
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
vscode: Make it so that it places the .app in $out/Applications on macOS #70486
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't verify it builds since I don't have a Macintosh but looking at the changes and file structure of the download this looks fine.
Worth noting, this will fix #47553 in theory. |
ln -s $out/lib/vscode/Contents/Resources/app/bin/${executableName} $out/bin | ||
mkdir -p "$out/Applications/${longName}.app" $out/bin | ||
cp -r ./* "$out/Applications/${longName}.app" | ||
ln -s "$out/Applications/${longName}.app/Contents/Resources/app/bin/code" $out/bin/${executableName} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ln -s "$out/Applications/${longName}.app/Contents/Resources/app/bin/code" $out/bin/${executableName} | |
ln -s "$out/Applications/${longName}.app/Contents/Resources/app/bin/${executableName}" $out/bin/${executableName} |
Why was that ^ changed? Is that not important for the macos build artifact?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was done because for whatever reason the vscodium macOS release .zip doesn't have the code
utility renamed to codium
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, can you add a comment? This installPhase is already completely uncommented, and if I were to notice this not using executableName
I would for sure change that and break things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be surprised to see a PR that changes working code from a static path reference to using a variable. I agree that a comment is probably a good idea, but my first impression upon reading this code is simply that the app includes a binary named app/bin/code
, and I wouldn't question that.
Just requesting review of people I'm aware of that use darwin for testing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @AmandaCameron, tested on Darwin
Motivation for this change
vscode and vscodium when installed under nix-darwin don't appear in the
~/Applications/Nix Apps
folder, due to how the $out is placed.Things done
Make it so that on macOS instead of extracting the contents of the app into
$out/lib/vscode
it goes into$out/Applications/${longName}.app
which will then be linked to the correct place by nix-darwinsandbox
innix.conf
on non-NixOS)nix-shell -p nix-review --run "nix-review wip"
./result/bin/
)nix path-info -S
before and after)Notify maintainers
cc @eadwu @Synthetica