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-with-extensions: fix insiders build #71251

Merged
merged 1 commit into from Mar 7, 2020

Conversation

@hyperfekt
Copy link
Contributor

hyperfekt commented Oct 16, 2019

Extensions are no longer unwrapped to the /share directory so the
extensions' derivations do not have to know about VSCode's package name.

Motivation for this change

Without this patch overriding the vscode used by vscode-with-extensions with isInsiders = true has VSCode fail to start because it tries to create a code-insiders directory inside the read-only combined extensions derivation output because it is instructed to use that nonexistent one as its extension directory.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nix-review --run "nix-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.
Notify maintainers

cc @eadwu @jraygauthier

@eadwu
eadwu approved these changes Oct 16, 2019
Copy link
Member

eadwu left a comment

I don't seem to have this problem but change looks fine nonetheless.

@hyperfekt
Copy link
Contributor Author

hyperfekt commented Oct 16, 2019

Strange. Have you tried with the current Insiders build, and made sure you're actually passing extensions? It's not immediately obvious to me why others wouldn't hit this except for a lack of trying it.
The issue probably lies with not overriding the vscode passed to the extensions' derivations as well, the necessity of which is not very obvious. I don't tend to use overlays, which is why that was an issue for me.

Extensions are no longer unwrapped to the /share directory so the
extensions' derivations do not have to know about VSCode's package name.
@hyperfekt hyperfekt force-pushed the hyperfekt:vscode-insiders-extensions branch from 66ebff3 to bb24faf Oct 16, 2019
@hyperfekt
Copy link
Contributor Author

hyperfekt commented Oct 16, 2019

This made me realize that since the unpacked extensions and the combined extensions derivation are both completely independent of the VSCode version they'll eventually be used for, I could remove their dependency on it.

@jraygauthier
Copy link
Member

jraygauthier commented Oct 17, 2019

Seems fine to me. A nice cleanup / simplification too.

@eadwu eadwu mentioned this pull request Oct 17, 2019
4 of 10 tasks complete
@veprbl
Copy link
Member

veprbl commented Feb 9, 2020

If this is ready and #71264 has stalled, let's merge this?
ping @worldofpeace

@eadwu
eadwu approved these changes Feb 10, 2020
Copy link
Member

eadwu left a comment

Builds/Executes fine [still] on my system.

@worldofpeace
Copy link
Member

worldofpeace commented Feb 10, 2020

No rejections here, just don't have time to review it.

@worldofpeace
Copy link
Member

worldofpeace commented Feb 10, 2020

I'm in favor of this PR I guess.


inherit vscodeExtUniqueId;
inherit configurePhase buildPhase dontPatchELF dontStrip;

installPrefix = "share/${extendedPkgName}/extensions/${vscodeExtUniqueId}";
installPrefix = "${vscodeExtUniqueId}";

This comment has been minimized.

Copy link
@jraygauthier

jraygauthier Feb 12, 2020

Member

What bothers me is that now, an extension no longer stand as a standard package which could simply be installed in an environment using nix-env. It this really a use case, I'm not sure.

In case we want to treat those extensions as standalone packages, stuff like this really should end up under the lfs share folder and the namespace under share being fairly unique. It was the approach taken by vim plugins which I initially took inspiration from.

It might even end up being required by licenses to expose the source code / files when installing vscode-with-extensions/vscodium-with-extensions/etc (is it?).

How about instead just outputting those under something more straigtforward such as: share/vscode/extensions/${vscodeExtUniqueId} regardless of the wrapped executable? This is what has been suggested in #76494.

Note that this is not a strong opinion. Anyone else interested in providing more insight?

This comment has been minimized.

Copy link
@hyperfekt

hyperfekt Feb 12, 2020

Author Contributor

That's a very good point! However that isn't a perfect solution, since Insiders versions of VSCode need their extensions to be in an appropriately named directory.
I wonder how many people actually use Nix-managed extensions but do not use them via vscode-with-extensions, but I agree that it's good to support it on principle.
My suggestion is that we have the extension derivations like they are now, and provide versions of them that have been symlinked to the appropriate place that people can use. This means the path could be varied depending on whether they use an Insiders version or not.

This comment has been minimized.

Copy link
@jraygauthier

jraygauthier Feb 12, 2020

Member

Insiders versions of VSCode need their extensions to be in an appropriately named directory.

I'm not sure to quite understand in what regards the current PR changes that and what exactly is the distinction in terms of extension location?

@veprbl veprbl merged commit a2437c3 into NixOS:master Mar 7, 2020
12 checks passed
12 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
billksun added a commit to billksun/home-manager that referenced this pull request Mar 18, 2020
Fix extension path symlink error caused by NixOS/nixpkgs#71251, which removes /share/{wrappedPkgName}/extensions from the extension install path
rycee added a commit to rycee/home-manager that referenced this pull request Mar 21, 2020
Fix extension path symlink error caused by [1], which removes
`/share/{wrappedPkgName}/extensions` from the extension install path.

[1] NixOS/nixpkgs#71251

PR #1100
@raboof raboof mentioned this pull request Apr 11, 2020
1 of 10 tasks complete
jorsn added a commit to jorsn/home-manager that referenced this pull request Apr 25, 2020
Fix extension path symlink error caused by [1], which removes
`/share/{wrappedPkgName}/extensions` from the extension install path.

[1] NixOS/nixpkgs#71251

PR rycee#1100
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

5 participants
You can’t perform that action at this time.