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

Always sanitize derivation name #166383

Merged
merged 7 commits into from
Apr 5, 2022

Conversation

roberth
Copy link
Member

@roberth roberth commented Mar 30, 2022

Description of changes

Make mkDerivation robust.

Fixes #166265

Naive change stats: https://github.com/NixOS/nixpkgs/pull/166383/checks?check_run_id=5751113354
2.7% total alloc, 8% call increase

With optimization: mixed stats.

TODO

  • optimize sanitizeDerivationName
  • time benchmark. Normally I'd be ok to rely on mem stats alone, but I don't trust regex without a good time measurement (which ofborg can't provide)

Before

benchmarking ~/nix/result/bin/nix-instantiate -A nixosTests.acme
time                 16.87 s    (15.23 s .. 18.43 s)
                     0.999 R²   (0.995 R² .. 1.000 R²)
mean                 16.71 s    (16.52 s .. 16.99 s)
std dev              294.7 ms   (27.40 ms .. 373.5 ms)
variance introduced by outliers: 19% (moderately inflated)

After

benchmarking ~/nix/result/bin/nix-instantiate -A nixosTests.acme
time                 16.77 s    (16.33 s .. 17.14 s)
                     1.000 R²   (1.000 R² .. 1.000 R²)
mean                 16.71 s    (16.66 s .. 16.79 s)
std dev              81.45 ms   (16.44 ms .. 107.6 ms)
variance introduced by outliers: 19% (moderately inflated)

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.05 Release Notes (or backporting 21.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

lib/strings.nix Outdated Show resolved Hide resolved
@roberth roberth force-pushed the always-sanitize-derivation-name branch from 2714486 to 2999ab1 Compare March 31, 2022 16:31
lib/strings.nix Outdated
@@ -756,7 +756,14 @@ rec {
sanitizeDerivationName pkgs.hello
=> "-nix-store-2g75chlbpxlrqn15zlby2dfh8hr9qwbk-hello-2.10"
*/
sanitizeDerivationName = string: lib.pipe string [
sanitizeDerivationName =
let okRegex = match "^[[:alnum:]+_?=-][[:alnum:]+._?=-]*$";
Copy link
Member

@infinisil infinisil Mar 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we could do all the regex matches from this function in one go. Like a regex that only matches invalid names, in which case it returns all the valid parts.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't have to do it for performance anyway, because you'll be optmizing for the rare case.
I can see how it might improve maintainability by DRY, but that may be offset by how ugly the expression may become. You can prove me wrong in a follow-up pr, after we decide whether this whole sanitize-everything idea is good or not :)

lib/strings.nix Outdated
@@ -756,7 +756,14 @@ rec {
sanitizeDerivationName pkgs.hello
=> "-nix-store-2g75chlbpxlrqn15zlby2dfh8hr9qwbk-hello-2.10"
*/
sanitizeDerivationName = string: lib.pipe string [
sanitizeDerivationName =
let okRegex = match "^[[:alnum:]+_?=-][[:alnum:]+._?=-]*$";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small nit: Nix's regex engine always adds an implicit ^/$ the beginning/end respectively, so these aren't needed here

`^` and `$` are implicit in `match`.
Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

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.

None yet

2 participants