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-exts/ms-dotnettools-csharp: Init at 1.23.2 #100181

Merged

Conversation

@jraygauthier
Copy link
Member

@jraygauthier jraygauthier commented Oct 10, 2020

Motivation for this change

Missing vscode extension. Not possible to install simply using vscode-utils.extensionsFromVscodeMarketplace as this depends on dowloaded binaries that needs to be patched.

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.

Tested in isolation using the following expressions:

$ cd ~/nixpkgs_latest
$ nix-shell -I nixpkgs=. -p 'with import <nixpkgs> {config={allowUnfree = true;};}; vscode-with-extensions.override {vscodeExtensions = with vscode-extensions; [ms-dotnettools.csharp];}'
$ nix-build -I nixpkgs=. -E 'with import <nixpkgs> {config={allowUnfree = true;};}; vscode-with-extensions.override {vscodeExtensions = with vscode-extensions; [ms-dotnettools.csharp];}'
@jraygauthier
Copy link
Member Author

@jraygauthier jraygauthier commented Oct 21, 2020

This latest version add a patch that fixes debugging a specific test.

At this point, this extension as been quite extensively tested on projects of significant size. It works impressively well.

Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Can you please fix the merge conflict? Then I can merge it.

@jraygauthier jraygauthier force-pushed the jrg/ms-dotnettools-csharp branch from f343ec2 to 66eb1ad Nov 28, 2020
@jraygauthier
Copy link
Member Author

@jraygauthier jraygauthier commented Nov 28, 2020

@SuperSandro2000: Thanks for the review. Re-based on top of latest which fixes the conflict, applied the suggested changes and re-tested. Everything still works fine.

@SuperSandro2000
Copy link
Member

@SuperSandro2000 SuperSandro2000 commented Dec 8, 2020

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

1 package built:
  • vscode-extensions.ms-dotnettools.csharp

@jraygauthier
Copy link
Member Author

@jraygauthier jraygauthier commented Dec 9, 2020

@SuperSandro2000: Is anything wrong with nixpkgs-review pr 100181's output? Running the command locally does not fail.

@SuperSandro2000
Copy link
Member

@SuperSandro2000 SuperSandro2000 commented Dec 9, 2020

@SuperSandro2000: Is anything wrong with nixpkgs-review pr 100181's output? Running the command locally does not fail.

Nope but I am not sure if this is all packaged correctly and it is a very big diff so I decided to not merge it.

@rdk31
Copy link
Contributor

@rdk31 rdk31 commented Feb 22, 2021

Will it eventually get merged or really there'll be no way of using the c# ext in vscode on nixos?

@jraygauthier
Copy link
Member Author

@jraygauthier jraygauthier commented Feb 22, 2021

@rdk31: Would like for this to go forward. However, I wasn't told what was missing / wrong.

@SuperSandro2000
Copy link
Member

@SuperSandro2000 SuperSandro2000 commented Feb 23, 2021

@ofborg eval

@rdk31
Copy link
Contributor

@rdk31 rdk31 commented Feb 23, 2021

@SuperSandro2000: Didn't you misstype eval?

@SuperSandro2000
Copy link
Member

@SuperSandro2000 SuperSandro2000 commented Feb 24, 2021

@SuperSandro2000: Didn't you misstype eval?

Yeah, fixed.

Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

I think there are other places where inherit like this can be used but just a suggestion.
The important part is to fix the eval error.

pkgs/misc/vscode-extensions/default.nix Outdated Show resolved Hide resolved
@jbalme
Copy link
Contributor

@jbalme jbalme commented Mar 1, 2021

IMO we need some utility functions to automate some of this across a few packages.

Inside the vsix there is a package.json with a runtimeDependencies key with platform specific runtime dependencies. That is how VSCode finds and downloads runtime dependencies, in this case, OmniSharp, .NET Debugger, and Razor.

An idiomatic solution would be something that lets us do the following:

{vscode-utils, omnisharp, dotnet-debugger, razor-language-server}:
vscode-utils.buildVscodeMarketplaceExtension {
  mktplcRef = {
    name = "csharp";
    publisher = "ms-dotnettools";
    version = "1.23.2";
    sha256 = "0ydaiy8jfd1bj50bqiaz5wbl7r6qwmbz9b29bydimq0rdjgapaar";
  };
  runtimeDependencies = {
    "Omnisharp" = omnisharp;
    "Debugger" = dotnet-debugger;
    "Razor" = razor-language-server;
  }
}

...and buildVscodeMarketplaceExtension just does the magic of mapping the dependencies in package.json to the supplied runtimeDependencies,

@jraygauthier jraygauthier force-pushed the jrg/ms-dotnettools-csharp branch from 8ffc053 to 35f8b19 Mar 3, 2021
Including PR fixes as suggested by:

 -  Sandro <sandro.jaeckel@gmail.com>
@jraygauthier jraygauthier force-pushed the jrg/ms-dotnettools-csharp branch from 35f8b19 to 3752605 Mar 3, 2021
@jraygauthier
Copy link
Member Author

@jraygauthier jraygauthier commented Mar 3, 2021

@SuperSandro2000: Fixes as requested.

@jbalme: Seems like good idea worth working upon to improve the vscode extension framework for nix. IMO: it seems indeed related to this issue but out of scope. It might be worth opening a separate Issue / PR with a reference to this issue (and other) as examples of what would be improved. This would allow not to delay this issue and continue working on improving the whole set of extensions.

@SuperSandro2000
Copy link
Member

@SuperSandro2000 SuperSandro2000 commented Mar 3, 2021

This is a semi-automatic executed nixpkgs-review with nixpkgs-review-checks extension. It is checked by a human on a best effort basis and does not build all packages (e.g. lumo, tensorflow or pytorch).
If you have any questions or problems please reach out to SuperSandro2000 on IRC.

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

1 package built:
  • vscode-extensions.ms-dotnettools.csharp

The following issues got detected with the above build packages.
Please fix at least the ones listed with your changed packages:

vscode-extensions.ms-dotnettools.csharp:

warning: build-tools-in-build-inputs
unzip is a build tool so it likely goes to nativeBuildInputs, not buildInputs.

Near pkgs/misc/vscode-extensions/vscode-utils.nix:25:5:

   |
25 |     buildInputs = [ unzip ] ++ buildInputs;
   |     ^

See: https://github.com/jtojnar/nixpkgs-hammering/blob/master/explanations/build-tools-in-build-inputs.md

@jraygauthier
Copy link
Member Author

@jraygauthier jraygauthier commented Mar 3, 2021

@SuperSandro2000: The above buildInput issue with automated check is caused by pkgs/misc/vscode-extensions/vscode-utils.nix which is not in our current set of files. Should we fix this as part of this PR or leave it for a separate PR?

@SuperSandro2000
Copy link
Member

@SuperSandro2000 SuperSandro2000 commented Mar 3, 2021

Should we fix this as part of this PR or leave it for a separate PR?

Separate PR please.

@SuperSandro2000 SuperSandro2000 merged commit 535f489 into NixOS:master Mar 3, 2021
18 checks passed
@jraygauthier jraygauthier deleted the jrg/ms-dotnettools-csharp branch Mar 3, 2021
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