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

dnspy: init at 6.5.1 #348390

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from
Draft

dnspy: init at 6.5.1 #348390

wants to merge 8 commits into from

Conversation

js6pak
Copy link
Contributor

@js6pak js6pak commented Oct 14, 2024

https://github.com/dnSpyEx/dnSpy

dnSpy(Ex) is a decompiler frontend and debugger for .NET assemblies. One small issue is that it's windows-only which makes this package a little cursed, but hey, it works. :)

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/)
  • 24.11 Release Notes (or backporting 23.11 and 24.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.

Comment on lines 38877 to 38885

dnspy = callPackage ../by-name/dn/dnspy/package.nix {
wine = wine64Packages.staging;
};

dnspy32 = callPackage ../by-name/dn/dnspy/package.nix {
wine = winePackages.staging;
is32bit = true;
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a better by-name-y way to do this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we have both 32 and 64 bit versions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For debugging you need the same process bitness as the process you want to debug.
I think we could get away with compiling dnSpy as non-self-contained once and then making 2 scripts that use win-x64/win-x86 dotnet-runtime, but upstream releases as 2 self-contained zips currently.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was just curious. it sounds like there's a good reason for it.

As for the by-name thing:

  • what does it mean to have dnspy in all-packages as well as by-name? I don't think I've seen that before. which takes precedence?

  • I'd probably just add by-name/dn/dnspy32/package.nix and use override in it

Copy link
Contributor

@corngood corngood Oct 14, 2024

Choose a reason for hiding this comment

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

I guess logically all-packages takes precedence. And I see why it would be tricky to do this in by-name with the way wine is handled.

It's probably better for it not to be in by-name at all if it's going to be like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what does it mean to have dnspy in all-packages as well as by-name? I don't think I've seen that before

A lot of packages do that already1 and it's documented in by-name docs.

Footnotes

  1. image

Comment on lines 52 to 53
(fetchNuGet { pname = "Microsoft.WindowsDesktop.App.Ref"; version = "8.0.8"; hash = "sha256-Gd3Y0mpvG9lADIr6o/E8Ab3Efml9RynKxVJCgBq+H9w="; })
(fetchNuGet { pname = "Microsoft.WindowsDesktop.App.Runtime.win-x64"; version = "8.0.8"; hash = "sha256-N78uhT3zWQIpl7TmVIuwhG7IIt0ti8+DUFGMxogOT7k="; })
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@corngood This is an implicit SDK reference when you enable UseWpf/UseWindowsForms, and it's not covered by targetPackages currently

Copy link
Contributor

Choose a reason for hiding this comment

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

I think adding these to targetPackages (via the dotnet binary update script) would be reasonable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 6cf9b69, but it will need to be run alongside a full dotnet update

Copy link
Contributor

Choose a reason for hiding this comment

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

it will need to be run alongside a full dotnet update

why do you say that? what you have looks good to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, is it just because it still needs to be run on 6 and 9?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, is it just because it still needs to be run on 6 and 9?

Yeah, what I've done is pkgs/development/compilers/dotnet/update.sh 8.0.8 and just copy pate the targetPackages (I couldn't copy the whole file because the sdks got bumped).
I wanted to do it for the others aswell but running update.sh with preview label didn't work so I thought it's going to be less effort to run it with/after #348077

Comment on lines 41 to 42
(fetchNuGet { pname = "Microsoft.NETCore.App.Host.win-x64"; version = "8.0.8"; hash = "sha256-pkfKvNeb779TUp9jp19peJjCXK3NGpexaFjWwc3dSBo="; })
(fetchNuGet { pname = "Microsoft.NETCore.App.Runtime.win-x64"; version = "8.0.8"; hash = "sha256-C0zUiMMAQ9nd1n7PDVoBhCShHzdGI67YFySTpeFH8uE="; })
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@corngood What would be the best way to use windows target packages here (#343837)? Can't it be simply picked up from the runtimeId attribute? Or am I supposed to use nixpkgs cross compiling machinery here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or am I supposed to use nixpkgs cross compiling machinery here?

In theory building the unwrapped dotnet module cross compiled to windows would be the way to do it. You'd want the supported platforms to be windows 64 and 32-bit, which would make the target packages for both available at fetch time.

None of that is well tested though, so I wouldn't be surprised if you had trouble.

    addNuGetDeps {
      inherit nugetDeps;
      overrideFetchAttrs =
        old:
        lib.optionalAttrs ((args'.runtimeId or null) == null) rec {
          dotnetRuntimeIds = map (system: dotnetCorePackages.systemToDotnetRid system) meta.platforms;
          buildInputs =
            old.buildInputs
            ++ lib.concatLists (lib.attrValues (lib.getAttrs dotnetRuntimeIds dotnet-sdk.targetPackages));
        };
    } args'' finalAttrs

dotnetRuntimeIds in the above (from buildDotnetModule) is where it determines the list of target RIDs needed for restore. An alternative to actually cross-targeting the whole derivation would be to use [ "win-x86" "win-x64" ] there, to match the explicit runtimeId.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

None of that is well tested though, so I wouldn't be surprised if you had trouble.

I had to make 5a01bbd but other than that, it seems to work.

In theory building the unwrapped dotnet module cross compiled to windows would be the way to do it.

In 6f51f2d I just used a cross dotnet sdk instead making the whole unwrapped module cross compile, is that fine?

An alternative to actually cross-targeting the whole derivation would be to use [ "win-x86" "win-x64" ] there, to match the explicit runtimeId.

For this package specifically it would actually be either [ "win-x64" ] or [ "win-x86"], not both. So I think it would work fine if the code you mentioned just added dotnet-sdk.targetPackages.${args'.runtimeId} to buildInputs, no?
Generally why is runtimeId even a thing? Isn't it redundant when the concept of targetPlatform exists in nixpkgs? One advantage that I think it has is that unlike gcc, dotnet-sdk can have multiple target platforms installed at a time, so a single buildDotnetModule could in theory output binaries for different platforms (say a C# IDE that compiles remote debugger agents for a lot of different platforms as part of its build process). But dotnetRuntimeIds isn't even exposed and runtimeId is not a list.

Copy link
Contributor

Choose a reason for hiding this comment

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

For this package specifically it would actually be either [ "win-x64" ] or [ "win-x86"], not both. So I think it would work fine if the code you mentioned just added dotnet-sdk.targetPackages.${args'.runtimeId} to buildInputs, no?

I was thinking you could have the 'unwrapped' derivation have both platforms in meta.platforms, which would allow a single deps.nix to be populated by restoring for both platforms. In practice it might not matter, if nothing platform specific ends up in deps.nix.

Generally why is runtimeId even a thing? Isn't it redundant when the concept of targetPlatform exists in nixpkgs?

It probably is redundant now. I could imagine a list potentially being useful, but I'd want to make sure it interacts in a sensible way with fetch-deps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

a single deps.nix to be populated by restoring for both platforms

That feels incorrect though, unless deps.nix had a way to make some dependencies only downloaded on specific rids.

It probably is redundant now.

So we should pick one and remove the other?

Copy link
Contributor

Choose a reason for hiding this comment

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

That feels incorrect though, unless deps.nix had a way to make some dependencies only downloaded on specific rids.

That's the way it works for all packages right now. The deps.nix always contains the dependencies for all supported platforms. You're right that it would be better to track where packages are actually used, and that's something I'd like us to do eventually (similar to how targetPackages was recently added to the sdks).

So we should pick one and remove the other?

I'm not sure there's a better option than what's in this PR. I'm playing around with rearranging this so that is cross compiles unwrapped instead of just using the cross sdk, but I think it's going to require more build-support fixes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking you could have the 'unwrapped' derivation have both platforms in meta.platforms

I've looked into doing this briefly and looks like buildDotnetModule will need a lot more changes for proper cross compilation

Copy link
Contributor Author

@js6pak js6pak Oct 15, 2024

Choose a reason for hiding this comment

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

Just realized you commented before I refreshed

I'm playing around with rearranging this so that is cross compiles unwrapped

Well... me too. Do you maybe have a matrix/discord? :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm on matrix @[username]:[username].com. A nixpkgs-dotnet channel is probably a decent idea. I mostly gave up lurking on the big nix channels.

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.

2 participants