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

zsteg: init at 0.2.2 #107190

Merged
merged 2 commits into from Apr 6, 2021
Merged

zsteg: init at 0.2.2 #107190

merged 2 commits into from Apr 6, 2021

Conversation

applePrincess
Copy link
Contributor

Motivation for this change

Add zsteg package.

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.

@buckley310
Copy link
Contributor

buckley310 commented Dec 21, 2020 •
edited

This builds and ran fine for me on NixOS against a steg file I had lying around.
Probably should squash the changes down to just the two commits though (maintainers list and then package itself), and change the package's commit message to match the PR title.

@buckley310
Copy link
Contributor

All looks good after that push.

@buckley310
Copy link
Contributor

all-packages is self-described as "roughly sorted by alphabet", although zsteg seems at home next to steghide, it might make sense to move it. Come to think of it, I'm not sure why steghide ended up in the DEVELOPMENT / LIBRARIES section anyway, I think its just a cli utility.

@applePrincess
Copy link
Contributor Author

In short, I'm confused.

In all-packages.nix,

/* The top-level package collection of nixpkgs.
 * It is sorted by categories corresponding to the folder names
 * in the /pkgs folder. Inside the categories packages are roughly
 * sorted by alphabet, but strict sorting has been long lost due
 * to merges. Please use the full-text search of your editor. ;)

To my understanding, sort by category (approx. folder) first, then sort by alphabet.
But the actual fact is that, sorted by either proprely (?? to my understanding), alphabet only or relevalence.
I think, one day, we need a branch to solve this.

@buckley310
Copy link
Contributor

If it were me, I would insert it alphabetically into the APPLICATIONS section.

@applePrincess
Copy link
Contributor Author

Thank you for guiding me.

@applePrincess
Copy link
Contributor Author

Is this PR couldn't get merged because of checks failure?
The file failing is automatically generated by bundix, so this can be either a bug(??) in bundix or add exception to editorconfig.

SuperSandro2000
SuperSandro2000 requested changes Mar 22, 2021
View reviewed changes
maintainers/maintainer-list.nix
@@ -686,6 +686,16 @@
githubId = 1078530;
name = "Alexandre Peyroux";
};
applePrincess = {
Copy link
Member

Choose a reason for hiding this comment

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

Please put this into a separate commit with the message maintainers: add applePrincess.

pkgs/tools/security/zsteg/Gemfile Outdated
@@ -0,0 +1,3 @@
source 'https://rubygems.org'
gem 'zsteg'

Copy link
Member

Choose a reason for hiding this comment

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

One new line to much.

pkgs/tools/security/zsteg/gemset.nix Outdated
};
version = "0.2.2";
};
}
Copy link
Member

Choose a reason for hiding this comment

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

Missing final new line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file is what I mentioned above. This is generad by running $(nix-build '<nixpkgs>' -A bundix --no-out-link)/bin/bundix --magic which is stated by Ruby section of Nixpkgs manual. To me, changing machine generated files is a hack.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK now I see trailing newline issue on nix-community/bundix repo, so will soon be fixed, for this time I added manually.

pkgs/top-level/all-packages.nix Outdated
@@ -26363,6 +26363,8 @@ in

zscroll = callPackage ../applications/misc/zscroll {};

zsteg = callPackage ../tools/security/zsteg {};
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
zsteg = callPackage ../tools/security/zsteg {};
zsteg = callPackage ../tools/security/zsteg { };

pkgs/top-level/all-packages.nix Outdated
@@ -26377,6 +26379,7 @@ in
guiModule = "zest";
};


Copy link
Member

Choose a reason for hiding this comment

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

Please revert that.

@SuperSandro2000
Copy link
Member

@ofborg eval

@SuperSandro2000 SuperSandro2000 merged commit 6f45d62 into NixOS:master Apr 6, 2021
@applePrincess applePrincess deleted the add-zsteg branch June 21, 2021 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SuperSandro2000 SuperSandro2000 SuperSandro2000 requested 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
@applePrincess @buckley310 @SuperSandro2000