blkhash: init at 0.7.1 #215551

noisersup · 2023-02-09T18:03:45Z

Description of changes

Things done

nirs

Looks good to me, but I don't know anything about Nix :-)

pkgs/development/libraries/blkhash/default.nix

nirs · 2023-02-09T19:25:20Z

pkgs/development/libraries/blkhash/default.nix

+  postUnpack = ''(
+    cd "$sourceRoot/subprojects"
+    cp -R --no-preserve=mode,ownership ${unity} unity
+  )'';


If we fetch the source, the normal build flow documented in the project README.md
already handles the subprojects.

Unfortunately, that's not a case here, as nix builds packages in offline sandbox to improve reproducibility.
Fetching subprojects manually gives control over the version of dependencies, so nix is able to build the same package with the same checksum independently from the changes on Unity repo.
I've tried to check how other packages using meson resolved that issue, and didn't find any better way to do that, but if anyone has better solution please let me know.

Adding fetchSubmodules = true to fetchFromGitLab will do the trick for you.

pkgs/development/libraries/blkhash/default.nix

nirs · 2023-02-09T21:41:11Z

I tested this pr on with single user Nix install:

$ nixpkgs-review pr https://github.com/NixOS/nixpkgs/pull/215551
$ git -c fetch.prune=false fetch --no-tags --force https://github.com/NixOS/nixpkgs master:refs/nixpkgs-review/0 pull/215551/head:refs/nixpkgs-review/1
remote: Enumerating objects: 29, done.
remote: Counting objects: 100% (26/26), done.
remote: Compressing objects: 100% (18/18), done.
remote: Total 29 (delta 8), reused 15 (delta 7), pack-reused 3
Unpacking objects: 100% (29/29), 334.75 KiB | 856.00 KiB/s, done.
From https://github.com/NixOS/nixpkgs
 * [new branch]              master                -> refs/nixpkgs-review/0
 * [new ref]                 refs/pull/215551/head -> refs/nixpkgs-review/1
$ git worktree add /home/nsoffer/.cache/nixpkgs-review/pr-215551/nixpkgs 9f6c8925bc19ab67292da521948e5840de807ffd
Preparing worktree (detached HEAD 9f6c8925bc1)
Updating files: 100% (33698/33698), done.
HEAD is now at 9f6c8925bc1 Merge pull request #215548 from dotlambda/keepass-2.53.1
$ git merge --no-commit --no-ff 4a5e1f4389bfc322c0b55dcf48a1ed9205011987
Auto-merging pkgs/top-level/all-packages.nix
Automatic merge went well; stopped before committing as requested
$ nix --experimental-features nix-command build --no-link --keep-going --no-allow-import-from-derivation --option build-use-sandbox relaxed -f /home/nsoffer/.cache/nixpkgs-review/pr-215551/build.nix

Link to currently reviewing PR:
https://github.com/NixOS/nixpkgs/pull/215551

1 package built:
blkhash

$ which blksum
/nix/store/yglgri66zfpyfsvgbgspij7kzzczzn9n-blkhash-0.6.1/bin/blksum

$ tree /nix/store/yglgri66zfpyfsvgbgspij7kzzczzn9n-blkhash-0.6.1
/nix/store/yglgri66zfpyfsvgbgspij7kzzczzn9n-blkhash-0.6.1
├── bin
│   └── blksum
├── include
│   └── blkhash.h
├── lib
│   ├── libblkhash.so -> libblkhash.so.0
│   ├── libblkhash.so.0 -> libblkhash.so.0.6.1
│   ├── libblkhash.so.0.6.1
│   └── pkgconfig
│       └── blkhash.pc
└── share
    └── man
        ├── man1
        │   └── blksum.1.gz
        └── man3
            ├── blkhash_final.3.gz
            ├── blkhash_free.3.gz
            ├── blkhash_new.3.gz
            ├── blkhash_update.3.gz
            └── blkhash_zero.3.gz

$ blksum /data/tmp/demo/fedora-35.raw 
6e5c00c995056319d52395f8d91c7f84725ae3da69ffcba4de4c7d22cff713a5  /data/tmp/demo/fedora-35.raw

$ blksum /data/tmp/demo/fedora-35.qcow2 
6e5c00c995056319d52395f8d91c7f84725ae3da69ffcba4de4c7d22cff713a5  /data/tmp/demo/fedora-35.qcow2

$ time blksum /data/tmp/demo/empty-8t.raw 
4750314ad6d0ab2e7f6508706e611434fa5f41859e5ff11434e2e37141dbced5  /data/tmp/demo/empty-8t.raw

real	0m2.526s
user	0m9.739s
sys	0m0.086s

blksum looks funcional.

Looks like all other files look right, but I don't know enough about Nix to detect issues.

Issues:

blksum depends on qemu-nbd, provided by qemu-img on Fedora/CentOS. On Nix it seems that you have to install the full qemu package to get qemu-nbd. My test worked since qemu-img was already installed from the Fedora package. So I think this package need to require the qemu package on Nix.

Otherwise it is awesome to see this package here, thanks @noisersup!

nirs

Currnet version works for me. See inline comments for possible improments.

pkgs/development/libraries/blkhash/default.nix

nirs · 2023-02-11T18:38:17Z

pkgs/development/libraries/blkhash/default.nix

+    asciidoc
+  ] ++ lib.optional (enableLibnbd) libnbd;
+
+  mesonFlags = lib.optional (!enableLibnbd) ["-Dnbd=disabled"];


meson should work automatically without specifying anything. The default value is "auto":

option('nbd', type: 'feature', value: 'auto', description: 'Support NBD URL')

So I think it is good enough to require libnbd only when it is available.

I had some issues when I was trying to build derivation without libnbd. When I was compiling manually it worked correctly, but with nix - it just returns the error. Maybe it's because some default meson flags that nix uses.

EDIT: It seems it is because of -Dauto_features=enabled which is set by default, changed it to auto.

nirs · 2023-02-11T18:41:18Z

pkgs/development/libraries/blkhash/default.nix

+, pkg-config
+, cmake
+, openssl
+, enableLibnbd ? stdenv.isLinux, libnbd


isLinux is likely correct now, but I think a better way woul be to add the
dependency dynanically if a libnbd nix package is available for the target.
But I think it is good enough for now.

pkgs/development/libraries/blkhash/default.nix

nirs · 2023-02-11T18:43:35Z

pkgs/development/libraries/blkhash/default.nix

+    cmake
+    openssl
+    qemu-utils
+    asciidoc


Sorting this list would be nice.

I was struggling with deciding what order should be used here. Many derivations in nixpkgs don't use lexicographical order. I would suggest to keep buildInputs sorted, but for attribute set on top of nix file I suggest to place all things related to nix tooling (like lib or fetchFromGitLab) on top of other arguments.

I think sorting helps with simple lists. The order of top level things
should depend on the semantics. Your suggestion sounds good.

nirs · 2023-02-11T18:43:44Z

pkgs/development/libraries/blkhash/default.nix

+    meson
+    ninja
+    pkg-config
+    cmake


cmake not needed.

pkgs/development/libraries/blkhash/default.nix

noisersup · 2023-02-11T23:59:03Z

Thank you for your detailed look on this PR! I've applied following changes:

allowed meson to skip libnbd (now it's skipped "natively")
separated buildInputs and nativeBuildInputs correctly
Sorted lists and inputs
removed cmake as it is unused

nirs

Looks good to me.

I opened an issue in blkhash for the build failure on darwin:
https://gitlab.com/nirs/blkhash/-/issues/75

I think we can limit the build to Linux for now, and add darwin support
when blkhash is fixed.

Also tested on Fedora 37 with single user Nix setup.

nirs · 2023-02-12T21:37:58Z

@noisersup the macOS issue is fixed by:
https://gitlab.com/nirs/blkhash/-/merge_requests/86

It can be useful to test this change with commit 389fbcf1a4906cb4987feb8b4f524f1cb947309c
to verify that it also builds via Nix before I merge this MR.

noisersup · 2023-02-13T20:49:40Z

@nirs Thank you for quickly fixing this issue, it looks like it works now! Would you rather merge nix derivation at 0.6.1 and mark darwin as broken until new version or wait for your fix to be merged?

nirs · 2023-02-14T11:25:23Z

@noisersup Great! the fix is merged as fd611881862d375b83f216cf1c59574b3c8ccf70.

I plan to release blkhash 0.7.1 this weekend. You can build this package based on the
commit hash if you want to merge it earilier.

noisersup · 2023-02-14T18:58:09Z

I believe that there's not much cases in nixpkgs where we track the commit in place of version/tag. ncmpcpp for example, is still on version from 2021, just because the upstream does not tag new changes (latest commit on master branch from Dec 31 2022).

I would suggest to init blkhash at 0.6.1 without darwin support and if it's going to be merged, just add darwin support after weekend.

If it's not merged yet, and I highly believe it won't :) then we will modify this PR to point at newer version.
What do you think?

nirs · 2023-02-15T16:03:45Z

I would suggest to init blkhash at 0.6.1 without darwin support and if it's going to be merged, just add darwin support after weekend.

I would do the same.

nirs · 2023-02-19T19:09:39Z

@noisersup blkhash 0.7.1 is available now, you can update the version.

noisersup · 2023-02-19T20:27:56Z

Hi, sorry for a little delay I'm on vacation right now :) I've bumped the version to 0.7.1, so CI should pass. Thank you for release!

nirs · 2023-02-19T22:11:56Z

@noisersup: One issue - current build installs unity headers and pkgconfig files:

Installing subprojects/unity/libunity.a to /nix/store/8sr9agjz7zdkp555gjds580zblmf3588-blkhash-0.7.1/lib
Installing /private/tmp/nix-build-blkhash-0.7.1.drv-0/source/subprojects/unity/src/unity.h to /nix/store/8sr9agjz7zdkp555gjds580zblmf3588-blkhash-0.7.1/include/unity/
Installing /private/tmp/nix-build-blkhash-0.7.1.drv-0/source/subprojects/unity/src/unity_internals.h to /nix/store/8sr9agjz7zdkp555gjds580zblmf3588-blkhash-0.7.1/include/unity/
Installing /private/tmp/nix-build-blkhash-0.7.1.drv-0/source/build/meson-private/unity.pc to /nix/store/8sr9agjz7zdkp555gjds580zblmf3588-blkhash-0.7.1/lib/pkgconfig

This is wrong, since unity is a test dependency and it should not be installed
on the target.

This is an issue with latest meson, fixed upstream like this:
https://gitlab.com/nirs/blkhash/-/commit/ff1046664a0617e1f23705123f66b701a4da2c5f

If there is a way to set arguments for the meson install command, you need to add
--skip-subprojects to avoid this.

nirs

Need to avoid installing unity headers and libraries.

nirs · 2023-02-21T01:00:13Z

@noisersup I found why unity is installed:
ThrowTheSwitch/Unity#661

I posted a fix to unity:
ThrowTheSwitch/Unity#662

Once this is fixed in unity, there will be no need to add --skip-subprojects.

Or, you can add this tiny patch to build with unity v2.5.2, which does not have
this issue:
https://gitlab.com/nirs/blkhash/-/merge_requests/89/diffs?commit_id=7687bbe227ea9dccd8d1fbd47d4f4477bd160c06

nirs

The unity fix was merged, so current code should be fine.

noisersup · 2023-02-23T17:54:11Z

Thank you for handling this problem! I've updated Unity repo revision to the latest and it seems that it doesn't import additional headers.

nirs · 2023-03-04T19:07:39Z

@noisersup anything blocking this PR?

noisersup · 2023-03-04T20:04:04Z

@noisersup anything blocking this PR?

The lack of reviewers. I'll ask in community.

nirs · 2023-03-31T18:06:29Z

@noisersup new release landed, it would be nice to update this package.

I don't think anything related to packaging was changed, so you just need to update
the version and the hash.

See https://gitlab.com/nirs/blkhash/-/releases/v0.8.1

nirs · 2023-09-09T14:51:46Z

@noisersup blkhash 0.9.1 released:
https://gitlab.com/nirs/blkhash/-/releases/v0.9.1

Aleksanaa

Also the package should go to pkgs/by-name, without adding top-level reference.

Aleksanaa · 2024-04-13T10:33:55Z

pkgs/development/libraries/blkhash/default.nix

+    makeWrapper
+    meson
+    ninja
+    openssl


openssl should be in buildInputs in most cases.

Aleksanaa · 2024-04-13T10:34:40Z

pkgs/development/libraries/blkhash/default.nix

+    cp -R --no-preserve=mode,ownership ${unity} unity
+  )'';
+
+  buildInputs = [ ] ++ lib.optional (enableLibnbd) libnbd;


Suggested change

buildInputs = [ ] ++ lib.optional (enableLibnbd) libnbd;

buildInputs = lib.optionals enableLibnbd [ libnbd ];

Aleksanaa · 2024-04-13T10:38:36Z

pkgs/development/libraries/blkhash/default.nix

+    homepage = "https://gitlab.com/nirs/blkhash";
+    license = licenses.lgpl21Plus;
+    maintainers = with maintainers; [ noisersup ];
+    platforms = platforms.unix;


Suggested change

platforms = platforms.unix;

mainProgram = "blksum";

platforms = platforms.unix;

Aleksanaa · 2024-04-13T10:41:44Z

pkgs/development/libraries/blkhash/default.nix

+  postUnpack = ''(
+    cd "$sourceRoot/subprojects"
+    cp -R --no-preserve=mode,ownership ${unity} unity
+  )'';


Adding fetchSubmodules = true to fetchFromGitLab will do the trick for you.

Aleksanaa · 2024-04-13T10:42:38Z

pkgs/development/libraries/blkhash/default.nix

+
+  src = fetchFromGitLab {
+    owner = "nirs";
+    repo = pname;


Suggested change

repo = pname;

repo = "blkhash";

Aleksanaa · 2024-04-13T10:44:47Z

pkgs/development/libraries/blkhash/default.nix

+  ];
+
+  # don't enforce libnbd
+  mesonAutoFeatures = "auto";


This can hide errors and make the product not as expected. For example, libnbd may not actually be found when building for Linux.

Aleksanaa · 2024-04-13T10:44:59Z

pkgs/development/libraries/blkhash/default.nix

+  # don't enforce libnbd
+  mesonAutoFeatures = "auto";
+
+  postInstall = ''


Suggested change

postInstall = ''

postFixup = ''

ofborg bot added 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Feb 9, 2023

nirs reviewed Feb 9, 2023

View reviewed changes

pkgs/development/libraries/blkhash/default.nix Show resolved Hide resolved

noisersup force-pushed the blkhash-init branch 2 times, most recently from 705576f to 3d2eda5 Compare February 11, 2023 13:59

ofborg bot added 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin labels Feb 11, 2023

nirs reviewed Feb 11, 2023

View reviewed changes

noisersup force-pushed the blkhash-init branch from 3d2eda5 to 37421d8 Compare February 11, 2023 23:53

noisersup requested a review from nirs February 12, 2023 00:00

noisersup force-pushed the blkhash-init branch from 37421d8 to fe14afc Compare February 12, 2023 05:48

nirs reviewed Feb 12, 2023

View reviewed changes

noisersup force-pushed the blkhash-init branch from fe14afc to c49e851 Compare February 13, 2023 20:08

noisersup force-pushed the blkhash-init branch from c49e851 to 027bd01 Compare February 19, 2023 20:22

noisersup changed the title ~~blkhash: init at 0.6.1~~ blkhash: init at 0.7.1 Feb 19, 2023

nirs approved these changes Feb 19, 2023

View reviewed changes

noisersup added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Feb 19, 2023

nirs suggested changes Feb 19, 2023

View reviewed changes

nirs approved these changes Feb 21, 2023

View reviewed changes

blkhash: init at 0.7.1

7944544

noisersup force-pushed the blkhash-init branch from 027bd01 to 7944544 Compare February 23, 2023 17:54

figsoda removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Mar 5, 2023

wegank added 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 2.status: merge conflict This PR has merge conflicts with the target branch labels Mar 19, 2024

stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 20, 2024

Aleksanaa reviewed Apr 13, 2024

View reviewed changes

Aleksanaa added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label May 24, 2024

	buildInputs = [ ] ++ lib.optional (enableLibnbd) libnbd;
	buildInputs = lib.optionals enableLibnbd [ libnbd ];

	platforms = platforms.unix;
	mainProgram = "blksum";
	platforms = platforms.unix;

blkhash: init at 0.7.1 #215551

Are you sure you want to change the base?

blkhash: init at 0.7.1 #215551

Conversation

noisersup commented Feb 9, 2023

Description of changes

Things done

nirs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noisersup Feb 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nirs commented Feb 9, 2023

nirs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noisersup Feb 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noisersup commented Feb 11, 2023 • edited Loading

nirs left a comment • edited Loading

Choose a reason for hiding this comment

nirs commented Feb 12, 2023

noisersup commented Feb 13, 2023 • edited Loading

nirs commented Feb 14, 2023

noisersup commented Feb 14, 2023 • edited Loading

nirs commented Feb 15, 2023

nirs commented Feb 19, 2023

noisersup commented Feb 19, 2023

nirs commented Feb 19, 2023 • edited Loading

nirs left a comment

Choose a reason for hiding this comment

nirs commented Feb 21, 2023 • edited Loading

nirs left a comment

Choose a reason for hiding this comment

noisersup commented Feb 23, 2023

nirs commented Mar 4, 2023

noisersup commented Mar 4, 2023

nirs commented Mar 31, 2023

nirs commented Sep 9, 2023

Aleksanaa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noisersup Feb 10, 2023 •

edited

Loading

noisersup Feb 11, 2023 •

edited

Loading

noisersup commented Feb 11, 2023 •

edited

Loading

nirs left a comment •

edited

Loading

noisersup commented Feb 13, 2023 •

edited

Loading

noisersup commented Feb 14, 2023 •

edited

Loading

nirs commented Feb 19, 2023 •

edited

Loading

nirs commented Feb 21, 2023 •

edited

Loading