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

[ci] fix valgrind workflow #6816

Merged
merged 4 commits into from
Feb 10, 2025
Merged

[ci] fix valgrind workflow #6816

merged 4 commits into from
Feb 10, 2025

Conversation

jameslamb
Copy link
Collaborator

Replaces #6815

The valgrind CI job is failing like this:

/__w/_temp/a09e15f1-9453-4607-9d54-66e1c2404ea4.sh: 6: comment+=https://github.com/microsoft/LightGBM/actions/runs/13159252384: not found

(build link)

This attempts to fix that.

Notes for Reviewers

As described in #6743, += is a non-POSIX behavior.
My guess is that using it in run: in GitHub Actions steps is problematic if the shell is not one that recognizes +=.

It should be the case that GitHub Actions is using bash in these jobs, which shouldn't be a problem. Per https://docs.github.com/en/actions/writing-workflows/workflow-syntax-for-github-actions#defaultsrunshell

shell unspecified

[bash -e {0} is] the default shell on non-Windows platforms. Note that this runs a different command to when bash is specified explicitly. If bash is not found in the path, this is treated as sh.

I'm not sure why (or even if) bash is not being found. The container image for the valgrind job definitely does have it.

docker run \
  --rm \
  -it wch1/r-debug \
  bash --version

# GNU bash, version 5.1.16(1)-release (x86_64-pc-linux-gnu)

This PR attempts to fix that job in 2 ways:

  • removing uses of += in Github Actions workflow steps
  • explicitly setting shell: bash in those steps

How to Test This

I think that we'll have to merge this to test it, because the comment-triggered workflows pull configuration from master: #6815 (comment)

Copy link
Collaborator

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

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

Ah, thanks a lot for this PR!

Indeed, I forgot about that too

I forgot that for these comment-triggered workflows, the GitHub Actions config is sourced from master. We'll have to merge these changes to test them.
#6815 (comment)

and my original check in #6758 (comment) made no sense. I'm really so sorry about that!

.github/workflows/r_valgrind.yml Outdated Show resolved Hide resolved
@jameslamb
Copy link
Collaborator Author

and my original check in #6758 (comment) made no sense. I'm really so sorry about that!

It's ok! I reviewed #6758 and also forgot about this 😅

@jameslamb jameslamb merged commit 1531d87 into master Feb 10, 2025
49 checks passed
@jameslamb jameslamb deleted the ci/fix-valgrind-v2 branch February 10, 2025 02:13
@jameslamb
Copy link
Collaborator Author

This worked!

#6802 (comment)

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.

2 participants