dotnet: implement autoPatchcilHook #373107
Conversation
|
50f7fa2 to
6313b38
Compare
|
A new change that I made with patchcil 0.2.1 is that now it uses Native AOT for a 200% speed improvement (albeit at the cost of a 5x size increase from 704K to 3.8M). v0.2.0: v0.2.1 (No AOT): v0.2.1 (AOT): |
6313b38 to
3ce5e1f
Compare
|
3ce5e1f to
e8c6de5
Compare
|
I'd like to get this merged, but I'd prefer to remove:
I don't think it sets a great example, and it's not coming from a maintainer. |
What are your apprehensions regarding it? Why would you say it's not a great example? Also, speaking of maintainers, @AngryAnt is one of the package's maintainers and didn't seem to have any feedback on the changes in |
It's way more verbose. Transitive dependencies could require changes to be made in an update. Keep in mind that the package should work with this change: diff --git a/pkgs/applications/misc/avalonia-ilspy/default.nix b/pkgs/applications/misc/avalonia-ilspy/default.nix
index 69aeb11e2bbd..b6bedeb8821f 100644
--- a/pkgs/applications/misc/avalonia-ilspy/default.nix
+++ b/pkgs/applications/misc/avalonia-ilspy/default.nix
@@ -4,15 +4,6 @@
fetchFromGitHub,
buildDotnetModule,
dotnetCorePackages,
- libX11,
- libICE,
- libSM,
- libXi,
- libXcursor,
- libXext,
- libXrandr,
- fontconfig,
- glew,
makeDesktopItem,
copyDesktopItems,
icoutils,
@@ -52,25 +43,6 @@ buildDotnetModule rec {
autoSignDarwinBinariesHook
];
- buildInputs = [
- # Dependencies of nuget packages w/ native binaries
- (lib.getLib stdenv.cc.cc)
- fontconfig
- ];
-
- runtimeDeps = [
- # Avalonia UI
- libX11
- libICE
- libSM
- libXi
- libXcursor
- libXext
- libXrandr
- fontconfig
- glew
- ];
-
postInstall =
''
icotool --icon -x ILSpy/ILSpy.icoIf I was the maintainer, I wouldn't want this extra burden. At least not without a clear benefit. |
Yes, since the transitive dependencies aren't packaged in nixpkgs that's how things work in nixpkgs. Just look at native programs (or even Java programs) that have transitive dependencies packaged together with them and it'll be exactly the same,
Totally disagree, there's no mention at all of an avalonia package which would be the build/patching for avalonia to work in nixpkgs. A better example would be (and then avalonia would be gone from diff --git a/pkgs/applications/misc/avalonia-ilspy/default.nix b/pkgs/applications/misc/avalonia-ilspy/default.nix
index 69aeb11e2bbd..b6bedeb8821f 100644
--- a/pkgs/applications/misc/avalonia-ilspy/default.nix
+++ b/pkgs/applications/misc/avalonia-ilspy/default.nix
@@ -4,15 +4,6 @@
fetchFromGitHub,
buildDotnetModule,
dotnetCorePackages,
+ dotnetPackages,
- libX11,
- libICE,
- libSM,
- libXi,
- libXcursor,
- libXext,
- libXrandr,
- fontconfig,
- glew,
makeDesktopItem,
copyDesktopItems,
icoutils,
@@ -52,25 +43,6 @@ buildDotnetModule rec {
autoSignDarwinBinariesHook
];
buildInputs = [
+ dotnetPackages.avalonia
- # Dependencies of nuget packages w/ native binaries
- (lib.getLib stdenv.cc.cc)
- fontconfig
];
-
- runtimeDeps = [
- # Avalonia UI
- libX11
- libICE
- libSM
- libXi
- libXcursor
- libXext
- libXrandr
- fontconfig
- glew
- ];
-
postInstall =
''
icotool --icon -x ILSpy/ILSpy.icoBut we aren't there yet so this is needed.
The maintainer hasn't voiced against it so I don't see any problems. |
|
I think it's a worse packaging experience than before, even if you ignore overrides. I think we've both made our points on this, and we're just talking in circles now. The rest of this PR is fine, and I would love to replace runtime deps for avalonia with it, but I still think overrides is the place to do it. |
It's not a worse packaging experiece, it just removes the magic that shouldn't be there to start with (as nothing in nixpkgs does the same nor should).
Okay, I'll just bite the bullet since overrides will be nuked after the nuget deps rework PR I'm planning anyways. |
e8c6de5 to
fe2166a
Compare
I fundamentally disagree with you on this, so I would really like some feedback from package maintainers. There's a derivation which outputs something like
In either of these cases I think the dll should be patched in store. This makes it quite simple to switch between them if necessary. I'm not against changing Also I've mentioned it above, but if you leave patching until the fixup phase of the leaf package, those assemblies will not work in build/check, etc. |
Were it option number 1 I'd be perfectly fine with it because it'd be explicit that you're requesting another package from nixpkgs (with its own build script and patches to work in nix world as it is expected whenever you add a dependency to another package) so it having its own modifications is expected.
No, it should only be done in the first, because the 2nd is a fetcher so it should just do that. It is a fetcher so it should only fetch the package, no extra implicit magic to make things "easier".
There's no "abstracting" to be done here. If you want extra processing then you use something else (like a nixpkgs package that wraps avalonia.x11 to patch it, some setup hook like
Yes, however it isn't needed in every case, and if users need to do patching beforehand, they can do so in the EDIT: |
The abstraction was done in #339953. That concept got approval and upvotes from a bunch of devs. Nobody brought up any concerns. I think this is getting off-topic for this PR, but it's an important discussion, so we should have it somewhere. |
I know, and I was one of the people that approved it. What I meant was that the ideal scenario would be for no abstraction to exist here.
I agree, also I apologize if I'm getting too long-winded or ended up sounding rude, I ended up getting a bit heated but mostly because I was frustrated at my own incapability at getting my point across. I think I did a good job on the previous comment so I'll copy it to another issue so we can move this there. I went ahead and removed the troubling commit, so everything should be fine to merge. |
Adds a new tool called
patchcilwhich aims to be the CIL (.NET's intermediate language) equivalent ofpatchelf.Also implements a new hook called
autoPatchcilHookwhich uses thepatchciltool to rewrite all static (read:[DllImport]-based) native library uses to point to the correct location in the nix store.Finally, rewritesavalonia-ilspyto useautoPatchcilHookas a (rather extreme, as it uses quite a few Xorg libraries, GTK 3 and OpenGL) test case.cc: @NixOS/dotnet @js6pak @AngryAnt @emilytrau
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.