Skip to content

stdenv: support multi-char separators in concatStringsSep#360466

Merged
philiptaron merged 1 commit intoNixOS:stagingfrom
wolfgangwalther:structured-attrs-concat-strings-sep-multi-char
Dec 2, 2024
Merged

stdenv: support multi-char separators in concatStringsSep#360466
philiptaron merged 1 commit intoNixOS:stagingfrom
wolfgangwalther:structured-attrs-concat-strings-sep-multi-char

Conversation

@wolfgangwalther
Copy link
Contributor

@wolfgangwalther wolfgangwalther commented Nov 30, 2024 •
edited
Loading

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.

@wolfgangwalther
stdenv: support multi-char separators in concatStringsSep
56b0962 
One prominent use-case for this is pytestCheckHook. This will help
making it work with structuredAttrs in the future.
@github-actions github-actions bot added 6.topic: python Python is a high-level, general-purpose programming language. 6.topic: stdenv Standard environment labels Nov 30, 2024
@wolfgangwalther wolfgangwalther mentioned this pull request Nov 30, 2024
Merged
13 tasks
@ofborg ofborg bot added 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 10.rebuild-linux-stdenv This PR causes stdenv to rebuild on Linux and must target a staging branch. 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. labels Dec 1, 2024
philiptaron
philiptaron approved these changes Dec 2, 2024
View reviewed changes
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
@wolfgangwalther wolfgangwalther deleted the structured-attrs-concat-strings-sep-multi-char branch December 2, 2024 08:36
@wolfgangwalther wolfgangwalther mentioned this pull request Dec 2, 2024
Open
4 tasks
@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

wolfgangwalther commented Dec 9, 2024

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

wolfgangwalther commented Dec 9, 2024

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
@wolfgangwalther
stdenv: fix nix develop
2332759 
This was introduced in NixOS#360466.
@wolfgangwalther wolfgangwalther mentioned this pull request Dec 9, 2024
Merged
13 tasks
@philiptaron
Copy link
Contributor

philiptaron commented Dec 9, 2024

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

Reviewers

@philiptaron philiptaron philiptaron approved these changes

@Artturin Artturin Awaiting requested review from Artturin

@emilazy emilazy Awaiting requested review from emilazy

@Ericson2314 Ericson2314 Awaiting requested review from Ericson2314

@mweinelt mweinelt Awaiting requested review from mweinelt

@natsukium natsukium Awaiting requested review from natsukium

@reckenrode reckenrode Awaiting requested review from reckenrode

@RossComputerGuy RossComputerGuy Awaiting requested review from RossComputerGuy

Assignees

No one assigned

Labels

6.topic: python Python is a high-level, general-purpose programming language. 6.topic: stdenv Standard environment 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. 10.rebuild-linux-stdenv This PR causes stdenv to rebuild on Linux and must target a staging branch.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@wolfgangwalther @trofi @philiptaron