-
-
Notifications
You must be signed in to change notification settings - Fork 15k
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
dotnet: infrastructure updates #361563
dotnet: infrastructure updates #361563
Changes from all commits
7b508c6
7f4c05f
535cfda
786747c
3fdc09c
70bf01c
81dd644
95d7a5a
0dcba5f
abd7695
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -35,6 +35,8 @@ lib.makeOverridable ( | |
; | ||
}; | ||
|
||
strictDeps = true; | ||
|
||
nativeBuildInputs = [ | ||
unzip | ||
patchNupkgs | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,10 +20,7 @@ buildDotnetModule rec { | |
fetchSubmodules = true; # It vendors BSIPA-Linux | ||
}; | ||
|
||
dotnet-sdk = with dotnetCorePackages; combinePackages [ | ||
sdk_7_0 | ||
sdk_6_0 | ||
]; | ||
dotnet-sdk = dotnetCorePackages.sdk_7_0; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same, and #339370 will cause a merge conflict |
||
|
||
dotnet-runtime = dotnetCorePackages.runtime_7_0; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -36,11 +36,10 @@ buildDotnetModule (finalAttrs: rec { | |
|
||
dotnet-sdk = | ||
with dotnetCorePackages; | ||
combinePackages [ | ||
sdk_6_0 | ||
sdk_7_0 | ||
sdk_8_0 | ||
]; | ||
sdk_8_0 | ||
// { | ||
inherit (sdk_6_0) packages targetPackages; | ||
}; | ||
Comment on lines
37
to
+42
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is more confusing than the earlier Should probably create a helper function that does this and documents its behavior and when to use if we do decide to move forward with this pattern. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The idea is that we don't actually need more than one SDK, but we do need the targeting packages. This breaks the dependencies on the insecure packages. It does feel messy. I started working on a bigger overhaul of how nuget packages are referenced, but it's not close to being ready. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The issue is that the packages themselves being used are also insecure, this feels like just swiping it under the rug as opposed to actually fixing the problem. This one is gonna be a hard no from me, considering that many .NET CVEs were on SDK/runtime packages and not on the compiler/runtime itself. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For net 6, it definitely is just sweeping it under the rug, but I still think it's better for things to only depend on the SDKs they use, instead of using I'd be happy to mark all net6/7 nuget packages as insecure, but that's a separate problem IMO. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Another question, shouldn't we keep the 8.0 packages here (and elsewhere where this is used)? And couldn't the instances where this uses And if possible, I'd like to have the packages be marked as insecure before we start actually using them so this doesn't end up unmarking packages as insecure and people mistakenly think these are safe to use. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm just having a go at this, by applying
It's going to be really painful for anyone who wants to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, there's no way around it unfortunately. I think making it easy for people to use insecure packages isn't an objective of nixpkgs, but worst case they can do something like nixpkgs.config.permittedInsecurePackages = lib.map (pkg: pkg.name) pkgs.dotnetCorePackages.sdk_6_0.packages; There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess you could make a pseudo-package referenced by the sdk, runtime, nuget packages, and mark that as insecure? I don't know if it's worth the trouble though.
You'd also need to bring in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Sounds like too much work for something we plan to remove in the long term, but it would make the deprecation process easier perhaps. If it's easy enough we could do it to make it easier on us to deprecate them as well.
Yeah, but I'd say allowing usage of insecure things is a non-objective. If people really want to use them, it should be doable but I wouldn't go out of my way to make it easy. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I actually don't think it would work anyway. I know you need to add the wrapper packages to |
||
dotnet-runtime = dotnetCorePackages.sdk_8_0; | ||
|
||
projectFile = "src/FsAutoComplete/FsAutoComplete.fsproj"; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,12 +20,7 @@ buildDotnetModule rec { | |
projectFile = "main/GarnetServer/GarnetServer.csproj"; | ||
nugetDeps = ./deps.nix; | ||
|
||
dotnet-sdk = | ||
with dotnetCorePackages; | ||
combinePackages [ | ||
sdk_6_0 | ||
sdk_8_0 | ||
]; | ||
dotnet-sdk = dotnetCorePackages.sdk_8_0; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same thing with this package too. Also the same change is being done in #361658 |
||
dotnet-runtime = dotnetCorePackages.runtime_8_0; | ||
|
||
dotnetBuildFlags = [ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -36,6 +36,8 @@ let | |
hash = artifactsHash; | ||
}; | ||
|
||
strictDeps = true; | ||
|
||
sourceRoot = "."; | ||
|
||
installPhase = '' | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for my lack of knowledge, I tried looking at the source for
mkDerivation
but I couldn't understand what this flag does, and #353937 hasn't been done to document what it does.Also, won't this break out-of-tree packages using
buildDotnetModule
? We should probably document this breaking change.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This came from:
#358824
You're right that it needs documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh okay, makes sense. Saw another PR that adds a little description to it so now I have an idea how it works.