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
dupeguru: init at 4.0.4 #75054
Conversation
559e18c
to
4a3cd3f
Compare
Just found that I need to rewrite source fetching, from fetchGit to fetchFromGitHub with submodules. (fixed) |
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. |
Yes.
|
@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) |
It's OK, welcome aboard the Caos 😺. You can mention maybe |
@doronbehar thanks for your review and help. |
#dontWrapQtApps = true; | ||
|
||
#preFixup = '' | ||
# makeWrapperArgs+=("''${gappsWrapperArgs[@]}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dontWrapQtApps = true;
preFixup = ''
makeWrapperArgs+=("''${qtWrapperArgs[@]}")
'';
This should be sufficient to prevent double wrapping.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
:
wrapPythonPrograms |
wrapPythonProgramsIn "$out/bin" "$out $pythonPath" |
Adding
postFixup = ''
wrapPythonProgramsIn "$out/share/dupeguru/run.py" "$out $pythonPath"
'';
should fix that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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[@]}"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, should have been "$out/share/dupeguru"
not "$out/share/dupeguru/run.py"
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixing this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I take that back, testing this change on my laptop is impossible, @worldofpeace tag!
@jtojnar I have some problems after trying to make things like in your review. I get error:
It looks like required packages are not in the python interpreter what wraps 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" . |
@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. |
#dontWrapQtApps = true; | ||
|
||
#preFixup = '' | ||
# makeWrapperArgs+=("''${gappsWrapperArgs[@]}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
:
wrapPythonPrograms |
wrapPythonProgramsIn "$out/bin" "$out $pythonPath" |
Adding
postFixup = ''
wrapPythonProgramsIn "$out/share/dupeguru/run.py" "$out $pythonPath"
'';
should fix that.
Sorry, should have been |
While the building of the first version of derivation I looked to many examples. And I picked |
@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"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry if I were unclear, I meant below version
and above src
.
pname = "dupeguru";
version = "4.0.4";
format = "other";
src = fetchFromGitHub {
Looking at the It appears that the It also suggests that we pass https://github.com/arsenetar/dupeguru/blob/2ea02bd7b50ce12ba489bc90717c4b578641deaf/Makefile#L22-L24 I recommend dropping makeFlags = [
"PREFIX=${placeholder ''out''}"
"NO_VENV=1"
]; |
Nice! I did not even think about this. |
"--assume-old=pyc" | ||
]; | ||
|
||
doCheck = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that it will be a nice task for another pr. I don't feel myself ready to do it right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add it as a comment then?
doCheck = false; | |
# TODO: package pytest-monkeyplus for running tests | |
# https://github.com/NixOS/nixpkgs/pull/75054/files#r357690123 | |
doCheck = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jtojnar fixed.
Also please split adding a maintainer into a separate commit a squash all dupeguru fixes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, great job.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, great job.
@jtojnar - what next? Is there any need to mention somebody who can make an additional review and merge pr? |
I set it to merge once CI succeeds. |
@jtojnar thanks you a lot again, you made my day, +1 maintainer to nixpkgs. |
Motivation for this change
Add dupeGuru programm https://dupeguru.voltaicideas.net .
Things done
sandbox
innix.conf
on non-NixOS)nix-shell -p nix-review --run "nix-review wip"
./result/bin/
)nix path-info -S
before and after)