Skip to content

DAOS-18905 build: fix in_list PATH check due to missing word-split#18482

Open
knard38 wants to merge 2 commits into
masterfrom
ckochhof/fix/master/daos-18905/patch-001
Open

DAOS-18905 build: fix in_list PATH check due to missing word-split#18482
knard38 wants to merge 2 commits into
masterfrom
ckochhof/fix/master/daos-18905/patch-001

Conversation

@knard38

@knard38 knard38 commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Description

This PR fixes two related issues in utils/sl/setup_local.sh.

Commit 1 — word-split bug (d51c4d1027)

setup_local.sh uses the in_list helper to avoid prepending $SL_PREFIX/bin to PATH when it is already present. The guard was broken because $old_path (and $added) were quoted when passed to in_list, so the entire colon-to-space-converted PATH string was passed as a single argument instead of being word-split into individual directories. As a result in_list never found a match and $SL_PREFIX/bin was unconditionally prepended every time setup_local.sh was sourced — even in the same shell session.

The same quoting mistake affected both the per-prefix loop (${added}, ${old_path}) and the final SL_PREFIX/bin check (${old_path}).

Commit 2 — latent set -e hazard (16925e48ce)

Once the word-split fix is in place, in_list correctly returns 1 for the "found" case. However, in_list was called as a standalone statement with its exit code checked afterward via $?. Scripts that source setup_local.sh under set -e (e.g. utils/rpms/build_packages.sh) abort immediately on that non-zero return before the $? check runs.

This was a latent design flaw independent of the quoting bug: in_list should always be called inside an if condition, not as a standalone statement with $? checked afterward. It surfaced here because fixing the quoting made in_list actually return 1 for the first time, triggering the abort in callers like utils/rpms/build_packages.sh — which is why the initial CI run on this PR showed build failures. The two commits are kept separate intentionally: the second independently documents the set -e hazard so it is findable in git log on its own. No explicit validation is needed — the CI passing on the second run is sufficient evidence.

Validation

Both steps below must be run from the repo root (where .build_vars.sh lives after a successful build), as setup_local.sh looks for that file relative to $PWD.

1. Without the fix — check out the base branch before this patch (commit 05984ffd13):

git checkout 05984ffd13
# Source three times and watch PATH grow by one entry each time:
for i in 1 2 3; do
    source utils/sl/setup_local.sh > /dev/null
    echo "source #$i: $(echo "$PATH" | tr ':' '\n' | grep -c install) install entries in PATH"
done

2. With the fix — check out the tip of this branch (commit 16925e48ce):

git checkout 16925e48ce
# Reset PATH to remove any duplicates accumulated above, then repeat:
for i in 1 2 3; do
    source utils/sl/setup_local.sh > /dev/null
    echo "source #$i: $(echo "$PATH" | tr ':' '\n' | grep -c install) install entries in PATH"
done

Without the fix the install entry count grows by one on every source. With the fix it stays constant.

Note: .build_vars.sh must exist in the repo root (generated by a successful build). Without it setup_local.sh exits early and PATH is not modified at all.

Steps for the author:

  • Commit message follows the guidelines.
  • Appropriate Features or Test-tag pragmas were used.
  • Appropriate Functional Test Stages were run.
  • At least two positive code reviews including at least one code owner from each category referenced in the PR.
  • Testing is complete. If necessary, forced-landing label added and a reason added in a comment.

After all prior steps are complete:

  • Gatekeeper requested (daos-gatekeeper added as a reviewer).

$old_path and $added were quoted when passed to in_list, causing
the entire space-separated string to be treated as a single argument.
As a result in_list never found a match and $SL_PREFIX/bin was always
prepended to PATH, even when already present.

Remove the quotes so the shell word-splits the list into individual
arguments, and suppress the SC2086 shellcheck warning at each call site.

Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>
@knard38 knard38 self-assigned this Jun 10, 2026
@knard38 knard38 marked this pull request as ready for review June 10, 2026 08:53
@knard38 knard38 requested review from a team as code owners June 10, 2026 08:53
@github-actions

github-actions Bot commented Jun 10, 2026

Copy link
Copy Markdown

Ticket title is 'setup_local.sh: in_list PATH check always passes due to missing word-split'
Status is 'In Review'
Labels: 'triaged'
https://daosio.atlassian.net/browse/DAOS-18905

@knard38 knard38 requested review from daltonbohning, jolivier23 and ryon-jensen and removed request for daltonbohning, jolivier23 and ryon-jensen June 10, 2026 08:54
@knard38 knard38 marked this pull request as draft June 10, 2026 09:21
After fixing the in_list callers to pass word-split lists, the helper can
again return 1 for the expected found case.  Callers that source
setup_local.sh under set -e then abort before the following status check.

Use in_list directly in if conditions so the expected found/not-found
statuses do not trip set -e.

Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>
@knard38 knard38 marked this pull request as ready for review June 10, 2026 12:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants