Skip to content

Navigation Menu

Sign in
Appearance settings
Sign up
Appearance settings

Add nix derivation - #45

#45
Open
Informatic wants to merge 1 commit into
openlgtv:masteropenlgtv/epk2extract:masterfrom
Informatic:nix-packageInformatic/epk2extract:nix-packageCopy head branch name to clipboard
Open

Add nix derivation#45
Informatic wants to merge 1 commit into
openlgtv:masteropenlgtv/epk2extract:masterfrom
Informatic:nix-packageInformatic/epk2extract:nix-packageCopy head branch name to clipboard

Conversation

@Informatic

Copy link
Copy Markdown
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

smx-smx commented May 22, 2021

Copy link
Copy Markdown
Member

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

@aszlig aszlig left a comment

Copy link
Copy Markdown

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.

Comment thread default.nix
src = ./.;

buildInputs = [
pkgs.cmake

Copy link
Copy Markdown

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.

Comment thread default.nix

buildInputs = [
pkgs.cmake
pkgs.openssl.dev

Copy link
Copy Markdown

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 thread default.nix
Comment on lines +15 to +22

configurePhase = ''
cmake .
'';

buildPhase = ''
make
'';

Copy link
Copy Markdown

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.

Comment thread default.nix
cd src
cp epk2extract tools/lzhsenc tools/lzhs_scanner tools/idb_extract tools/jffs2extract $out/bin

chmod -x ../keys/*

Copy link
Copy Markdown

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.

Comment thread default.nix
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
Copy Markdown

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 thread default.nix
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
Copy Markdown

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 thread default.nix
Comment on lines +4 to +5
pname = "epk2extract";
version = "devel";

Copy link
Copy Markdown

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";

Comment thread .gitignore
src/tools/lzhs_scanner
src/tools/lzhsenc

result

Copy link
Copy Markdown

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

Informatic commented Jun 25, 2021

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown

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

Morty Proxy This is a proxified and sanitized view of the page, visit original site.