Skip to content

Use canonical SHELL + .SHELLFLAGS instead of the shorthand - #2230

Draft
pose wants to merge 1 commit into
masterfrom
shell-shellflags-canonical
Draft

Use canonical SHELL + .SHELLFLAGS instead of the shorthand#2230
pose wants to merge 1 commit into
masterfrom
shell-shellflags-canonical

Conversation

@pose

@pose pose commented May 13, 2026

Copy link
Copy Markdown
Member

Summary

  • SHELL := /bin/bash -o pipefail happens to work in practice because GNU Make splits the SHELL variable on whitespace when invoking, but per the GNU Make documentation the canonical form is SHELL := /bin/bash combined with the shell flags in .SHELLFLAGS.

  • Switch the two sites under ci-mgmt's control (the meta-Makefile and the bridged crossbuild.mk template) to:

    SHELL := /bin/bash
    .SHELLFLAGS := -eo pipefail -c
  • The extra -e makes every recipe invocation exit on first error by default. Recipes that already set set -e themselves are unaffected; recipes relying on commands continuing past a failure now have to opt in explicitly with cmd || true.

Test plan

  • cd provider-ci && make all passes locally (Go unit tests, actionlint on regenerated test-providers, no unrelated diffs).
  • Reviewer: confirm regenerated test-providers scripts/crossbuild.mk diffs match the template change (SHELL + .SHELLFLAGS swap, nothing else).

Notes

Opened as draft for initial review. PR-field checklist (assignee, reviewers, milestone, project) will be filled once the diff is acked.

🤖 Generated with Claude Code

`SHELL := /bin/bash -o pipefail` happens to work in practice because
GNU Make splits the SHELL variable on whitespace when invoking, but
per the GNU Make documentation the canonical form is `SHELL := /bin/bash`
combined with the shell flags in `.SHELLFLAGS`. Switch the two sites
under ci-mgmt's control (the meta-Makefile and the bridged crossbuild.mk
template) to the canonical form:

    SHELL := /bin/bash
    .SHELLFLAGS := -eo pipefail -c

The extra `-e` makes every recipe invocation exit on first error by
default; recipes that already set `set -e` themselves are unaffected,
and recipes relying on commands continuing past a failure now have to
opt in explicitly with `cmd || true`. `make all` passes locally with
no other diffs beyond the regenerated test-providers fixtures.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@pose
pose force-pushed the shell-shellflags-canonical branch from 144eacb to 1943428 Compare May 13, 2026 17:31
@pose

pose commented May 13, 2026

Copy link
Copy Markdown
Member Author

Parking this PR pending a lint-recipe fix

While verifying the impact of -e in .SHELLFLAGS on bridged providers, I found a regression: the templated lint and lint.fix recipes in provider-ci/internal/pkg/templates/base/Makefile (lines ~298-310) use a capture-then-cleanup pattern that depends on the shell continuing past a golangci-lint non-zero exit:

cd provider && golangci-lint run …; LINT_EXIT=$$?; \
if git grep -ql 'goembed'; then git grep -l 'goembed' | xargs perl -i -pe 's/ goembed/go:embed/g'; fi; \
exit $$LINT_EXIT

The first recipe line rewrites go:embed to goembed (with a leading space) to hide it from golangci-lint. The continuation captures golangci-lint's exit, runs the inverse rewrite as cleanup, and then exits with the captured code. With -e in .SHELLFLAGS, a lint failure aborts the shell at the golangci-lint call, so the cleanup is skipped and the working tree is left with goembed strings that don't compile until manually restored.

CI is unaffected (lint failure still aborts the workflow on an ephemeral runner), but local-dev experience after a lint failure is broken.

Path forward

  1. Land pulumi/ci-mgmt#2229 first to unblock the immediate [[-silent-failure concern.
  2. Rewrite the lint / lint.fix recipes in base/Makefile to use a trap '... cleanup ...' EXIT pattern that survives -e.
  3. Re-validate this PR (pulumi/ci-mgmt#2230) on top of the trap-based cleanup, then mark ready for review.

Leaving this PR as a draft until step 2 lands.

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.

1 participant