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

Add visual studio code editor #14418

Merged
merged 2 commits into from
Apr 3, 2016
Merged

Add visual studio code editor #14418

merged 2 commits into from
Apr 3, 2016

Conversation

mucaho
Copy link
Contributor

@mucaho mucaho commented Apr 3, 2016

This is the 3rd implementation variant as proposed in #14354. See that issue for details:

  • Still need to adapt the configuration file, which enables a vital part of the application - the extension manager. Seems like it can stay FOSS even with the configuration file pointing at official marketplace.
  • This PR downgrades current version of electron to be compatible with vscode. electron will have to follow vscodes version requirement in future. TODO: make seperate electron nix expression, just for vscode.
  • This PR shifts the injection of shared library lookup paths from the wrapProgram utility to patchELF itself. The flag dontPatchELF is needed, because in the fixupPhase the added $out/bin rpath is automatically removed. Additionally, both dontPatchELF & dontStrip need to be set to false in order for the binary to run without segfault. TODO: Move patchELF to postFixup.
Things done:
  • Tested using buildtime sandboxing (nix-build . --option build-use-chroot true -A vscode)
  • Tested using runtime sandboxing (nix-shell -I nixpkgs=/path/to/vscode/branch --pure -p vscode)
  • Built on platform(s)
    • NixOS
    • OS X
    • Linux
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip --against HEAD~2" from root of nixpkgs fork
    • dependent pkg electron compiles & runs
    • dependent pkg vscode compiles & runs
    • Tested using electron-quick-start
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.
More

Fixes issue #14354

cc @travisbhartwell


@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @fluffynukeit, @gebner and @travisbhartwell to be potential reviewers

@vcunat
Copy link
Member

vcunat commented Apr 3, 2016

The flag dontPatchELF is needed, because in the fixupPhase the added $out/bin rpath is automatically removed. Additionally, both dontPatchELF & dontStrip need to be set to false in order for the binary to run without segfault.

It's generally better to run patchelf in postFixup. That way you shouldn't need to disable the automating stripping and patchelfing. (Stripping a patchelfed file breaks it sometimes.)

@vcunat
Copy link
Member

vcunat commented Apr 3, 2016

And 👍 for using RPATH instead of $LD_LIBRARY_PATH, as the latter e.g. "relinks" any child processes, etc.

Use patchelf instead of wrapProgram to inject lookup path of shared
libraries.
Allow the nix expression to be called with optional version argument.
@mucaho mucaho force-pushed the vscode branch 2 times, most recently from 8dcae83 to f147f96 Compare April 3, 2016 16:32
'';

meta = with stdenv.lib; {
description = "Code Editing. Redefined.";
Copy link
Member

Choose a reason for hiding this comment

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

Please write a more appreciate description.

@mucaho
Copy link
Contributor Author

mucaho commented Apr 3, 2016

Ok, thank you all for your input, I have applied the suggested changes.

@joachifm joachifm merged commit a703bd4 into NixOS:master Apr 3, 2016
@mucaho mucaho deleted the vscode branch April 3, 2016 20:16
@mucaho mucaho mentioned this pull request Apr 4, 2016
9 tasks
@LisannaAtHome
Copy link

Why was vscode put into the "unfree" section of nixpkgs? What are the non-free components in it?

@gebner
Copy link
Member

gebner commented Nov 9, 2017

@ledettwy The binary distribution of vscode is non-free. The source distribution as hosted on github is licensed under MIT, but it cannot access the marketplace (i.e., the extensions) out of the box. It is not clear to me what would happen if we were to patch the open-source version to access the marketplace (you just need to modify a single JSON file).

The original vscode derivation (this PR) was built from source, but it is fundamentally broken since it needs to access the network during the build (#14447). The current tracking issue for the from-source build is #26698.

@LisannaAtHome
Copy link

It is not clear to me what would happen if we were to patch the open-source version to access the marketplace (you just need to modify a single JSON file).

Not sure what you mean. Free software that connects to non-free servers doesn't stop becoming free software. Browsers can have the capability to download extensions from the Internet and the browser is still free.

If you're concerned about the JSON file itself not being free software, doing a clean room implementation of that seems like it would be pretty straightforward.

@orivej
Copy link
Contributor

orivej commented Nov 9, 2017

The license of the binary build that we use says that "You may not … share, publish, or lend the software". This makes it unfree nonredistributable.

@LisannaAtHome
Copy link

Oh, so it's the binary version that's in unfree nixpkgs, and the source version (when it works) will go into the free nixpkgs?

@orivej
Copy link
Contributor

orivej commented Nov 9, 2017

Yes.

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

Successfully merging this pull request may close these issues.

9 participants