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

Fix valgrind job #413

Merged
merged 1 commit into from
Apr 4, 2024
Merged

Fix valgrind job #413

merged 1 commit into from
Apr 4, 2024

Conversation

ctz
Copy link
Member

@ctz ctz commented Apr 4, 2024

export VALGRIND="valgrind -q" doesn't work as intended:

  • in GH actions, environment alterations don't affect subsequent run steps
  • in tests/client_server.rs, we require VALGRIND to be a single command to execute: no shell expansion takes place so no arguments are allowed

`export VALGRIND="valgrind -q"` doesn't work as intended:

- in GH actions, environment alterations don't affect subsequent `run` steps
- in `tests/client_server.rs`, we require `VALGRIND` to be a single command to
  execute: no shell expansion takes place so no arguments are allowed
@@ -52,8 +52,7 @@ jobs:
persist-credentials: false
- name: Install valgrind
run: sudo apt-get update && sudo apt-get install -y valgrind
- run: export VALGRIND="valgrind -q"
- run: make test integration
- run: VALGRIND=valgrind make test integration
Copy link
Member

@cpu cpu Apr 4, 2024

Choose a reason for hiding this comment

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

Good catch 😭 It looks like the VALGRIND env manipulation has been inoperable since it was introduced in 456d64b

@cpu
Copy link
Member

cpu commented Apr 4, 2024

Since this is a simple fix and doesn't involve any non-CI updates I think it's fair to merge without blocking on additional reviews.

@cpu cpu merged commit 6d69958 into main Apr 4, 2024
42 checks passed
@cpu cpu deleted the jbp-valgrind branch April 4, 2024 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants