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

upgrade msbuild and fix omnisharp-roslyn #132165

Merged
merged 4 commits into from
Aug 8, 2021
Merged

Conversation

corngood
Copy link
Contributor

Should fix #126477

Motivation for this change
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/)
  • 21.11 Release Notes (or backporting 21.05 Relase notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@r-rmcgibbo
Copy link

r-rmcgibbo commented Jul 30, 2021

Result of nixpkgs-review pr 132165 at 415bef1 run on aarch64-linux 1

2 packages failed to build:
1 suggestion:
  • warning: missing-phase-hooks

    installPhase should probably contain runHook preInstall and runHook postInstall.

    Near pkgs/development/tools/omnisharp-roslyn/default.nix:94:3:

       |
    94 |   installPhase = ''
       |   ^
    

Note that build failures may predate this PR, and could be nondeterministic or hardware dependent.
Please exercise your independent judgement. Does something look off? Please file an issue or reach out on IRC.


Result of nixpkgs-review pr 132165 at 415bef1 run on x86_64-linux 1

2 packages built successfully:
  • msbuild
  • omnisharp-roslyn
5 suggestions:
  • warning: missing-phase-hooks

    buildPhase should probably contain runHook preBuild and runHook postBuild.

    Near pkgs/development/tools/build-managers/msbuild/default.nix:63:3:

       |
    63 |   buildPhase = ''
       |   ^
    
  • warning: missing-phase-hooks

    installPhase should probably contain runHook preInstall and runHook postInstall.

    Near pkgs/development/tools/omnisharp-roslyn/default.nix:94:3:

       |
    94 |   installPhase = ''
       |   ^
    
  • warning: build-tools-in-build-inputs

    makeWrapper is a build tool so it likely goes to nativeBuildInputs, not buildInputs.

    Near pkgs/development/tools/build-managers/msbuild/default.nix:39:3:

       |
    39 |   buildInputs = [
       |   ^
    
  • warning: unused-argument

    Unused argument: fetchpatch.
    Near pkgs/development/tools/build-managers/msbuild/default.nix:1:26:

      |
    1 | { lib, stdenv, fetchurl, fetchpatch, makeWrapper, glibcLocales, mono, dotnetPackages, unzip, dotnet-sdk, writeText, roslyn }:
      |                          ^
    
  • warning: missing-phase-hooks

    installPhase should probably contain runHook preInstall and runHook postInstall.

    Near pkgs/development/tools/build-managers/msbuild/default.nix:94:3:

       |
    94 |   installPhase = ''
       |   ^
    

@felschr
Copy link
Member

felschr commented Aug 1, 2021

I've updated my test project to use the changes of this PR: https://github.com/felschr/omnisharp-nix-issue
I was also able to remove quite a bit of omnisharp & dotnet configuration that was apparently unnecessary.
omnisharp is finally working for me, at least on this project. I'm gonna try using it on a more complex project next week and report back.

@mohe2015 mohe2015 self-requested a review August 1, 2021 12:59
@corngood corngood changed the title wip: upgrade msbuild and fix omnisharp-roslyn upgrade msbuild and fix omnisharp-roslyn Aug 2, 2021
@hpfr
Copy link
Contributor

hpfr commented Aug 2, 2021

Thanks, this also fixed an issue I had using omnisharp for emacs-lsp where omnisharp would find the embedded msbuild 16.8 but then claim it needed at least msbuild 16.8, which was strange. Much of the log messages seem to be duplicated, but that might not be due to omnisharp. Looking forward to the merge!

@mohe2015 mohe2015 removed their request for review August 3, 2021 13:36
@hpfr
Copy link
Contributor

hpfr commented Aug 6, 2021

I guess @jirkadanek for msbuild, although they don't look active. Maybe msbuild needs another maintainer as well?

@corngood
Copy link
Contributor Author

corngood commented Aug 6, 2021

I thought about adding myself as maintainer. I'm still working on some cleanup of the nuget dependencies. I'd love it if all of that was dealt with in a consistent manner across nixpkgs, but msbuild has so many quirks around bootstrapping that it's quite a pain in the ass. I'd like to at least make create-deps.sh easier to use.

@jiridanek
Copy link
Contributor

I'm now known as @jiridanek (was @jirkadanek), but yes, I haven't touched nixpkgs in a long while now. And msbuild is quite difficult build to get back up to speed for me on. For personal use, I've given up on nixpkgs-packaged dotnet (and IntelliJ IDEs) and I am instead running the binary downloads...

I had big plans around msbuild nixpkgs, but I somehow moved to other things; It would be still interesting to get back, but... so would be 100+1 other possible things...

Copy link
Member

@Artturin Artturin left a comment

Choose a reason for hiding this comment

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

not tested (other than running the binary) but they built and its better than the status quo

@Artturin Artturin merged commit fcc7cd5 into NixOS:master Aug 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

omnisharp-roslyn cannot find Microsoft.Common.props
7 participants