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.

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: Fix "Save as Admin" #49643 #102010

Closed
wants to merge 1 commit into from
Closed

vscode: Fix "Save as Admin" #49643 #102010

wants to merge 1 commit into from

Conversation

@kode54
Copy link

@kode54 kode54 commented Oct 29, 2020

Motivation for this change

I decided to fix something that I found was broken, and saw in the relevant issue that a fix was already applied to another package in a similar fashion, but had not made its way to this package yet.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
@@ -69,6 +69,16 @@ in
dontBuild = true;
dontConfigure = true;

patchPhase =
if system != "x86_64-darwin" then ''

This comment has been minimized.

@SuperSandro2000

SuperSandro2000 Oct 29, 2020
Member

Can you elaborate why this is not required on darwin?

This comment has been minimized.

@kode54

kode54 Oct 30, 2020
Author

Does the darwin flavor of sudo-prompt even use the same scripts to achieve administrator access? How does nix even handle sandboxing shell commands on an OS like that?

This comment has been minimized.

@kode54

kode54 Oct 30, 2020
Author

As I suspected:

https://github.com/jorangreef/sudo-prompt/blob/master/index.js

sudo-prompt uses a completely different code path on macOS/Darwin to achieve the sudo prompt, going as far as bundling a special program in a zip file, unpacking it, and running it, to elevate the requested command.

The above patch for /bin/bash and /usr/bin/pkexec processes only applies to NixOS, and any other Linux system adhering to Nix file paths for these two utilities.

I wasn't sure if this was the correct way to limit it, though. I took the hint from the install process, which gates Darwin specific install process from everything else, since only Darwin will be installing a single .app bundle.

I'm just going to guess Nix doesn't target native Windows? If it does, then this check should specifically apply only to Linux versions.

This comment has been minimized.

@SuperSandro2000

SuperSandro2000 Jan 15, 2021
Member

I would prefer if we apply this unconditionally to prevent bit rot as long as it does not break something not already broken on darwin.

This comment has been minimized.

@kode54

kode54 Jan 16, 2021
Author

Sure, you can apply it unconditionally, but upstream only supplies binary packages for x86_64 anyway.

@SuperSandro2000
Copy link
Member

@SuperSandro2000 SuperSandro2000 commented Oct 29, 2020

Result of nixpkgs-review pr 102010 run on x86_64-linux 1

3 packages built:
  • vscode
  • vscode-with-extensions
  • vscodium
@berbiche
Copy link
Member

@berbiche berbiche commented Jan 13, 2021

Friendly bump 😃

@Synthetica9
Copy link
Contributor

@Synthetica9 Synthetica9 commented Jan 13, 2021

@@ -69,6 +69,16 @@ in
dontBuild = true;
dontConfigure = true;

patchPhase =

This comment has been minimized.

@SuperSandro2000

SuperSandro2000 Feb 12, 2021
Member

Suggested change
patchPhase =
postPatch =
@@ -69,6 +69,16 @@ in
dontBuild = true;
dontConfigure = true;

patchPhase =
if system != "x86_64-darwin" then ''

This comment has been minimized.

@SuperSandro2000

SuperSandro2000 Feb 12, 2021
Member

Suggested change
if system != "x86_64-darwin" then ''
lib.optionalString (system != "x86_64-darwin") ''
sed -i 's|/bin/bash|${bash}/bin/bash|' tmp/sudo-prompt/index.js
${nodePackages.asar}/bin/asar pack tmp ./resources/app/node_modules.asar
rm -rf tmp
'' else '''';

This comment has been minimized.

@SuperSandro2000

SuperSandro2000 Feb 12, 2021
Member

Suggested change
'' else '''';
'';
@kode54
Copy link
Author

@kode54 kode54 commented Feb 12, 2021

I'm just gonna close this, as I don't intend to use Nix again any time in the future anyway.

@kode54 kode54 closed this Feb 12, 2021
@berbiche
Copy link
Member

@berbiche berbiche commented Feb 13, 2021

@kode54 sorry for the cc, the situation is unfortunate, thanks for the groundwork in any event 👍 .
Surely someone else will pick it up at some point in time!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants