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

ilspy: init at 7.0-rc2 #91255

Closed
wants to merge 1 commit into from
Closed

ilspy: init at 7.0-rc2 #91255

wants to merge 1 commit into from

Conversation

mausch
Copy link
Member

@mausch mausch commented Jun 21, 2020

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/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

sha256 = "1p1fkdkpbcqj2q4n7y241lzx98w33vmh4n2gahw7lbrrg1p7rxxa";
};

installPhase = ''
Copy link
Member

Choose a reason for hiding this comment

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

Why not build it from source?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi, and thanks for reviewing! I didn't build from source because I don't need it, it's more work, and I don't even have any other platforms to test things.

Copy link
Member

Choose a reason for hiding this comment

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

For nixpkgs generally build from source, since it is then linked correctly (avoids the LD_LIBRARY_PATH environment variable) and we can support platforms as they are added to NixOS. Additionally we can catch more NixOS-related issues of the package while building which often only show themselves when running in the case of prebuilt packages.

Overall the resulting packages are also cleaner and more transparent as to what is going on.

I understand it is more work and it often can be kind of tricky to get more obscure build systems to work with nix. If you don't want to rework it for NixOS, you can still use callPackage from your configuration.nix to use it on your system or override your nixpkgs.

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately I don't think I'll spend any more time on this any time soon. It does work so I think it should be merged (i.e. I think it's better than not even having a package for this), but if it might cause trouble for maintainers I understand why maintainers wouldn't want to merge it.

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough, as said you can always use overriding to add local packages to nixpkgs for your system.

pkgs/development/tools/ilspy/default.nix Show resolved Hide resolved
pkgs/development/tools/ilspy/default.nix Outdated Show resolved Hide resolved
mv * $out/bin

wrapProgram $out/bin/ILSpy \
--prefix LD_LIBRARY_PATH : "${stdenv.lib.makeLibraryPath libs}"
Copy link
Member

Choose a reason for hiding this comment

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

Probably worth it to also install a matching .desktop file and install it to better integrate it with desktop environments.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added one in e85a1c1 but tbh I have no idea what I'm doing here... I use i3 so it's been many years since I even consumed these things I think.

pkgs/development/tools/ilspy/default.nix Outdated Show resolved Hide resolved
pkgs/development/tools/ilspy/default.nix Outdated Show resolved Hide resolved
pkgs/development/tools/ilspy/default.nix Outdated Show resolved Hide resolved
pkgs/development/tools/ilspy/default.nix Outdated Show resolved Hide resolved
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-in-distress/3604/31

maintainers/maintainer-list.nix Outdated Show resolved Hide resolved
pkgs/development/tools/ilspy/default.nix Outdated Show resolved Hide resolved
pkgs/development/tools/ilspy/default.nix Outdated Show resolved Hide resolved
$out/share/applications \
$out/share/pixmaps
ln -s ${icon} $out/share/pixmaps/ILSpy.ico
mv * $out/bin
Copy link
Member

Choose a reason for hiding this comment

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

Are there so many executables? Maybe also use install to remove the chmod.

Copy link
Member Author

Choose a reason for hiding this comment

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

As with most .NET apps there's one entrypoint executable plus lots of dll files and other auxiliary files that need to be in the same directory.

Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be a good idea to move them into some lib/libexec etc directory and build a wrapper around the main executable? Polluting $PATH with many files even if not executable is not that great.

@mausch mausch force-pushed the ilspy branch 3 times, most recently from 9ec6102 to 346fb17 Compare February 27, 2021 15:00
@stale
Copy link

stale bot commented Sep 3, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Sep 3, 2021
@mausch mausch marked this pull request as draft September 4, 2021 16:10
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Sep 4, 2021
@mausch mausch force-pushed the ilspy branch 2 times, most recently from f3f9765 to 49aed12 Compare September 4, 2021 16:44
@mausch mausch changed the title ilspy: init at 5.0-rc2 ilspy: init at 7.0-rc2 Sep 4, 2021
@stale
Copy link

stale bot commented Apr 18, 2022

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Apr 18, 2022
@emilytrau
Copy link
Member

Closing as we now have avalonia-ilspy in nixpkgs

@emilytrau emilytrau closed this Jul 22, 2023
@mausch mausch deleted the ilspy branch July 23, 2023 09:38
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.

None yet

5 participants