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

Jump to bottom

scrypt: 1.3.0 → 1.3.1, build library, enable tests #98059

Merged
merged 2 commits into from Oct 14, 2020
Merged

scrypt: 1.3.0 → 1.3.1, build library, enable tests #98059

merged 2 commits into from Oct 14, 2020

Conversation

sternenseemann
Copy link
Member

@sternenseemann sternenseemann commented Sep 15, 2020 •
edited

  • update scrypt
  • enable running of tests
  • build development library libscrypt-kdf,
    install to lib output, headers to dev
  • default output remains untouched: contains binary plus man pages

cc @thoughtpolice

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@sternenseemann
scrypt: 1.3.0 → 1.3.1, build library, enable tests
c66aaf1 
* update scrypt
* enable running of tests
* build development library libscrypt-kdf,
  install to lib output, headers to dev
* default output remains untouched: contains binary plus man pages
@ofborg ofborg bot requested a review from thoughtpolice September 15, 2020 18:34
@sternenseemann
Copy link
Member Author

Odd thing about this, is that it doesn't install the crypto/crypto_scrypt.h header like the documentation says, but debian also has it this way, not sure what we should do.

Also: Do we install *.a to dev or lib?

@thoughtpolice
Copy link
Member

thoughtpolice commented Sep 16, 2020 •
edited

.a files should go in $lib, while $dev should contain header files only. You can also add a $man output so that $out won't needlessly contain the man pages too. I wouldn't block this PR on that or anything, but it would be a nice to have.

The lack of crypto_scrypt.h is perhaps more concerning; it's also possible this is a bug in Debian's installer or even the scrypt package itself. What do the consumers of libscrypt-kdf typically ask for?

@sternenseemann
Copy link
Member Author

sternenseemann commented Sep 16, 2020 •
edited

The lack of crypto_scrypt.h is perhaps more concerning; it's also possible this is a bug in Debian's installer or even the scrypt package itself. What do the consumers of libscrypt-kdf typically ask for?

Seems like it was a conscious decision to rename the header and main function for the shared library: Tarsnap/scrypt#168 (comment) I also found one instance of libscrypt-kdf being used in actual C code with a quick github search and there the scrypt-kdf.h header is used.

I'll open I've opened an issue about the misleading documentation on their website.

.a files should go in $lib, while $dev should contain header files only.

Then this is already working correctly.

You can also add a $man output so that $out won't needlessly contain the man pages too. I wouldn't block this PR on that or anything, but it would be a nice to have.

I've generally experienced hesitance to add a new output for man pages of small size (or a single one in this case), so I didn't add a separate output (seemingly the consensus in nixpkgs seems to be to use them sparingly?), but I'd be happy to add one if you prefer it.

@sternenseemann
scrypt: fix build of pkgsStatic.scrypt
ac4d469 
The getconf input defaults to the glibc one if it is being used and uses
the netbsd version in all other cases. This fixes the build when
building with musl, since it doesn't ship a version of getconf.
@sternenseemann
Copy link
Member Author

@GrahamcOfBorg build pkgsStatic.scrypt

@gperciva
Copy link

@sternenseemann has correctly summarized scrypt's position about scrypt_kdf(): that's the intended public API for libscrypt-kdf. crypto_scrypt() was the function to use before we had a library, when people just copied our files into their projects directly.

We'll be updating our README.md and website soon.

@thoughtpolice thoughtpolice merged commit 1eb4b2c into NixOS:master Oct 14, 2020
@sternenseemann sternenseemann deleted the scrypt-1.3.1 branch October 14, 2020 23:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thoughtpolice thoughtpolice thoughtpolice approved these changes

Assignees
No one assigned
Labels
10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@sternenseemann @thoughtpolice @gperciva