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

dupeguru: init at 4.0.4 #75054

Merged
merged 2 commits into from Dec 13, 2019
Merged

dupeguru: init at 4.0.4 #75054

merged 2 commits into from Dec 13, 2019

Conversation

@novoxudonoser
Copy link
Member

@novoxudonoser novoxudonoser commented Dec 5, 2019

Motivation for this change

Add dupeGuru programm https://dupeguru.voltaicideas.net .

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nix-review --run "nix-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.
@novoxudonoser novoxudonoser force-pushed the novoxudonoser:dupeguru branch 2 times, most recently from 559e18c to 4a3cd3f Dec 5, 2019
@novoxudonoser
Copy link
Member Author

@novoxudonoser novoxudonoser commented Dec 5, 2019

Just found that I need to rewrite source fetching, from fetchGit to fetchFromGitHub with submodules. (fixed)

@novoxudonoser
Copy link
Member Author

@novoxudonoser novoxudonoser commented Dec 9, 2019

Please review, I don't know how long it takes to get a review in nixpkgs, but there are now auto-suggested reviewers. Maybe @doronbehar can help.

@doronbehar
Copy link
Contributor

@doronbehar doronbehar commented Dec 9, 2019

@novoxudonoser novoxudonoser requested a review from doronbehar Dec 10, 2019
@novoxudonoser
Copy link
Member Author

@novoxudonoser novoxudonoser commented Dec 11, 2019

@doronbehar I don't know - but what next? Do I need to wait for this pr to be merged? Or I need to mention someone ho can add his review and merge? (I am quite new to nixpkgs)

@doronbehar
Copy link
Contributor

@doronbehar doronbehar commented Dec 11, 2019

It's OK, welcome aboard the Caos 😺. You can mention maybe @jonringer or @worldofpeace if days pass and nothing happens but you better get used to this - PRs can take infinitely long time to be merged..

pkgs/applications/misc/dupeguru/default.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/dupeguru/default.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/dupeguru/default.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/dupeguru/default.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/dupeguru/default.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/dupeguru/default.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/dupeguru/default.nix Outdated Show resolved Hide resolved
@novoxudonoser
Copy link
Member Author

@novoxudonoser novoxudonoser commented Dec 11, 2019

It's OK, welcome aboard the Caos . You can mention maybe @jonringer or @worldofpeace if days pass and nothing happens but you better get used to this - PRs can take infinitely long time to be merged..

@doronbehar thanks for your review and help.

#dontWrapQtApps = true;

#preFixup = ''
# makeWrapperArgs+=("''${gappsWrapperArgs[@]}")

This comment has been minimized.

@jtojnar

jtojnar Dec 13, 2019
Contributor

  dontWrapQtApps = true;

  preFixup = ''
    makeWrapperArgs+=("''${qtWrapperArgs[@]}")
  '';

This should be sufficient to prevent double wrapping.

This comment has been minimized.

@jtojnar

jtojnar Dec 13, 2019
Contributor

Okay, the issue is that the GUI executable is in in share, the thing in bin is just a symlink. And Python infrastructure only sets up things in bin:


wrapPythonProgramsIn "$out/bin" "$out $pythonPath"

Adding

postFixup = ''
  wrapPythonProgramsIn "$out/share/dupeguru/run.py" "$out $pythonPath"
'';

should fix that.

This comment has been minimized.

@jtojnar

jtojnar Dec 13, 2019
Contributor

Hmm, there is a bug in the Python wrapper, this line

local -a user_args="($makeWrapperArgs)"

needs to be

                    local -a user_args="${makeWrapperArgs[@]}"

to work around it, the preFixup section should go like

    # TODO: A bug in python wrapper
    # see https://github.com/NixOS/nixpkgs/pull/75054#discussion_r357656916
    makeWrapperArgs="''${qtWrapperArgs[@]}"

This comment has been minimized.

@jtojnar

jtojnar Dec 13, 2019
Contributor

Sorry, should have been "$out/share/dupeguru" not "$out/share/dupeguru/run.py".

This comment has been minimized.

@lovesegfault

lovesegfault Dec 18, 2019
Contributor

Fixing this

This comment has been minimized.

@lovesegfault

lovesegfault Dec 18, 2019
Contributor

I take that back, testing this change on my laptop is impossible, @worldofpeace tag!

@novoxudonoser
Copy link
Member Author

@novoxudonoser novoxudonoser commented Dec 13, 2019

@jtojnar I have some problems after trying to make things like in your review.

I get error:

✦ ➜ /nix/store/5l9ckal7javjd04qa4n2afsmlal9naj5-dupeguru-4.0.4/bin/dupeguru
Traceback (most recent call last):
  File "/nix/store/5l9ckal7javjd04qa4n2afsmlal9naj5-dupeguru-4.0.4/bin/dupeguru", line 12, in <module>
    from PyQt5.QtCore import QCoreApplication, QSettings
ModuleNotFoundError: No module named 'PyQt5'

It looks like required packages are not in the python interpreter what wraps out/bin/dupguru . I had the same error on start but here I don't know what to do or how to analyze the problem. Can you give any advice about figuring out why it fails?

But as I know I made default.nix correct, all by instructions. Can you please review changes, maybe I am mistaken somewhere? The problem can be in dupguru itself its Makefile is a little "broken" .
(the pr files are updated somehow it is not displaying in the tread)

@doronbehar
Copy link
Contributor

@doronbehar doronbehar commented Dec 13, 2019

@novoxudonoser Try to find some python apps in the collection which use qt and wrap it and you should be able to get an idea as for how to fix your issue.

pkgs/applications/misc/dupeguru/default.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/dupeguru/default.nix Outdated Show resolved Hide resolved
#dontWrapQtApps = true;

#preFixup = ''
# makeWrapperArgs+=("''${gappsWrapperArgs[@]}")

This comment has been minimized.

@jtojnar

jtojnar Dec 13, 2019
Contributor

Okay, the issue is that the GUI executable is in in share, the thing in bin is just a symlink. And Python infrastructure only sets up things in bin:


wrapPythonProgramsIn "$out/bin" "$out $pythonPath"

Adding

postFixup = ''
  wrapPythonProgramsIn "$out/share/dupeguru/run.py" "$out $pythonPath"
'';

should fix that.
@novoxudonoser
Copy link
Member Author

@novoxudonoser novoxudonoser commented Dec 13, 2019

@jtojnar made all as you said. The problem is still there. Seems that we are missing something. The most interesting thin that in d409fe3 all worked, I guess that the problem is - that using buildPythonApplication for some bugs or wrong configuration same actions like in d409fe3 are not reproduced.

@jtojnar
Copy link
Contributor

@jtojnar jtojnar commented Dec 13, 2019

Sorry, should have been "$out/share/dupeguru" not "$out/share/dupeguru/run.py".

@novoxudonoser
Copy link
Member Author

@novoxudonoser novoxudonoser commented Dec 13, 2019

@novoxudonoser Try to find some python apps in the collection which use qt and wrap it and you should be able to get an idea as for how to fix your issue.

While the building of the first version of derivation I looked to many examples. And I picked stdenv.mkDerivation cause the make file in dupguru is a little "broken" (seems like symlink to python file breaks nixpkgs standart buildPythonApplication process ). Rith now I don't have enough knowledge to understand what breaks here. I think more deep research in nix build process and python section can help, and I plan to do it as part of my education process.

@novoxudonoser novoxudonoser requested a review from jtojnar Dec 13, 2019
@novoxudonoser
Copy link
Member Author

@novoxudonoser novoxudonoser commented Dec 13, 2019

@jtojnar finally it worked! Thanks a lot for your help, how I said upper - I don't have enough knowledge to make this derivation correct on my own.

@jtojnar
Copy link
Contributor

@jtojnar jtojnar commented Dec 13, 2019

@jtojnar finally it worked! Thanks a lot for your help, how I said upper - I don't have enough knowledge to make this derivation correct on my own.

Unfortunately, bash is such a terrible language that lot of these things are more complicated than they should be.


python3Packages.buildPythonApplication rec {
pname = "dupeguru";
format = "other";

This comment has been minimized.

@jtojnar

jtojnar Dec 13, 2019
Contributor

Sorry if I were unclear, I meant below version and above src.

  pname = "dupeguru";
  version = "4.0.4";

  format = "other";

  src = fetchFromGitHub {
@novoxudonoser novoxudonoser requested a review from jtojnar Dec 13, 2019
@jtojnar
Copy link
Contributor

@jtojnar jtojnar commented Dec 13, 2019

Looking at the makeFile:

It appears that the buildFlags are already part of the default make target (make all):
https://github.com/arsenetar/dupeguru/blob/2ea02bd7b50ce12ba489bc90717c4b578641deaf/Makefile#L47

It also suggests that we pass NO_VENV=1 to make:

https://github.com/arsenetar/dupeguru/blob/2ea02bd7b50ce12ba489bc90717c4b578641deaf/Makefile#L22-L24

I recommend dropping buildFlags and installFlags and just using:

  makeFlags = [
    "PREFIX=${placeholder ''out''}"
    "NO_VENV=1"
  ];
@novoxudonoser
Copy link
Member Author

@novoxudonoser novoxudonoser commented Dec 13, 2019

I recommend dropping buildFlags and installFlags and just using:

  makeFlags = [
    "PREFIX=${placeholder ''out''}"
    "NO_VENV=1"
  ];

Nice! I did not even think about this.

@novoxudonoser novoxudonoser requested a review from jtojnar Dec 13, 2019
"--assume-old=pyc"
];

doCheck = false;

This comment has been minimized.

@jtojnar

jtojnar Dec 13, 2019
Contributor

We could probably do

  checkInputs = with python3Packages; [
    pytest
    pytest-monkeyplus
  ];

  pytestFlagsArray = [ "core" "hscommon" ];

but that would require packaging pytest-monkeyplus.

If you do not feel like packaging it, please add a comment explaining that we are missing some test dependencies.

This comment has been minimized.

@novoxudonoser

novoxudonoser Dec 13, 2019
Author Member

I think that it will be a nice task for another pr. I don't feel myself ready to do it right now.

This comment has been minimized.

@jtojnar

jtojnar Dec 13, 2019
Contributor

Could you add it as a comment then?

Suggested change
doCheck = false;
# TODO: package pytest-monkeyplus for running tests
# https://github.com/NixOS/nixpkgs/pull/75054/files#r357690123
doCheck = false;

This comment has been minimized.

@jtojnar
Copy link
Contributor

@jtojnar jtojnar commented Dec 13, 2019

Also please split adding a maintainer into a separate commit a squash all dupeguru fixes.

@novoxudonoser novoxudonoser force-pushed the novoxudonoser:dupeguru branch from 652dcd7 to a4b5196 Dec 13, 2019
@novoxudonoser novoxudonoser requested a review from jtojnar Dec 13, 2019
@novoxudonoser novoxudonoser force-pushed the novoxudonoser:dupeguru branch from a4b5196 to c2cf629 Dec 13, 2019
Copy link
Contributor

@jtojnar jtojnar left a comment

Looks good to me, great job.

Copy link
Contributor

@jtojnar jtojnar left a comment

Looks good to me, great job.

@novoxudonoser
Copy link
Member Author

@novoxudonoser novoxudonoser commented Dec 13, 2019

@jtojnar - what next? Is there any need to mention somebody who can make an additional review and merge pr?

@jtojnar
Copy link
Contributor

@jtojnar jtojnar commented Dec 13, 2019

I set it to merge once CI succeeds.

@novoxudonoser
Copy link
Member Author

@novoxudonoser novoxudonoser commented Dec 13, 2019

@jtojnar thanks you a lot again, you made my day, +1 maintainer to nixpkgs.

@jtojnar jtojnar merged commit 968afa4 into NixOS:master Dec 13, 2019
13 of 15 checks passed
13 of 15 checks passed
dupeguru on aarch64-linux
Details
dupeguru on x86_64-linux
Details
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@novoxudonoser novoxudonoser deleted the novoxudonoser:dupeguru branch Dec 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.