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

imhex: init at 1.19.3 #184488

Merged
merged 1 commit into from Oct 16, 2022
Merged

imhex: init at 1.19.3 #184488

merged 1 commit into from Oct 16, 2022

Conversation

peterhoeg
Copy link
Member

@peterhoeg peterhoeg commented Aug 1, 2022

Description of changes

Further to #148646

Closes #105997
Closes #148646

A slightly more elegant approach compared to symlinking the plugins dir to $out/bin would be to patch things up to look for it in the right place.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 22.11 Release Notes (or backporting 22.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@peterhoeg
Copy link
Member Author

Just to elaborate on why #148646 is not sufficient:

  1. here we use as much as possible from nixpkgs instead of building the vendored dependencies
  2. we use the install infrastructure instead of manually overriding it completely

Copy link
Member

@Vonfry Vonfry left a comment

Choose a reason for hiding this comment

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

The bin can work for me. Here are some suggestions on the pr.

hash = "sha256-SFv5ulyjm5Yf+3Gpx+A74so2YClCJx1sx0LE5fh5eG4=";
};

nativeBuildInputs = [ cmake llvm python3 perl pkg-config xlibsWrapper ];
Copy link
Member

Choose a reason for hiding this comment

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

Are perl and xlib really necessary? After removing them, I can build and run this package as well.

mbedtls
nlohmann_json
yara
];
Copy link
Member

Choose a reason for hiding this comment

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

Some of them are not found by rg -i ... in sources such as jansson. Are they necessary? OpenGL and freetype included by cmake find_package are not listed here. They may be input from other dependencies, but it is better to add it here explicitly.

Copy link
Member

Choose a reason for hiding this comment

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

By the way, could we seperate plugins from this drv like pass, fcitx5 and others or add some options to control it?

"-DUSE_SYSTEM_YARA=ON"
];

# for reasons unknown, the built-in plugin isn't found unless made available under $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.

Please check the source here which shows the reason. I think we should patch or make a pr to let add the share dir into default because plugins are better put there instead of bin, IMO.

"-DUSE_SYSTEM_LLVM=ON"
"-DUSE_SYSTEM_NLOHMANN_JSON=ON"
"-DUSE_SYSTEM_YARA=ON"
];
Copy link
Member

@Vonfry Vonfry Aug 2, 2022

Choose a reason for hiding this comment

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

should extra dirs be updated or patched for nix?

}:

let
# when bumping the version, check if imhex has gotten support for the capstone version in nixpkgs
Copy link
Member

Choose a reason for hiding this comment

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

Could we comment the compatible version or update it?

@WerWolv
Copy link

WerWolv commented Aug 3, 2022

Hi! Thanks for adding ImHex to Nix!
If there's anything you'd need me to add / change in ImHex itself, please contact me either on Discord (WerWolv#1337) or open an issue on my GitHub repo. I'm more than happy to provide direct support for things so you don't need to make and maintain Nix-only patches

@WerWolv WerWolv mentioned this pull request Aug 3, 2022
1 task
@bobvanderlinden bobvanderlinden mentioned this pull request Aug 4, 2022
10 tasks
@bobvanderlinden
Copy link
Member

Awesome! I was just looking for ImHex in Nixpkgs and found this PR. Thank you!

I tried this on my NixOS machine. I haven't used ImHex before. The core functionality seems to be working nicely, but the file dialogs are not working. For example clicking File -> Open File... doesn't show anything. In addition, it seems the patterns are not automatically loaded, for instance, opening an ELF file using command-line didn't show anything in the "pattern data" panel.

I just couldn't figure out how to debug these issues properly and wasn't sure this was related to this Nix package in particular.

@newAM
Copy link
Member

newAM commented Aug 5, 2022

The core functionality seems to be working nicely, but the file dialogs are not working. For example clicking File -> Open File... doesn't show anything.

I tested this on my gnome system and my KDE system and the file dialog worked for me on both. What desktop environment are you using?

@bobvanderlinden
Copy link
Member

I tested this on my gnome system and my KDE system and the file dialog worked for me on both.

Ah maybe it isn't a packaging problem.

What desktop environment are you using?

i3 on xserver. I guess the file dialog not working can be ignored.

@WerWolv
Copy link

WerWolv commented Aug 8, 2022

The file dialog not working correctly on some systems is a known "issue". It's because ImHex uses xdg-desktop-portal for it and that's somehow not correctly setup on some systems or doesn't have a frontend available.
I don't really know how to fix it either but I'd be interested on a global fix too that I could implement

@bobvanderlinden
Copy link
Member

The file dialog not working correctly on some systems is a known "issue". It's because ImHex uses xdg-desktop-portal for it and that's somehow not correctly setup on some systems or doesn't have a frontend available. I don't really know how to fix it either but I'd be interested on a global fix too that I could implement

I can confirm it works with xdg-desktop-portal-gtk enabled 👍 Loading patterns also just opens the file dialog in my home directory, instead of the directory where the patterns are located. That said, it already is quite usable the way it is right now.

@peterhoeg
Copy link
Member Author

@WerWolv thanks for both imhex and supporting the packaging efforts here.

It would be fantastic if imhex supported looking for plugins under PREFIX/share/imhex (or similar) out of the box so that the the files installed through make install are available/usable.

@WerWolv
Copy link

WerWolv commented Aug 10, 2022

@peterhoeg We already added this to make bundling ImHex for Fedora easier. As far as I know it came with v1.20.0 so the nix package here needs to be updated first.
After that you can simply set the IMHEX_PLUGINS_IN_SHARE cmake option and it should put and load all plugins in share/imhex/plugins :)

@peterhoeg
Copy link
Member Author

peterhoeg commented Oct 11, 2022 via email

@iTrooz
Copy link

iTrooz commented Oct 12, 2022

It would be fantastic if ImHex supported looking for plugins under PREFIX/share/imhex (or similar) out of the box so that the the files installed through make install are available/usable.

Do you mean the make install of ImHex ? It installs plugins under PREFIX/[lib|lib64]/imhex/plugins by default for almost every package now (see the release artifacts). Only the AppImage uses PREFIX/share/imhex (with IMHEX_PLUGINS_IN_SHARE) for some reason related to path hardcoding (I don't really remember)

Basically if we can have an option to provide the path for the plugins (and the rest from ImHex-Patterns) at build-time, that would be great.

I personally think it is a bad idea to add another location for the plugins, because we would end up confusing people making plugins, and make overall packaging harder (I understand it's not my decision to make through). Is there a reason why this would help you, rather than using PREFIX/[lib|lib64]/imhex/plugins ?

EDIT : Relevant code : https://github.com/WerWolv/ImHex/blob/dd9b6643e6e535765f8b464c4c35d557bdd64e44/cmake/build_helpers.cmake#L100-L106

Copy link
Contributor

@Qubasa Qubasa left a comment

Choose a reason for hiding this comment

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

Works as intended. Improvements can be made incrementally with new pull requests.

@peterhoeg peterhoeg merged commit 751e222 into NixOS:master Oct 16, 2022
@peterhoeg peterhoeg deleted the p/imhex branch October 16, 2022 14:17
@panicgh panicgh mentioned this pull request Oct 25, 2022
13 tasks
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.

ImHex package request
8 participants