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

sasquatch: init #66600

Merged
merged 2 commits into from Sep 15, 2019
Merged

sasquatch: init #66600

merged 2 commits into from Sep 15, 2019

Conversation

Pamplemousse
Copy link
Member

Fixes #66270 .

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Fits CONTRIBUTING.md.

Copy link
Member

@aanderse aanderse left a 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.

pkgs/tools/filesystems/sasquatch/default.nix Outdated Show resolved Hide resolved
pkgs/tools/filesystems/sasquatch/default.nix Outdated Show resolved Hide resolved
makeFlags = [ "XZ_SUPPORT=1" ]
++ stdenv.lib.optional lz4Support "LZ4_SUPPORT=1";

meta = {
Copy link
Member

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.

Copy link
Member Author

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...

Copy link
Member

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?

Copy link
Member Author

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 .

pkgs/tools/filesystems/sasquatch/default.nix Outdated Show resolved Hide resolved
pkgs/tools/filesystems/sasquatch/default.nix Show resolved Hide resolved
pkgs/tools/filesystems/sasquatch/default.nix Show resolved Hide resolved
pkgs/tools/filesystems/sasquatch/default.nix Outdated Show resolved Hide resolved
pkgs/tools/filesystems/sasquatch/default.nix Show resolved Hide resolved
pkgs/tools/filesystems/sasquatch/default.nix Outdated Show resolved Hide resolved
pkgs/tools/filesystems/sasquatch/default.nix Show resolved Hide resolved
@Pamplemousse
Copy link
Member Author

Pamplemousse commented Aug 27, 2019

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 squashfs it works with, it seems that the current version of sasquatch specifically targets 4.3...
(See their build script, pulling a specific version from sourceforge).
*Maybe* the patch would be working for now, but would that be worth adding to the current squashfs package as a variant, considering the risk of divergence in the future? (hope this is clear...)

Copy link
Member

@aanderse aanderse left a 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?

pkgs/tools/filesystems/sasquatch/default.nix Outdated Show resolved Hide resolved
Copy link
Member

@aanderse aanderse 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 following up on the license! Just a few small things I noticed.

maintainers/maintainer-list.nix Show resolved Hide resolved
pkgs/tools/filesystems/sasquatch/default.nix Outdated Show resolved Hide resolved
pkgs/tools/filesystems/sasquatch/default.nix Outdated Show resolved Hide resolved
@Pamplemousse
Copy link
Member Author

Regarding your comment about the -lgcc_s flag, I do not know.

I had a look around:

  • The squashfs patch does not seem to touch anything related to pthread_cancel;
  • The used Makefile (from squashfs, used in sasquatch's build script), does not have any kind of mention of the gcc_s lib.

So, I would not be inclined adding it.

@Pamplemousse Pamplemousse force-pushed the sasquatch branch 2 times, most recently from bf0065a to 2e5e587 Compare September 3, 2019 16:48
@Pamplemousse
Copy link
Member Author

Regarding the CVEs, I have can found two related to sasquatch: CVE-2015-4645 and CVE-2015-4646.

As per this comment on devttys0/sasquatch#5, fixes are taken care of upstream (in squashfs).

@aanderse
Copy link
Member

aanderse commented Sep 4, 2019

Thanks. Maybe @ruuda, @danbst, or @charles-dyfis-net could take a quick look as people familiar with squashfsTools.

@ruuda
Copy link
Contributor

ruuda commented Sep 5, 2019

Looks good to me. I think it is good to keep the sources separate and not base this package on squashfstools. Otherwise, when there is a squashfstools update and the patches no longer apply, we either need to wait for upstream Sasquatch to update the patches before we can upgrade squashfstools, or we need to disable the sasquatch package. Now they can coexist.

@aanderse
Copy link
Member

aanderse commented Sep 5, 2019

@Pamplemousse at this point the only thing missing is a license then. Ready for merge once that situation is resolved. Thanks! 👍

@aanderse
Copy link
Member

Thanks @Pamplemousse, and welcome to the team 🎉

@aanderse aanderse merged commit 6f24ec8 into NixOS:master Sep 15, 2019
@Pamplemousse Pamplemousse deleted the sasquatch branch August 11, 2021 03:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add sasquatch
3 participants