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

stdenv: support multi-char separators in concatStringsSep #360466

Conversation

wolfgangwalther
Copy link
Contributor

@wolfgangwalther wolfgangwalther commented Nov 30, 2024

One prominent use-case for this is pytestCheckHook. This will help making it work with structuredAttrs in the future.

As discussed in https://github.com/NixOS/nixpkgs/pull/347194/files#r1818073811.

cc @ShamrockLee @NixOS/stdenv

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/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 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.

Add a 👍 reaction to pull requests you find important.

One prominent use-case for this is pytestCheckHook. This will help
making it work with structuredAttrs in the future.
Copy link
Contributor

@philiptaron philiptaron left a comment

Choose a reason for hiding this comment

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

Very nice.

@philiptaron philiptaron merged commit 815e926 into NixOS:staging Dec 2, 2024
53 of 54 checks passed
@wolfgangwalther wolfgangwalther deleted the structured-attrs-concat-strings-sep-multi-char branch December 2, 2024 08:36
@trofi
Copy link
Contributor

trofi commented Dec 9, 2024

Bisect claims that 56b0962 stdenv: support multi-char separators in concatStringsSep broke nix develop at least for nix-2.25.2 on staging:

$ nix develop -f. stdenv.cc.cc
error: [json.exception.parse_error.101] parse error at line 38, column 445: syntax error while parsing value - invalid string: control character U+001E (RS) must be escaped to \u001E; last read: '" \n    local sep=\"$1\";\n    local name=\"$2\";\n    local type oldifs;\n    if type=$(declare -p \"$name\" 2> /dev/null); then\n        local -n nameref=\"$name\";\n        case \"${type#* }\" in \n            -A*)\n                echo \"concatStringsSep(): ERROR: trying to use concatStringsSep on an associative array.\" 1>&2;\n                return 1\n            ;;\n            -a*)\n                local IFS='<U+001E>'

$ nix develop -f. re2c
error: [json.exception.parse_error.101] parse error at line 40, column 445: syntax error while parsing value - invalid string: control character U+001E (RS) must be escaped to \u001E; last read: '" \n    local sep=\"$1\";\n    local name=\"$2\";\n    local type oldifs;\n    if type=$(declare -p \"$name\" 2> /dev/null); then\n        local -n nameref=\"$name\";\n        case \"${type#* }\" in \n            -A*)\n                echo \"concatStringsSep(): ERROR: trying to use concatStringsSep on an associative array.\" 1>&2;\n                return 1\n            ;;\n            -a*)\n                local IFS='<U+001E>'

@wolfgangwalther
Copy link
Contributor Author

Uh, that's odd. Will look into it today.

@trofi
Copy link
Contributor

trofi commented Dec 9, 2024

A bit of debugging:

The environment persisted expanded IFS value to:

$ nix develop --debug -f. stdenv.cc.cc
...
reading environment file '/nix/store/wvlypb02pl61qwbbmadxnm9lsmbjwlkr-gcc-14-20241116-env'
error: [json.exception.parse_error.101] parse error at line 38, column 445: syntax error while parsing value - invalid string: control character U+001E (RS) must be escaped to \u001E; last read: '" \n    local sep=\"$1\";\n    local name=\"$2\";\n    local type oldifs;\n    if type=$(declare -p \"$name\" 2> /dev/null); then\n        local -n nameref=\"$name\";\n        case \"${type#* }\" in \n            -A*)\n                echo \"concatStringsSep(): ERROR: trying to use concatStringsSep on an associative array.\" 1>&2;\n                return 1\n            ;;\n            -a*)\n                local IFS='<U+001E>'

The env is generated as:

$ nix derivation show /nix/store/wvlypb02pl61qwbbmadxnm9lsmbjwlkr-gcc-14-20241116-env
{
  "/nix/store/0c69903g1i7nxsw39ihb2gr2ma1nz853-gcc-14-20241116-env.drv": {
    "args": [
      "/nix/store/08i4419qr06fflq7qp20rxnrhw2c06vq-get-env.sh"
    ],
    "builder": "/nix/store/razasrvdg7ckplfmvdxv4ia3wbayr94s-bootstrap-tools/bin/bash",
...

I think get-env.sh comes from nix from https://github.com/NixOS/nix/blob/master/src/nix/get-env.sh

@wolfgangwalther
Copy link
Contributor Author

So the concatStringsSep function is dumped in get-env.sh with type. This dumps the record separator as a literal character and not encoded - even though we have it encoded in our source.

Consider:

bash-5.2$ function test() {
> local IFS=$'\036'
> }
bash-5.2$ type test
test is a function
test ()
{
    local IFS='�'
}

I can't actually see that record separator on my screen, but when I dump the output to a file and open it with my editor, it's there - unencoded.

But also consider this:

bash-5.2$ function test2() {
> local IFS="$(printf '\036')"
> }
bash-5.2$ type test2
test2 is a function
test2 ()
{
    local IFS="$(printf '\036')"
}
bash-5.2$ [ "$(printf '\036')" == $'\036' ]; echo same
same

Thus we can use printf to print the encoded character - and this should not affect the dump.

I can prepare a PR later.

wolfgangwalther added a commit to wolfgangwalther/nixpkgs that referenced this pull request Dec 9, 2024
This was introduced in NixOS#360466.
@wolfgangwalther wolfgangwalther mentioned this pull request Dec 9, 2024
13 tasks
@philiptaron
Copy link
Contributor

See also NixOS/nix#11921 for the Nix-side PR on this topic.

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.

3 participants