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.

Sign up for GitHub

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

Jump to bottom

roslyn-ls: 4.13.0-3.24577.4 -> 4.13.0-3.25051.1 #372476

Closed
wants to merge 1 commit into from
Closed

roslyn-ls: 4.13.0-3.24577.4 -> 4.13.0-3.25051.1 #372476

wants to merge 1 commit into from

Conversation

tris203
Copy link

@tris203 tris203 commented Jan 9, 2025

Minor update and removes the net6.0 build dep

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 Release 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.

Add a 👍 reaction to pull requests you find important.

@tris203
Copy link
Author

tris203 commented Jan 9, 2025

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 372476

x86_64-linux

✅ 1 package built:
  • roslyn-ls

@tris203 tris203 marked this pull request as ready for review January 9, 2025 20:45
@nix-owners nix-owners bot requested a review from konradmalik January 9, 2025 20:45
@konradmalik
Copy link
Contributor

Hmm, the update is good, but do we really want to remove net6?
The dependency is still there, just hidden in deps.json.
WDYT @corngood @GGG-KILLER ?

@tris203
Copy link
Author

tris203 commented Jan 10, 2025 •
edited
Loading

Hmm, the update is good, but do we really want to remove net6? The dependency is still there, just hidden in deps.json. WDYT @corngood @GGG-KILLER ?

The main reason was that we weren't getting the update script passthru anymore due to it
https://nixpkgs-update-logs.nix-community.org/roslyn-ls/

By (re)moving it, i think we will resolve that

@GGG-KILLER
Copy link
Contributor

The dependency is still there, just hidden in deps.json.

That does bother me, but since we have no mechanisms to currently detect whether a package is EOL or has a known vulnerability, it should be fine (begrudgingly).

This is more an issue of the dotnet infra that should be solved at a higher level.

@corngood
Copy link
Contributor

That does bother me, but since we have no mechanisms to currently detect whether a package is EOL or has a known vulnerability, it should be fine (begrudgingly).

I don't like this either. If it was just App.Ref maybe, but this includes the host/runtimes. Let me see if we can hack away the dependency...

@konradmalik
Copy link
Contributor

FYI in one of the previous updates I was able to override NetRoslynBuildHostNetCoreVersion by setting it to net8.
The dependency on net6 was removed but unfortunately, the build failed after that so I needed to revert this commit.

@corngood
Copy link
Contributor

FYI in one of the previous updates I was able to override NetRoslynBuildHostNetCoreVersion by setting it to net8. The dependency on net6 was removed but unfortunately, the build failed after that so I needed to revert this commit.

This is what I was trying too. The first problem I ran into was:

src/Workspaces/Core/MSBuild.BuildHost/Microsoft.CodeAnalysis.Workspaces.MSBuild.BuildHost.csproj

  <ItemGroup Condition="'$(DotNetBuildSourceOnly)' != 'true'">
    <!-- Reference MSBuild directly to avoid redistributing its package closure and including the dependencies in deps.json file. -->
    <PackageDownloadAndReference Include="Microsoft.Build" Version="$(_MsbuildVersion)" Folder="ref/$(TargetFramework)" />
    <PackageDownloadAndReference Include="Microsoft.Build.Framework" Version="$(_MsbuildVersion)" Folder="ref/$(TargetFramework)" />
    <PackageDownloadAndReference Include="Microsoft.Build.Tasks.Core" Version="$(_MsbuildVersion)" Folder="ref/$(TargetFramework)" />
    <PackageDownloadAndReference Include="Microsoft.Build.Utilities.Core" Version="$(_MsbuildVersion)" Folder="ref/$(TargetFramework)" />
  </ItemGroup>

These have to be ref/net6.0, because that package doesn't have net8.0 assemblies.

That's not the last of the problems unfortunately.

@corngood
Copy link
Contributor

corngood commented Jan 10, 2025 •
edited
Loading

This seems to work, but I haven't tested the language server beyond just building the passthru tests:

diff --git a/eng/targets/TargetFrameworks.props b/eng/targets/TargetFrameworks.props
index 2dddaff1560..bc8fd1938d5 100644
--- a/eng/targets/TargetFrameworks.props
+++ b/eng/targets/TargetFrameworks.props
@@ -17,7 +17,7 @@
     <NetVS>net8.0</NetVS>
     <NetVSCode>net8.0</NetVSCode>
     <NetVSShared>net8.0</NetVSShared>
-    <NetRoslynBuildHostNetCoreVersion>net6.0</NetRoslynBuildHostNetCoreVersion>
+    <NetRoslynBuildHostNetCoreVersion>net8.0</NetRoslynBuildHostNetCoreVersion>
     <NetRoslynNext>net9.0</NetRoslynNext>
   </PropertyGroup>
 
diff --git a/src/Workspaces/Core/MSBuild.BuildHost/Microsoft.CodeAnalysis.Workspaces.MSBuild.BuildHost.csproj b/src/Workspaces/Core/MSBuild.BuildHost/Microsoft.CodeAnalysis.Workspaces.MSBuild.BuildHost.csproj
index 8101f56b8be..1733955dc3d 100644
--- a/src/Workspaces/Core/MSBuild.BuildHost/Microsoft.CodeAnalysis.Workspaces.MSBuild.BuildHost.csproj
+++ b/src/Workspaces/Core/MSBuild.BuildHost/Microsoft.CodeAnalysis.Workspaces.MSBuild.BuildHost.csproj
@@ -28,6 +28,12 @@
     -->
     <_MsbuildVersion>17.3.4</_MsbuildVersion>
   </PropertyGroup>
+  <PropertyGroup>
+    <_MsbuildFramework>$(TargetFramework)</_MsbuildFramework>
+  </PropertyGroup>
+  <PropertyGroup Condition="'$(TargetFramework)' == 'net8.0'">
+    <_MsbuildFramework>net6.0</_MsbuildFramework>
+  </PropertyGroup>
   <ItemGroup Condition="'$(DotNetBuildSourceOnly)' == 'true'">
     <PackageReference Include="Microsoft.Build" VersionOverride="$(_MsbuildVersion)" ExcludeAssets="Runtime" PrivateAssets="All" />
     <PackageReference Include="Microsoft.Build.Framework" VersionOverride="$(_MsbuildVersion)" ExcludeAssets="Runtime" />
@@ -36,10 +42,10 @@
   </ItemGroup>
   <ItemGroup Condition="'$(DotNetBuildSourceOnly)' != 'true'">
     <!-- Reference MSBuild directly to avoid redistributing its package closure and including the dependencies in deps.json file. -->
-    <PackageDownloadAndReference Include="Microsoft.Build" Version="$(_MsbuildVersion)" Folder="ref/$(TargetFramework)" />
-    <PackageDownloadAndReference Include="Microsoft.Build.Framework" Version="$(_MsbuildVersion)" Folder="ref/$(TargetFramework)" />
-    <PackageDownloadAndReference Include="Microsoft.Build.Tasks.Core" Version="$(_MsbuildVersion)" Folder="ref/$(TargetFramework)" />
-    <PackageDownloadAndReference Include="Microsoft.Build.Utilities.Core" Version="$(_MsbuildVersion)" Folder="ref/$(TargetFramework)" />
+    <PackageDownloadAndReference Include="Microsoft.Build" Version="$(_MsbuildVersion)" Folder="ref/$(_MsbuildFramework)" />
+    <PackageDownloadAndReference Include="Microsoft.Build.Framework" Version="$(_MsbuildVersion)" Folder="ref/$(_MsbuildFramework)" />
+    <PackageDownloadAndReference Include="Microsoft.Build.Tasks.Core" Version="$(_MsbuildVersion)" Folder="ref/$(_MsbuildFramework)" />
+    <PackageDownloadAndReference Include="Microsoft.Build.Utilities.Core" Version="$(_MsbuildVersion)" Folder="ref/$(_MsbuildFramework)" />
   </ItemGroup>
   <ItemGroup>
     <PackageReference Include="Microsoft.Build.Locator" PrivateAssets="All" />

Edit: we could probably also upgrade _MsbuildVersion.

konradmalik added a commit to konradmalik/nixpkgs that referenced this pull request Jan 12, 2025
@konradmalik
roslyn-ls: 4.13.0-3.24577.4 -> 4.13.0-3.25051.1
9753abd 
Added a git-patch to drop net6.0 dependency, which is now EOL.
See NixOS#372476 (comment)
@konradmalik konradmalik mentioned this pull request Jan 12, 2025
Merged
13 tasks
konradmalik added a commit to konradmalik/nixpkgs that referenced this pull request Jan 12, 2025
@konradmalik
roslyn-ls: 4.13.0-3.24577.4 -> 4.13.0-3.25051.1
b5473e6 
Added a git-patch to drop net6.0 dependency, which is now EOL.
See NixOS#372476 (comment)
konradmalik added a commit to konradmalik/nixpkgs that referenced this pull request Jan 12, 2025
@konradmalik
roslyn-ls: 4.13.0-3.24577.4 -> 4.13.0-3.25051.1
c560586 
Added a git-patch to drop net6.0 dependency, which is now EOL.
See NixOS#372476 (comment)
konradmalik added a commit to konradmalik/nixpkgs that referenced this pull request Jan 13, 2025
@konradmalik
roslyn-ls: 4.13.0-3.24577.4 -> 4.13.0-3.25051.1
d9b2b97 
Added a git-patch to drop net6.0 dependency, which is now EOL.
See NixOS#372476 (comment)
@konradmalik
Copy link
Contributor

konradmalik commented Jan 13, 2025 •
edited
Loading

@corngood I've tried to update _MsbuildVersion to 17.9.8, then to 17.11.9, and both attempts failed. Build crashes with missing namespaces in Microsoft.Build.

I ended up applying your patch directly (without MsbuildVersion changes) and it seems to compile and work fine. I've tested the basic functionality in Neovim.

see #373293

@corngood
Copy link
Contributor

superseded by #373293

@corngood corngood closed this Jan 13, 2025
nixpkgs-ci bot pushed a commit that referenced this pull request Jan 13, 2025
@konradmalik @github-actions
roslyn-ls: 4.13.0-3.24577.4 -> 4.13.0-3.25051.1
333eb49 
Added a git-patch to drop net6.0 dependency, which is now EOL.
See #372476 (comment)

(cherry picked from commit d9b2b97)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@konradmalik konradmalik Awaiting requested review from konradmalik

Assignees
No one assigned
Labels
10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tris203 @konradmalik @GGG-KILLER @corngood