Skip to content

python3Packages.pytestCheckHook: fix Bash heredoc EOF outside subshell#403973

Merged
wolfgangwalther merged 2 commits intoNixOS:stagingfrom
ShamrockLee:pytestCheckHook-fix-heredoc
May 3, 2025
Merged

python3Packages.pytestCheckHook: fix Bash heredoc EOF outside subshell#403973
wolfgangwalther merged 2 commits intoNixOS:stagingfrom
ShamrockLee:pytestCheckHook-fix-heredoc

Conversation

@ShamrockLee
Copy link
Contributor

@ShamrockLee ShamrockLee commented May 3, 2025

This PR fixes the unterminated here document of the pytest-check-hook.sh error mesage by moving the EOF of the here document between the parenthesis of the process substitution.

Context: #386513 (review)

The error it fixes is passed silently due to Bash's tolerance to errors inside prcess substitution subshell, but the error message is printed out to the build log.

Here's the partial build logs of python3Packages.tests.basic before and after this fix:

Before:

python3.12-test-pytestCheckHook-basic-objprint> Sourcing pytest-check-hook
python3.12-test-pytestCheckHook-basic-objprint> /nix/store/lbw329diypjdljmc229l9h6zd51drp2v-pytest-check-hook/nix-support/setup-hook: line 41: warning: command substitution: 1 unterminated here-document
python3.12-test-pytestCheckHook-basic-objprint> Using pytestCheckPhase

After:

python3.12-test-pytestCheckHook-basic-objprint> Sourcing pytest-check-hook
python3.12-test-pytestCheckHook-basic-objprint> Using pytestCheckPhase

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.

ShamrockLee added 2 commits May 4, 2025 01:31
@ShamrockLee
python3Packages.pytestCheckHook: fix Bash heredoc EOF outside subshell
63dea02 
Fix the unterminated here document of the pytest-check-hook.sh
error mesage by moving the EOF of the here document
between the parenthesis of the process substitution.
@ShamrockLee
python3Packages.pytestCheckHook: format with shfmt
842f12f
@github-actions github-actions bot added the 6.topic: python Python is a high-level, general-purpose programming language. label May 3, 2025
@ShamrockLee ShamrockLee mentioned this pull request May 3, 2025
Merged
13 tasks
@nix-owners nix-owners bot requested review from mweinelt and natsukium May 3, 2025 17:51
@github-actions github-actions bot added 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin 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: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. labels May 3, 2025
wolfgangwalther
wolfgangwalther requested changes May 3, 2025
View reviewed changes
Copy link
Contributor

@wolfgangwalther wolfgangwalther left a comment

Choose a reason for hiding this comment

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

I think we should add a test that covers the functionality inside the heredoc. This would then be a test that actually fails when anything like this breaks. Given that the whole subshell / kill etc. stuff is rather hacky, this would make me feel much better about it.

@wolfgangwalther
Copy link
Contributor

wolfgangwalther commented May 3, 2025

We seem to already have some test-cases, but at least one of them doesn't seem to work properly:

          enabledTestPaths-glob = objprint.overridePythonAttrs (previousPythonAttrs: {
            pname = "test-pytestCheckHook-enabledTestPaths-glob-${previousPythonAttrs.pname}";
            enabledTestPaths = [
              "tests/test_obj*.py"
            ] ++ previousPythonAttrs.enabledTestPaths or [ ];
          });
          enabledTestPaths-glob-nonexistent = testers.testBuildFailure (
            objprint.overridePythonAttrs (previousPythonAttrs: {
              pname = "test-pytestCheckHook-enabledTestPaths-glob-nonexistent-${previousPythonAttrs.pname}";
              enabledTestPaths = [
                "tests/test_foo*.py"
              ] ++ previousPythonAttrs.enabledTestPaths or [ ];
            })
          );

Aka, why is the second one not failing? I guess we need to look for the error message that is actually returned from the python heredoc - otherwise it probably just fails later for some other reasons or so?

@wolfgangwalther
Copy link
Contributor

wolfgangwalther commented May 3, 2025

Ok, this is confusing. Before this PR, I get this for the second test mentioned above (non existent glob):

...
original builder: /nix/store/lbw329diypjdljmc229l9h6zd51drp2v-pytest-check-hook/nix-support/setup-hook: line 41: warning: command substitution: 1 unterminated here-document
...
original builder: Enabled tests path glob "tests/test_foo*.py" does not match any paths. Aborting
testBuildFailure: Original builder produced exit code: 143

This very much looks like... the python code is actually running, despite the warning? Maybe the warning is actually false alarm... and we are "only" fixing the warning here, but not the actual feature.

@wolfgangwalther
Copy link
Contributor

wolfgangwalther commented May 3, 2025

Oh my, I hate bash...

$ cat <(cat <<EOF)
foo
EOF
bash: warning: command substitution: 1 unterminated here-document
> > foo

So bash complains about the unterminated here-doc, but still proceeds to work perfectly fine with it.

wolfgangwalther
wolfgangwalther approved these changes May 3, 2025
View reviewed changes
Copy link
Contributor

@wolfgangwalther wolfgangwalther left a comment

Choose a reason for hiding this comment

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

Works for me to suppress the warning.

@wolfgangwalther
Copy link
Contributor

wolfgangwalther commented May 3, 2025

And for the record, zsh behaves as expected:

% cat <(cat <<EOF)
foo
EOF

foo: command not found
EOF: command not found

@wolfgangwalther wolfgangwalther merged commit 8c835e0 into NixOS:staging May 3, 2025
24 of 27 checks passed
@ShamrockLee ShamrockLee deleted the pytestCheckHook-fix-heredoc branch May 4, 2025 18:08
@ShamrockLee
Copy link
Contributor Author

ShamrockLee commented May 4, 2025

Oh my, I hate bash...

$ cat <(cat <<EOF)
foo
EOF
bash: warning: command substitution: 1 unterminated here-document
> > foo

So bash complains about the unterminated here-doc, but still proceeds to work perfectly fine with it.

I filed an issue (koalaman/shellcheck#3199) to ShellCheck, the missing (?) syntax checker for Bash.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wolfgangwalther wolfgangwalther wolfgangwalther approved these changes

@natsukium natsukium Awaiting requested review from natsukium

@mweinelt mweinelt Awaiting requested review from mweinelt

Assignees

No one assigned

Labels

6.topic: python Python is a high-level, general-purpose programming language. 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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ShamrockLee @wolfgangwalther