-
-
Notifications
You must be signed in to change notification settings - Fork 13.1k
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
ophcrack-gui: init at 3.8.0 #290496
ophcrack-gui: init at 3.8.0 #290496
Conversation
|
||
postInstall = '' | ||
wrapProgram $out/bin/ophcrack \ | ||
--prefix PATH : "${libsForQt5.qtbase}/bin" |
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.
Is this really needed? Have you tried adding qtbase to buildInputs
?
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.
Yes i did try having qtbase in buildInputs without postInstall it results to,
"qt.qpa.plugin: Could not find the Qt platform plugin "xcb" in ""
This application failed to start because no Qt platform plugin could be initialized. Reinstalling the application may fix this problem.
Aborted (core dumped)"
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 this postInstall not needed anymore
NOTE: 3.8.0 is 6 years ago |
Does that mean it won't get merged? |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review/3032/4076 |
that means that it's likely unmaintained, and it'll likely gonna break now or sooner, and that'll raise the question: do we really wanna include it in our official repo regarding that we know that it will break and we'll put ourselves in "okay, remove it and break some users workflows, or do some temporary workarounds that we know that it'll stop working at some point" situation |
OK. |
should this be merged with "ophcrack-cli" ? I mean you can still have two top level attributes, but merge common bits ? or you can have one derivation and make GUI as parameter ? |
Im not quite sure, the gui produces same result when argument/options are passed same as ophcrack-cli but when the program ophcrack is executed, the GUI can only be used. i only adopted same spec like Kali for building the package. Here in Kali they have ophcrack-cli and ophcrack. |
you can still have two attributes, my comment is about code duplication between the two |
What do you think? the Ophcrack gui is also needed by another package. |
I am not sure I understand the question. My comment is about code duplication between GUI and CLI derivations. you can still have two seperate derivations, just reuse some of common attributes, eg |
ba8e0d1
to
4476760
Compare
pname = "ophcrack"; | ||
version = "3.8.0"; | ||
|
||
src = fetchurl { |
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.
this is duplicated with ophcrack-cli
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.
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.
yes, that a great example.
you can add enableGui
parameter to this derivation ( have it disabled by default )
then remove/rename ophcrack-cli to ophcrack-gui, and all it will do ophcrack.override { enableGuui = true; }
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 this recent expression replace ophcrack-cli?
162b958
to
da3a01b
Compare
cherry-pick done. @kirillrdy |
this PR seems to be missing changes to |
Changes to ophcrack-cli added @kirillrdy |
@Tochiaha can you rebase to resolve merge conflict ? |
merge conflict resolved. Do we push update with the ophcrack-cli? @kirillrdy |
@Tochiaha yes, we should include changes to ophcrack-cli in this PR |
Description of changes
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.