Skip to content

[WIP] Fix position of set -e directive in install_cargo_bundle script#14

Closed
BunsDev with Copilot wants to merge 1 commit into
mainfrom
copilot/fix-set-e-directive-position
Closed

[WIP] Fix position of set -e directive in install_cargo_bundle script#14
BunsDev with Copilot wants to merge 1 commit into
mainfrom
copilot/fix-set-e-directive-position

Conversation

Copilot AI commented May 14, 2026

Copy link
Copy Markdown

Thanks for asking me to work on this. I will get started on it and keep this PR's description up to date as I form a plan and make progress.

Original prompt
Please apply the following diffs and create a pull request.
Once the PR is ready, give it a title based on the messages of the fixes being applied.

[{"message":"The 'set -e' directive is placed inside the conditional block, which means the script will only fail fast when installing cargo-bundle but not when checking for its existence. Consider moving 'set -e' to the top of the script (after the shebang and comments) to ensure consistent error handling throughout the entire script.","fixFiles":[{"filePath":"script/install_cargo_bundle","diff":"diff --git a/script/install_cargo_bundle b/script/install_cargo_bundle\n--- a/script/install_cargo_bundle\n+++ b/script/install_cargo_bundle\n@@ -4,11 +4,12 @@\n \n # This script should be platform-agnostic (eg. no unix-only references like /dev/null).\n \n+set -e\n+\n cargo_location=$(dirname \"$(which cargo)\")\n if test -f \"$cargo_location/cargo-bundle\"; then\n     echo \"cargo-bundle already exists, not installing.\"\n else\n-    set -e\n     cargo install cargo-bundle --git=https://github.com/burtonageo/cargo-bundle --rev ae4c76e92c08774bf54ff077b1c52e3d1cd6c16d\n     echo \"Successfully installed cargo-bundle to location $cargo_location\"\n fi\n"}]},{"message":"The git revision hash is hardcoded without explanation. Consider adding a comment explaining why this specific commit is required, or document the rationale for pinning to this revision to aid future maintenance.","fixFiles":[{"filePath":"script/install_cargo_bundle","diff":"diff --git a/script/install_cargo_bundle b/script/install_cargo_bundle\n--- a/script/install_cargo_bundle\n+++ b/script/install_cargo_bundle\n@@ -9,6 +9,8 @@\n     echo \"cargo-bundle already exists, not installing.\"\n else\n     set -e\n+    # Intentionally pin to a specific commit for deterministic CI installs.\n+    # This avoids unexpected behavior changes from newer upstream revisions.\n     cargo install cargo-bundle --git=https://github.com/burtonageo/cargo-bundle --rev ae4c76e92c08774bf54ff077b1c52e3d1cd6c16d\n     echo \"Successfully installed cargo-bundle to location $cargo_location\"\n fi\n"}]}]

@BunsDev

BunsDev commented May 15, 2026

Copy link
Copy Markdown
Member

Superseded by #15 which implements all four fixes.

@BunsDev BunsDev closed this May 15, 2026
BunsDev added a commit that referenced this pull request May 15, 2026
…et -e, remove deprecated ::echo:: commands (#15)

- bundle.ps1: remove premature $BUNDLE_ID assignment using undefined $app_name (fixes #11)
- install_build_deps: guard /etc/os-release source with file existence check (fixes #13)
- install_cargo_bundle: move set -e to top of script so all failures are caught (fixes #14)
- Grammatical fix in wasm/bundle comment (fixes #12)
- Remove deprecated ::echo::on/off GitHub Actions commands from all bundle scripts and workflows (fixes #12)
@BunsDev
BunsDev deleted the copilot/fix-set-e-directive-position branch June 15, 2026 06:26
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