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.fhs: add buildFHSUserEnv wrapped version #99968

Open
wants to merge 5 commits into
base: master
from

Conversation

@jonringer
Copy link
Contributor

@jonringer jonringer commented Oct 7, 2020

Motivation for this change

Extensions commonly use pre-compiled binaries which aren't
aware of the nix store. Using buildFHSUserEnv will allow
for a more "natural" usage of vscode.

EDIT:
To not break users due to FHSUser regressions, I'm instead adding this as a passthru to the original vscode/vscodium flavors.

closes: #73810
closes: #91179

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.
@jonringer jonringer force-pushed the jonringer:vscode-buildfhsuserenv branch from 0a7bba3 to 41a65db Oct 7, 2020
@buckley310
Copy link
Contributor

@buckley310 buckley310 commented Oct 8, 2020

This opens and mostly works for me, although unfortunately the UserEnv also applies to vscode's integrated terminal, which would basically break that feature for me. Not sure if that can be fixed.

@mebubo
Copy link

@mebubo mebubo commented Oct 8, 2020

I'd like to keep using the unwrapped version. Would it be possible to make this overridable?

@jonringer
Copy link
Contributor Author

@jonringer jonringer commented Oct 9, 2020

I'd like to keep using the unwrapped version. Would it be possible to make this overridable?

it's on the passthru vscode.unwrapped

@jonringer
Copy link
Contributor Author

@jonringer jonringer commented Oct 9, 2020

I'm in no hurry to merge this, and it could probably be improved to support the other use cases. Meant for this to be more of a discussion than anything else

@buckley310
Copy link
Contributor

@buckley310 buckley310 commented Oct 9, 2020

Using the unwrapped version seems good enough for me since I already got my plugins working natively.
It does seem desirable to have plugins working out of the box, though. That did cause me some trouble early on.

I wonder if it would be a good idea for discoverability if the unwrapped versions of vscode/vscodium were added to all-packages, so that they would show up on https://search.nixos.org/packages

@jonringer
Copy link
Contributor Author

@jonringer jonringer commented Oct 9, 2020

the other option would be to expose vscode-fhs, but I'm not a big fan of that.

@buckley310 can you give some steps to repro the terminal breaking?

@buckley310
Copy link
Contributor

@buckley310 buckley310 commented Oct 9, 2020

I have gcc in my systemPackages, but when I try and compile a C hello-world from the wrapped vscode terminal gcc returns

ld: cannot find crt1.o: No such file or directory
collect2: error: ld returned 1 exit status

The bash prompt itself is just the bash default bash-4.4$, some hotkeys (ctrl+arrows) and my aliases were not working as well since a lot of the global bashrc stuff doesn't apply to UserEnvs. Some or all of that might be fixable via ~/.bashrc, but I haven't tried.

@jonringer
Copy link
Contributor Author

@jonringer jonringer commented Oct 9, 2020

I'll try to see what I can do, it would be nice for vscode to "just work"

@jonringer jonringer force-pushed the jonringer:vscode-buildfhsuserenv branch from 41a65db to c31a17d Nov 2, 2020
@jonringer
Copy link
Contributor Author

@jonringer jonringer commented Nov 2, 2020

@buckley310 do you mind trying again. I was able to get my terminal to be similar to my normal terminal

@jonringer
Copy link
Contributor Author

@jonringer jonringer commented Nov 2, 2020

@Atemu @illegalprime do you mind reviewing the bubblewrap changes?

@jonringer jonringer force-pushed the jonringer:vscode-buildfhsuserenv branch from c31a17d to 438ca07 Nov 2, 2020
@buckley310
Copy link
Contributor

@buckley310 buckley310 commented Nov 2, 2020

Bash interaction seems all good now.
Gcc issue is still present.
I actually noticed that SSL certificates seem to be problematic now, but maybe that one is just a missing bind in /etc/.

@jonringer
Copy link
Contributor Author

@jonringer jonringer commented Nov 2, 2020

what gcc issues are you having?

@buckley310
Copy link
Contributor

@buckley310 buckley310 commented Nov 2, 2020

echo 'int main(){return 0;}' >a.c ; gcc a.c

fails with

/nix/store/hiwz81i1g3fn3p0acjs042a4h5fri6dh-binutils-2.31.1/bin/ld: cannot find crt1.o: No such file or directory
/nix/store/hiwz81i1g3fn3p0acjs042a4h5fri6dh-binutils-2.31.1/bin/ld: cannot find crti.o: No such file or directory
collect2: error: ld returned 1 exit status
@jonringer
Copy link
Contributor Author

@jonringer jonringer commented Nov 2, 2020

@buckley310 I figured it out, the unwrapped gcc gets put into the environment

@jonringer
Copy link
Contributor Author

@jonringer jonringer commented Nov 2, 2020

the libstdc++.so seems to be present even if I remove any mention of stdenv.

@buckley310 please try again

@jonringer
Copy link
Contributor Author

@jonringer jonringer commented Nov 2, 2020

doing stdenv.cc in targetPkgs does make gcc-wrapped available, but I (for example) don't normally have gcc in my path

@jonringer jonringer force-pushed the jonringer:vscode-buildfhsuserenv branch from 027eff6 to 803082c Nov 2, 2020
@jonringer
Copy link
Contributor Author

@jonringer jonringer commented Nov 2, 2020

since I have buildFHSUserEnv sourcing your bash terminal. It should be inheriting from your normal shell

@jonringer
Copy link
Contributor Author

@jonringer jonringer commented Nov 2, 2020

pinging vscode extension contributors @eadwu @raboof @mcwitt @austinbutler @oxalica @veprbl

do you all mind testing this?

If you normally track unstable, this should be something like:

home-manager -I nixpkgs=https://github.com/jonringer/nixpkgs/archive/vscode-buildfhsuserenv.tar.gz switch
@jonringer jonringer force-pushed the jonringer:vscode-buildfhsuserenv branch from 803082c to 51210e7 Nov 2, 2020
@veprbl
Copy link
Member

@veprbl veprbl commented Nov 2, 2020

@jonringer Just in case, I'm not a vscode user. I was only reviewing vscode PR's because it used to be that there was no one else to look at those.

@jonringer
Copy link
Contributor Author

@jonringer jonringer commented Nov 2, 2020

oh, then thank you for your contributions. Sorry for bothering you :)

@ofborg ofborg bot requested a review from Synthetica9 Nov 2, 2020
MITSHM is an X11 extension which allows for slightly
faster access. However, the chroot isolation breaks
this, and certain items such as hover tooltips will
render black in gui applications
@jonringer jonringer force-pushed the jonringer:vscode-buildfhsuserenv branch from 51210e7 to 34dc29b Nov 3, 2020
@austinbutler
Copy link
Contributor

@austinbutler austinbutler commented Nov 3, 2020

@jonringer I use home-manager as a NixOS module so don't have home-manager directly available. I added overrides for vscode and vscode-extensions (not sure if both are necessary or if one will get in the way) in the way I normally override packages, though.

{
  nixpkgs.overlays = [
    (self: super: {
      vscode = (import (builtins.fetchTarball {
        url =
          "https://github.com/jonringer/nixpkgs/archive/34dc29b45c5f560c32b65937c265016f526be9fc.tar.gz";
        sha256 = "0p90nlia627hqn14rrs4bpbpn4bb1mjicddhc7av9lpkcy5c3l4h";
      }) { config = { allowUnfree = true; }; }).vscode;

      vscode-extensions = (import (builtins.fetchTarball {
        url =
          "https://github.com/jonringer/nixpkgs/archive/34dc29b45c5f560c32b65937c265016f526be9fc.tar.gz";
        sha256 = "0p90nlia627hqn14rrs4bpbpn4bb1mjicddhc7av9lpkcy5c3l4h";
      }) { config = { allowUnfree = true; }; }).vscode-extensions;
    })
  ];
}

When I open VS Code after switching I get a popup about Git not being found and the terminal is using a basic ZSH instead of my normal powerline goodies, aliases, etc. FYI I do have a number of VSC Extension installed via home-manager.

@jonringer
Copy link
Contributor Author

@jonringer jonringer commented Nov 3, 2020

When I open VS Code after switching I get a popup about Git not being found and the terminal is using a basic ZSH instead of my normal powerline goodies, aliases, etc. FYI I do have a number of VSC Extension installed via home-manager.

do you mind displaying your zsh related /etc/ files?

to get bash working, i had to ro-bind /etc/bashrc into the chroot'd environment.

@jonringer jonringer mentioned this pull request Nov 3, 2020
0 of 10 tasks complete
@austinbutler
Copy link
Contributor

@austinbutler austinbutler commented Nov 3, 2020

These are the zsh-related files in /etc/.

/etc/zshenv

# /etc/zshenv: DO NOT EDIT -- this file has been generated automatically.
# This file is read for all shells.

# Only execute this file once per shell.
if [ -n "$__ETC_ZSHENV_SOURCED" ]; then return; fi
__ETC_ZSHENV_SOURCED=1

if [ -z "$__NIXOS_SET_ENVIRONMENT_DONE" ]; then
    . /nix/store/w4nvlk6vh8mg6zr7wjkk5zp7ya2ms7jq-set-environment
fi

HELPDIR="/nix/store/a9f0q1cg0x2xj1pvvp52l86n8a531y3b-zsh-5.8/share/zsh/$ZSH_VERSION/help"

# Tell zsh how to find installed completions.
for p in ${(z)NIX_PROFILES}; do
    fpath+=($p/share/zsh/site-functions $p/share/zsh/$ZSH_VERSION/functions $p/share/zsh/vendor-completions)
done

# Setup custom shell init stuff.




# Read system-wide modifications.
if test -f /etc/zshenv.local; then
    . /etc/zshenv.local
fi

/etc/zshrc

# /etc/zshrc: DO NOT EDIT -- this file has been generated automatically.
# This file is read for interactive shells.
#
# Note that generated /etc/zprofile and /etc/zshrc files do a lot of
# non-standard setup to make zsh usable with no configuration by default.
#
# Which means that unless you explicitly meticulously override everything
# generated, interactions between your ~/.zshrc and these files are likely
# to be rather surprising.
#
# Note however, that you can disable loading of the generated /etc/zprofile
# and /etc/zshrc (you can't disable loading of /etc/zshenv, but it is
# designed to not set anything surprising) by setting `no_global_rcs` option
# in ~/.zshenv:
#
#   echo setopt no_global_rcs >> ~/.zshenv
#
# See "STARTUP/SHUTDOWN FILES" section of zsh(1) for more info.


# Only execute this file once per shell.
if [ -n "$__ETC_ZSHRC_SOURCED" -o -n "$NOSYSZSHRC" ]; then return; fi
__ETC_ZSHRC_SOURCED=1

# Set zsh options.
setopt HIST_IGNORE_DUPS SHARE_HISTORY HIST_FCNTL_LOCK


# Setup command line history.
# Don't export these, otherwise other shells (bash) will try to use same HISTFILE.
SAVEHIST=2000
HISTSIZE=2000
HISTFILE=$HOME/.zsh_history

# Configure sane keyboard defaults.
. /etc/zinputrc

# Enable autocompletion.
autoload -U compinit && compinit




# Setup custom interactive shell init stuff.


# oh-my-zsh configuration generated by NixOS
export ZSH=/nix/store/h1yc1l6n98dc3hsrwgsmhzy3gga7gqxq-oh-my-zsh-2020-10-18/share/oh-my-zsh

And my related config from configuration.nix:

{
  users.defaultUserShell = pkgs.zsh;
  programs.zsh = {
    enable = true;
    ohMyZsh.enable = true;
    promptInit =
      "source ${pkgs.zsh-powerlevel10k}/share/zsh-powerlevel10k/powerlevel10k.zsh-theme";
    shellAliases = {
      nixgc = "sudo nix-collect-garbage -d";
      nixup = "cd ~/Documents/nixos && bash upgrade.sh && cd -";
    };
    vteIntegration = true;
  };
}
@jonringer
Copy link
Contributor Author

@jonringer jonringer commented Nov 3, 2020

@austinbutler do you mind trying again? :)

@buckley310
Copy link
Contributor

@buckley310 buckley310 commented Nov 3, 2020

curl https://x.org does not work on 34dc29b. certificate errors. That's the only problem left that I can find.
It is a little quirky that vscode is in it's own PID namespace now, but I don't think that would really bother me. Maybe others if they spend a lot more time in the integrated terminal?

@jonringer
Copy link
Contributor Author

@jonringer jonringer commented Nov 3, 2020

@buckley310 that probably because we do --unshare-all to the chroot, I will probably make this more "configurable" to allow a "barely" isolated vscode. But I have to go cook dinner

@austinbutler
Copy link
Contributor

@austinbutler austinbutler commented Nov 3, 2020

@austinbutler do you mind trying again? :)

I still get the "Git not found" error when I open VSC. The terminal now looks a bit more like my normal terminal, but still it is not the same (no git status). And the terminal has a new error: /etc/zshrc:.:36: no such file or directory: /etc/zinputrc.

@austinbutler
Copy link
Contributor

@austinbutler austinbutler commented Nov 3, 2020

Maybe you need to also add /etc/zinputrc and /etc/zprofile.
most likely

@jonringer
Copy link
Contributor Author

@jonringer jonringer commented Nov 3, 2020

never realised how distributed zsh files were

@jonringer jonringer force-pushed the jonringer:vscode-buildfhsuserenv branch from 289048f to 584745f Nov 3, 2020
@austinbutler
Copy link
Contributor

@austinbutler austinbutler commented Nov 3, 2020

Latest commit has resolved the zinputrc error, but still Git is not found. I do have my ZSH aliases though.

@jonringer
Copy link
Contributor Author

@jonringer jonringer commented Nov 3, 2020

Latest commit has resolved the zinputrc error, but still Git is not found. I do have my ZSH aliases though.

how do you have git installed?

I have it configured through home-manager, and it's able to be found

@jonringer
Copy link
Contributor Author

@jonringer jonringer commented Nov 3, 2020

I might break this out into a separate top-level attr. Something like vscode-fhs or added it to the passthru as vscode.fhs

Actually, just having a "fhs" on the original vscode/vscodium might be best.

thoughts? @Atemu

@jonringer
Copy link
Contributor Author

@jonringer jonringer commented Nov 3, 2020

@timokau how do you feel about instead of making vscode wrapped, just provide a vscode.fhs "variant"

cc @FRidh , probably know of a better way

@timokau
Copy link
Member

@timokau timokau commented Nov 3, 2020

Did you ping me on purpose? I have never contributed to or used vscode.

@jonringer
Copy link
Contributor Author

@jonringer jonringer commented Nov 3, 2020

I did, I was asking more about how this should be exposed in nixpkgs.

Currently the PR is replacing the existing vscode/vscodium, but there's still some regressions due to how our current buildFHSUserEnvBubblewrap is called. And I was wondering if we should just provide a "new" attr, so that people using the previous workflow aren't "suddenly" broken.

@FRidh
Copy link
Member

@FRidh FRidh commented Nov 3, 2020

I prefer to avoid using buildFHSUserEnv, because its more of a last-resort method, but I understand why some would like it. For that reason I would prefer to be able to choose.

@jonringer
Copy link
Contributor Author

@jonringer jonringer commented Nov 3, 2020

I prefer to avoid using buildFHSUserEnv, because its more of a last-resort method, but I understand why some would like it. For that reason I would prefer to be able to choose.

Being able to use vscode extensions with no additional effort is a pretty big win for some (myself included, where some projects just say to inherit the vscode settings to avoid linting and styling CI pain)

I'll refactor the package so there's a fhs attr on the passthru. And people can just do programs.vscode.package = pkgs.vscode.fhs; in home-manager to bring it in

@jonringer jonringer changed the title vscode: use buildFHSUserEnv vscode.fhs: add buildFHSUserEnv wrapped version Nov 3, 2020
@timokau
Copy link
Member

@timokau timokau commented Nov 3, 2020

I did, I was asking more about how this should be exposed in nixpkgs.

Ah, I see. My uneducated opinion is that it would be nice to have the "just works" (FHS) option as the default, with the option to chose the plain version. If its possible to detect broken behavior in the FHS version, you could print a warning and suggest that uses switch to the plain version. If that is not possible its maybe better to keep the plain version as the default now.

But I'm really not very qualified to comment on this :)

@jonringer
Copy link
Contributor Author

@jonringer jonringer commented Nov 3, 2020

But I'm really not very qualified to comment on this :)

Well, it's still nice to get feedback, in case you're going down the wrong path. Thanks for your input.

The FHS "regressions" are more around creating kernel namespaces, which makes it feel like you're developing on "someone else's machine" rather than your own.

I think I will use the vscode.fhs path for now, and then when we are "stable enough" maybe switch it.

@Atemu
Atemu approved these changes Nov 3, 2020
Copy link
Member

@Atemu Atemu left a comment

Bubblewrap changes LGTM. All FHS packages I use still work just like before.

Can't comment on vscode, I use Emacs :p

@@ -1,7 +1,7 @@
{ stdenv, lib, makeDesktopItem
, unzip, libsecret, libXScrnSaver, wrapGAppsHook
, gtk2, atomEnv, at-spi2-atk, autoPatchelfHook
, systemd, fontconfig, libdbusmenu
, systemd, fontconfig, libdbusmenu, buildFHSUserEnvBubblewrap

This comment has been minimized.

@Atemu

Atemu Nov 3, 2020
Member

Nit: buildFHSUserEnvBubblewrap is only meant as a temporary measure until it replaces buildFHSUserEnv completely, so vscode should probably not explicitly depend on it and rather override the buildFHSUserEnv with it in its callPackage.

This comment has been minimized.

@jonringer

jonringer Nov 4, 2020
Author Contributor

I was thinking about this, but I'm starting to like having dependency mapping in two places less.

@Atemu
Copy link
Member

@Atemu Atemu commented Nov 3, 2020

The . in vscode.fhs kind of implies that fhs is an attribute of vscode and the only attributes I'd expect under a derivation are mkDerivation arguments and other implementation details. I think the two variants should be on the same level.

My proposal would be either: Add a vscode-fhs package or switch vscode over to FHS by default and provide -unwrapped.

In both cases there should be an explanation why there are two packages and what makes them different from another in one of the desciprions so that it shows up in a package search. Many users will have no idea what -unwrapped or -fhs stand for and may be confused which they should choose (first hand experience).

@jonringer
Copy link
Contributor Author

@jonringer jonringer commented Nov 4, 2020

The . in vscode.fhs kind of implies that fhs is an attribute of vscode and the only attributes I'd expect under a derivation are mkDerivation arguments and other implementation details. I think the two variants should be on the same level.

part of this too, is that vscodium would also received a wrapped version. I would have to do another vscodium-fhs to be consistent. And eventually, I would like the default vscode to "just work as expected". So I don't really want to have people rely on vscode-fhs always being present.

I could rename fhs to fhs-wrapped if that's more explicit as to what's going on

Most C compilers have a .cc attr which points to the unwrapped version, it's not the most common paradigm, but it does exist.

@austinbutler
Copy link
Contributor

@austinbutler austinbutler commented Nov 6, 2020

Latest commit has resolved the zinputrc error, but still Git is not found. I do have my ZSH aliases though.

how do you have git installed?

I have it configured through home-manager, and it's able to be found

In home manager config in configuration.nix:

programs.git.enable = true;
@nixos-discourse
Copy link

@nixos-discourse nixos-discourse commented Nov 18, 2020

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/new-users-using-nixos-wrong/9973/38

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.

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