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
sasquatch: init #66600
sasquatch: init #66600
Conversation
2b7e714
to
960ff9f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! Thanks for deciding to contribute to nixpkgs and adding yourself as a maintainer. Sorry we took a while to get around to reviewing your PR.
I have made some comments I hope you will find helpful. The most important comment is my question about whether the version of squashfs
we have in nixpkgs is compatible with the patch sasquatch
provides. If the patch is compatible we could just modify the existing squashfs
package to make a new sasquatch
variant of it, and save yourself a fair bit of work. Let me know.
Please let me know if you have any questions.
makeFlags = [ "XZ_SUPPORT=1" ] | ||
++ stdenv.lib.optional lz4Support "LZ4_SUPPORT=1"; | ||
|
||
meta = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A common thing to do is write meta = with stdenv.lib; {
to save yourself some typing in maintainers
and, platforms
, and license
.
Please include a license
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They do not provide a licence in the github repo...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arch seems to think GPL for some reason... but I don't see any justification for that 😕 Maybe you could ask upstream to clarify?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guess that is because sasquatch
is just a set of patch for squashfs
, which is itself GPL-2.0 .
Hi @aanderse; Thank you for your feedback! No worries regarding the delays, I understand you are crumbling under the amount of PRs the repo receive :) I have mostly commented directly on your remarks. Regarding the version of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also noticed NIX_LDFLAGS = "-lgcc_s"; # for pthread_cancel
from the history of our squashfsTools. Is that needed?
And how about CVEs listed against squashfsTools
at version 4.3? Any?
960ff9f
to
3e840c7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for following up on the license
! Just a few small things I noticed.
Regarding your comment about the I had a look around:
So, I would not be inclined adding it. |
bf0065a
to
2e5e587
Compare
Regarding the CVEs, I have can found two related to As per this comment on devttys0/sasquatch#5, fixes are taken care of upstream (in |
2e5e587
to
f107d79
Compare
f107d79
to
ca2858e
Compare
Thanks. Maybe @ruuda, @danbst, or @charles-dyfis-net could take a quick look as people familiar with |
Looks good to me. I think it is good to keep the sources separate and not base this package on |
@Pamplemousse at this point the only thing missing is a license then. Ready for merge once that situation is resolved. Thanks! 👍 |
ca2858e
to
d25bf04
Compare
Thanks @Pamplemousse, and welcome to the team 🎉 |
Fixes #66270 .
Things done
sandbox
innix.conf
on non-NixOS)nix-shell -p nix-review --run "nix-review wip"