Skip to content

lib.escapeRegex: fix with boost::regex implementation#271245

Closed
9999years wants to merge 1 commit intoNixOS:masterfrom
9999years:fix-escapeRegex
Closed

lib.escapeRegex: fix with boost::regex implementation#271245
9999years wants to merge 1 commit intoNixOS:masterfrom
9999years:fix-escapeRegex

Conversation

@9999years
Copy link
Contributor

NixOS/nix#7762 switched nix to use the boost::regex implementation, which expects that all literal braces are escaped. That is, the regex \{crate} no longer parses. We modify lib.escapeRegex to accommodate this change. Fortunately, the old regex implementation doesn't mind if closing braces are escaped as well, so this is backwards compatible and we don't need to worry about version-gating it.

Here's how nix works without this patch:

Old regex implementation:

$ nix repl nixpkgs
Welcome to Nix 2.18.1. Type :? for help.

Loading installable 'flake:nixpkgs#'...
Added 5 variables.
nix-repl> builtins.match (lib.escapeRegex "{}") "{}"
[ ]

$ ~/nix/result/bin/nix repl nixpkgs
Welcome to Nix 2.20.0pre20231130_dirty. Type :? for help.

boost::regex implementation:

$ ~/nix/result/bin/nix repl nixpkgs
Welcome to Nix 2.20.0pre20231130_dirty. Type :? for help.

Loading installable 'flake:nixpkgs#'...
Added 5 variables.
nix-repl> builtins.match (lib.escapeRegex "{}") "{}"
error:
       … while calling the 'match' builtin

         at «string»:1:1:

            1| builtins.match (lib.escapeRegex "{}") "{}"
             | ^

       error: invalid regular expression '\{}'

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • 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/)
  • 24.05 Release Notes (or backporting 23.05 and 23.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
  • Fits CONTRIBUTING.md.

Priorities

Add a 👍 reaction to pull requests you find important.

NixOS/nix#7762 switched `nix` to use the
`boost::regex` implementation, which expects that all literal braces are
escaped. That is, the regex `\{crate}` no longer parses. We modify
`lib.escapeRegex` to accommodate this change. Fortunately, the old regex
implementation doesn't mind if closing braces are escaped as well, so
this is backwards compatible and we don't need to worry about
version-gating it.

Without this patch:

Old regex implementation:

    $ nix repl nixpkgs
    Welcome to Nix 2.18.1. Type :? for help.

    Loading installable 'flake:nixpkgs#'...
    Added 5 variables.
    nix-repl> builtins.match (lib.escapeRegex "{}") "{}"
    [ ]

    $ ~/nix/result/bin/nix repl nixpkgs
    Welcome to Nix 2.20.0pre20231130_dirty. Type :? for help.

`boost::regex` implementation:

    $ ~/nix/result/bin/nix repl nixpkgs
    Welcome to Nix 2.20.0pre20231130_dirty. Type :? for help.

    Loading installable 'flake:nixpkgs#'...
    Added 5 variables.
    nix-repl> builtins.match (lib.escapeRegex "{}") "{}"
    error:
           … while calling the 'match' builtin

             at «string»:1:1:

                1| builtins.match (lib.escapeRegex "{}") "{}"
                 | ^

           error: invalid regular expression '\{}'
@9999years 9999years requested a review from infinisil as a code owner November 30, 2023 18:18
@github-actions github-actions bot added the 6.topic: lib The Nixpkgs function library label Nov 30, 2023
@infinisil
Copy link
Member

infinisil commented Nov 30, 2023

This is a Nix regression that should just get reverted. Nixpkgs is the best test suite we have for Nix, if there's a regression for Nixpkgs there will also be regressions for third-parties.

I submitted a revert PR here: NixOS/nix#9508

@9999years
Copy link
Contributor Author

But the fix is back-compatible -- why not just roll forward?

@infinisil
Copy link
Member

@9999years The problem is that Nix broke backwards compatibility. Older versions of Nixpkgs should always work with newer Nix versions.

@infinisil
Copy link
Member

Revert in NixOS/nix#9508 was merged, it also includes your report as a test case, so this shouldn't be a problem going forward :)

@infinisil infinisil closed this Dec 1, 2023
@9999years 9999years deleted the fix-escapeRegex branch December 1, 2023 22:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: lib The Nixpkgs function library

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants