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.

Sign up for GitHub

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

Jump to bottom

rmlint: fix running rmlint --gui #90267

Merged
merged 1 commit into from Jun 18, 2020
Merged

rmlint: fix running rmlint --gui #90267

merged 1 commit into from Jun 18, 2020

Conversation

flokli
Copy link
Contributor

@flokli flokli commented Jun 13, 2020

rmlint has a --gui option which is writing a python "bootstrap" script
in /tmp.

It currently can't find its own python modules, as we don't prefix
PYTHONPATH to point to it. Also, there's some other runtime
dependencies, mostly due to using gtk from python(3), which also needs
to be preprended to PYTHONPATH.

rmlint also executes itself again with --version to determine some
features, for which we need to prefix PATH with $out/bin.

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.

cc @k0ral

@flokli flokli requested review from Mic92, bachp and jtojnar June 13, 2020 23:37
@flokli
Copy link
Contributor Author

flokli commented Jun 13, 2020

I'm not sure if we should put all these additions behind a withGui argument - it's not a separate binary here, but the (already existing) option is broken :-/

@k0ral
Copy link
Contributor

k0ral commented Jun 14, 2020

From documentation:

If you compiled rmlint from source, scons will try to build and install the GUI, except you pass --without-gui to it.

Therefore, a withGui derivation flag would definitely make sense, wouldn't it ?

Mic92
Mic92 reviewed Jun 14, 2020
View reviewed changes
pkgs/tools/misc/rmlint/default.nix Outdated
];

prefixKey = "--prefix=";

postInstall = ''
wrapProgram $out/bin/rmlint \
Copy link
Member

Choose a reason for hiding this comment

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

We usually use wrapPython for this.

nativeBuildInputs = [
  pythonPackages.wrapPython
];

  pythonPath = [ python3.pkgs.pygobject3 python3.pkgs.pycairo ];
  # not sure if this line is needed, wrapPythonProgramsIn might already prefix `$out/bin`.
  makeWrapperArgs = ["--prefix" "PATH" ":" "$out/bin"];
  postFixup = ''
    wrapPythonProgramsIn "$out/bin/rmlint" "$out $pythonPath"
  '';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wrapPythonProgramsIn doesn't work here, as it checks for a python shebang, and $out/bin/rmlint itself is no python program.

@jtojnar
Copy link
Contributor

jtojnar commented Jun 14, 2020 •
edited

So weird, why do not they just build a separate shredder executable? Is there an upstream issue? Then we could make a separate package for shredder using buildPythonApplication.

But yeah, I would use wrapPython as suggested by @Mic92 and https://nixos.org/nixpkgs/manual/#ssec-gnome-common-issues-double-wrapped

@flokli
Copy link
Contributor Author

flokli commented Jun 14, 2020

But yeah, I would use wrapPython as suggested by @Mic92 and https://nixos.org/nixpkgs/manual/#ssec-gnome-common-issues-double-wrapped

wrapPython also won't work, as it assumes to be handling a python script (and prepends to it), which of course doesn't work with ELF binaries…

@flokli
Copy link
Contributor Author

flokli commented Jun 15, 2020

@jtojnar, @Mic92 did I oversee something, or is the wrapProgram the only approach possible here?

In that case, I'd go with that (and add a withGui option and pass --without-gui conditionally, as suggested in #90267 (comment)).

@jtojnar
Copy link
Contributor

jtojnar commented Jun 16, 2020

Yeah, that sounds right. If you want to prevent double wrapping, you can also go the other way round with gappsWrapperArgs+=(--prefix PYTHONPATH : …).

I would still prefer working with upstream to decouple the GUI into a separate executable and possibly package, though.

@flokli
Copy link
Contributor Author

flokli commented Jun 17, 2020

I pushed a new version, preventing the double wrapping as recommended in #90267 (comment).

@flokli flokli mentioned this pull request Jun 17, 2020
Open
@flokli
Copy link
Contributor Author

flokli commented Jun 17, 2020

I opened an issue at sahib/rmlint#419 about moving the GUI into a separate binary.

After some thinking, I'll also flip withGui to false for the time being.

I could imagine people wanting to run this on headless systems, without pulling in all of GTK+.

@flokli
rmlint: fix running rmlint --gui
35040cf 
rmlint has a `--gui` option which is writing a python "bootstrap" script
in `/tmp`.

It currently can't find its own python modules, as we don't prefix
`PYTHONPATH` to point to it. Also, there's some other runtime
dependencies, mostly due to using gtk from python(3), which also needs
to be preprended to `PYTHONPATH`.

The `rmlint` GUI components also execute `rmlint` again with `--version`
to determine some features, for which we need to prefix `PATH` with
`$out/bin`.
@flokli flokli mentioned this pull request Jun 17, 2020
Open
@flokli flokli merged commit 79e8ba7 into NixOS:master Jun 18, 2020
@flokli flokli deleted the rmlint-gui branch June 18, 2020 22:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mic92 Mic92 Mic92 left review comments

@bachp bachp Awaiting requested review from bachp

@jtojnar jtojnar Awaiting requested review from jtojnar

Assignees
No one assigned
Labels
10.rebuild-darwin: 0 10.rebuild-linux: 1-10 10.rebuild-linux: 1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@flokli @k0ral @jtojnar @Mic92