change hash algorithm compatible for fetchNextcloudApp #3
Conversation
|
Hey isnt there a better way to calculate that hash instead of spawing seperate processes? And why is this a good idea? |
|
@Scriptkiddi I guess calculating hashes has changed since NixOS/nixpkgs#193075 and nc4nix probably didn't got updated yet. |
|
I think we should rework this to go code and then merge it |
onny
force-pushed
the
hashing-algo
branch
3 times, most recently
from
947d8b4
to
2bcb4fe
Compare
Found a way to calculate the hash in Go without depending on external commands :) |
onny
force-pushed
the
hashing-algo
branch
2 times, most recently
from
1d75aef
to
9f50c21
Compare
|
Thank you @leonklingele , fixed the issues 👍 |
|
Oh missed that, thank you! |
|
Is there anything I can test or further add please let me know :) |
|
@onny thanks! Code-wise this lgtm, what's the intention behind this change though? Does the current version (i.e. latest |
|
yes the hash is now different, probably because fetchnextcloudapp requires a recursive hash, not of the single zip file like before, but of the zip content recursively https://github.com/NixOS/nixpkgs/blob/c5e62ec76a0198822c1cf017f83c1d4c9d1ceb60/pkgs/servers/nextcloud/packages/default.nix#L18 |
|
To me this change looks like it's switching to the new hash representation used by nix which additionally allows for different hashing algorithms (e.g. @Conni2461 @dasJ what do you think? nc4nix is more performant when not using this change, and instead leaving the hash-calculation to stdlib's |
sha256.Sum256 only hashes the zip file whereas this PR calculates the hash by unpacking the zip and calculating the hash recursively over all containing files, see: the unpacked hash is used in fetchNextcloudApp because this function also uses fetchzip with recursive hash calculation: https://github.com/NixOS/nixpkgs/blob/791355b27c5bfc58f7339a3dafc936ec50130e01/pkgs/build-support/fetchnextcloudapp/default.nix I'm not sure if it's possible and recommended by upstream to use the single zip file hash to package nextcloud apps |
I'm not 100% fluent in Nix but I can't see a difference here from a integrity-preserving point of view. Let's see what my work mates have to say. I'll get back to you tomorrow! :) |
| dname, err := os.MkdirTemp("", "") | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to create temp dir: %w", err) | ||
| } |
There was a problem hiding this comment.
Just noticed that this directory should be removed after use here, and not further down below, i.e.
| dname, err := os.MkdirTemp("", "") | |
| if err != nil { | |
| return "", fmt.Errorf("failed to create temp dir: %w", err) | |
| } | |
| dname, err := os.MkdirTemp("", "") | |
| if err != nil { | |
| return "", fmt.Errorf("failed to create temp dir: %w", err) | |
| } | |
| defer func() { | |
| if err := os.RemoveAll(dname); err != nil { | |
| log.Printf("failed to clean up temp dir %q: %v", dname, err) | |
| } | |
| }() |
| if err := nar.DumpPath(h, entryPath); err != nil { | ||
| return "", fmt.Errorf("failed to dump path: %w", err) | ||
| } | ||
| defer os.RemoveAll(dname) |
There was a problem hiding this comment.
| defer os.RemoveAll(dname) |
|
My work mates are busy this week, unfortunately. We‘ll get back to you asap!
|
|
I checked the output and somehow, there are some empty values. If I run Otherwise, the PR looks good to me and can be merged. |
|
thank you so much for helping and merging this :) |
No description provided.