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

creddump: init at 0.3 #78483

Merged
merged 1 commit into from Aug 23, 2020
Merged

creddump: init at 0.3 #78483

merged 1 commit into from Aug 23, 2020

Conversation

fishi0x01
Copy link
Member

@fishi0x01 fishi0x01 commented Jan 25, 2020 •
edited

Motivation for this change

creddump is a collection of python scripts for retrieving hashes from windows hive files.

Things done

I am wrapping the scripts with python27Packages.buildPythonApplication. Build and run successful locally on Ubuntu18.04. Installed via:

nix-env -f $NIXPKGS -iA creddump
  • 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.

@fishi0x01 fishi0x01 changed the title init: creddump at 0.3 creddump: init at 0.3 Jan 27, 2020
@fishi0x01 fishi0x01 force-pushed the fishi0x01/creddump branch 3 times, most recently from d2519d8 to 6e02840 Compare February 2, 2020 13:50
@doronbehar
Copy link
Contributor

Please use the PR format as is.

doronbehar
doronbehar reviewed Feb 23, 2020
View reviewed changes
pkgs/tools/security/creddump/default.nix Outdated Show resolved Hide resolved
pkgs/tools/security/creddump/default.nix Show resolved Hide resolved
pkgs/tools/security/creddump/default.nix Show resolved Hide resolved
pkgs/tools/security/creddump/default.nix Outdated Show resolved Hide resolved
pkgs/tools/security/creddump/default.nix Outdated Show resolved Hide resolved
@doronbehar
Copy link
Contributor

Things done

I am wrapping the scripts with python27Packages.buildPythonApplication. Build and run successful locally on Ubuntu18.04. Installed via:

nix-env -f $NIXPKGS -iA creddump

@fishi0x01 Please update your first comment to the PR template: https://github.com/NixOS/nixpkgs/blob/master/.github/PULL_REQUEST_TEMPLATE.md

@fishi0x01 fishi0x01 force-pushed the fishi0x01/creddump branch 5 times, most recently from 95b54e6 to 5bf745c Compare March 8, 2020 00:21
doronbehar
doronbehar reviewed Mar 8, 2020
View reviewed changes
pkgs/tools/security/creddump/default.nix Show resolved Hide resolved
pkgs/tools/security/creddump/default.nix Show resolved Hide resolved
pkgs/tools/security/creddump/default.nix Outdated Show resolved Hide resolved
pkgs/tools/security/creddump/default.nix Outdated
buildInput = [ python2 ];

installPhase = ''
mkdir -p $out/bin
Copy link
Contributor

Choose a reason for hiding this comment

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

Change this here and in the other commands.

Suggested change
mkdir -p $out/bin
mkdir -p ${placeholder "out"}/bin

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok adjusted 👍
Out of curiosity: why is this better?

Copy link
Contributor

Choose a reason for hiding this comment

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

TBH I'm not sure how to explain this I just had that comment in the past. I think the idea is that you get the actual content of what's in $out inside these shell hooks so it's 1 less thing the shell evaluates and 1 more thing Nix does..

Copy link
Member Author

Choose a reason for hiding this comment

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

I just found a thread which claims to avoid placeholder if possible, as it is designed to be used for edge cases in which you need to reference $out outside of classic places like buildPhase or installPhase. However, I cannot really find a good reason for that claim inside the referenced documentation :(

Anyways I also do not really see a downside using placeholder, so I do not mind applying it here 👍

doronbehar
doronbehar requested changes Mar 8, 2020
View reviewed changes
pkgs/tools/security/creddump/default.nix Outdated Show resolved Hide resolved
pkgs/tools/security/creddump/default.nix Outdated Show resolved Hide resolved
doronbehar
doronbehar approved these changes Mar 8, 2020
View reviewed changes
Copy link
Contributor

@doronbehar doronbehar left a comment

Choose a reason for hiding this comment

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

Awesome :) Now your PR is perfect (in my eyes). Thank you for your contribution.

@fishi0x01
Copy link
Member Author

@doronbehar Thank you for the detailed review! I learned a lot from this 🙇‍♂️

@fishi0x01 fishi0x01 mentioned this pull request Mar 20, 2020
Merged
10 tasks
@infinisil infinisil merged commit ba1088c into NixOS:master Aug 23, 2020
@fishi0x01 fishi0x01 deleted the fishi0x01/creddump branch September 26, 2020 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@doronbehar doronbehar doronbehar approved these changes

Assignees
No one assigned
Labels
8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@fishi0x01 @doronbehar @infinisil