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

Add nix derivation #45

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

Informatic
Copy link
Contributor

Convenience for NixOS / Nixpkgs users, nix-build ran in main project
directory should build this and create a result/ symlink pointing at a
built package.

Convenience for NixOS / Nixpkgs users, `nix-build` ran in main project
directory should build this and create a `result/` symlink pointing at a
built package.
@smx-smx
Copy link
Member

smx-smx commented May 22, 2021

Thanks, but keep in mind that I don't have a CI/Environment to test NixOS.
I don't know if I can ensure it doesn't break in the future

Copy link

@aszlig aszlig left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution. Apart from my other comments: What's the reason for using a default.nix instead of making it a Flake?

Note that I'm not involved with this project, just stumbled on the project when trying to reverse-engineer some firmware and saw your pull request.

src = ./.;

buildInputs = [
pkgs.cmake
Copy link

Choose a reason for hiding this comment

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

This needs to be in nativeBuildInputs because otherwise you'll get cmake for the target platform when cross-compiling.


buildInputs = [
pkgs.cmake
pkgs.openssl.dev
Copy link

Choose a reason for hiding this comment

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

No need to specify .dev since this output is already used by default for buildInputs.

Comment on lines +15 to +22

configurePhase = ''
cmake .
'';

buildPhase = ''
make
'';
Copy link

Choose a reason for hiding this comment

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

Suggested change
configurePhase = ''
cmake .
'';
buildPhase = ''
make
'';

If you place cmake in nativeBuildInputs, you get the right setup hooks and won't need this.

cd src
cp epk2extract tools/lzhsenc tools/lzhs_scanner tools/idb_extract tools/jffs2extract $out/bin

chmod -x ../keys/*
Copy link

Choose a reason for hiding this comment

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

It's the upstream project, so no need to fix up something like this.

cp epk2extract tools/lzhsenc tools/lzhs_scanner tools/idb_extract tools/jffs2extract $out/bin

chmod -x ../keys/*
cp ../keys/*.key ../keys/*.pem $out/bin
Copy link

Choose a reason for hiding this comment

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

This is pretty ugly IMO and those files ideally should be in $out/share/epk2extract or something like that but certainly not in $out/bin. Probably it's a good idea to make this configurable (see src/main.c:223) via cmake.

Comment on lines +25 to +27
mkdir -p $out/bin
cd src
cp epk2extract tools/lzhsenc tools/lzhs_scanner tools/idb_extract tools/jffs2extract $out/bin
Copy link

Choose a reason for hiding this comment

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

I know it's probably personal taste, but what about the following (which does not change the current working directory):

Suggested change
mkdir -p $out/bin
cd src
cp epk2extract tools/lzhsenc tools/lzhs_scanner tools/idb_extract tools/jffs2extract $out/bin
for i in src/epk2extract src/tools/{lzhsenc,lzhs_scanner,idb_extract,jffs2extract}; do
install -vD "$i" "$out/bin/$(basename "$i")"
done

Comment on lines +4 to +5
pname = "epk2extract";
version = "devel";
Copy link

Choose a reason for hiding this comment

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

Since we don't have an explicit version here, it's probably better to just use name instead of pname and version:

Suggested change
pname = "epk2extract";
version = "devel";
name = "epk2extract";

@@ -31,3 +31,5 @@ src/tools/idb_extract
src/tools/jffs2extract
src/tools/lzhs_scanner
src/tools/lzhsenc

result
Copy link

Choose a reason for hiding this comment

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

Just a small nitpick:

Suggested change
result
/result

@Informatic
Copy link
Contributor Author

Informatic commented Jun 25, 2021

Thanks for your contribution. Apart from my other comments: What's the reason for using a default.nix instead of making it a Flake?

Just a very simple reason - I just haven't started using Flakes yet :D

Thank you very much for a review. As you may have guessed, I am not very well versed in practical nix packaging. I'll integrate the changes this weekend.

@smx-smx smx-smx force-pushed the master branch 3 times, most recently from eef7a24 to e42ad47 Compare July 11, 2023 21:20
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants