Skip to content

[Merged by Bors] - Small script fix#2591

Closed
ghost wants to merge 1 commit intomainfrom
unknown repository
Closed

[Merged by Bors] - Small script fix#2591
ghost wants to merge 1 commit intomainfrom
unknown repository

Conversation

@ghost
Copy link

@ghost ghost commented Aug 3, 2021

Objective

Prevent some possible problem
Found by IntelliJ IDEA

Solution

Double quoting variable and exit on possible failure of cd command.

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Aug 3, 2021
tools/publish.sh Outdated
)

cd crates
cd crates || exit
Copy link
Contributor

Choose a reason for hiding this comment

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

I think set -ex or something like that should be used.

tools/publish.sh Outdated
)

cd crates
cd crates || set -ex
Copy link
Contributor

Choose a reason for hiding this comment

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

I actually mean set -ex on top of the file and then remove the || exit 1. It should have been set -eou pipefail instead of set -ex it seems by the way. set -e causes the script to exit if any command exists with a non-zero exit code. set -u causes undefined variables to be treated as error. set -o pipefail causes pipelines (|) to exit with a non-zero exit code if any command that is part of it exits with a non-zero exit code.

Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

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

Not an expert in these matters, but none of these changes are very scary.

@@ -3,16 +3,16 @@
duration='2'
function wait_seconds() { perl -e 'alarm shift; exec @ARGV' "$@"; }
Copy link
Member

Choose a reason for hiding this comment

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

This should probably not be calling into perl, and this entire script should instead just be a cargo xtask style command (for cross platform compatibility).

However, that's out of scope for this PR - this is an improvement.

@DJMcNab DJMcNab added A-Meta About the project itself C-Code-Quality A section of code that is hard to understand or change and removed S-Needs-Triage This issue needs to be labelled labels Aug 6, 2021
tools/publish.sh Outdated
bevy_internal
bevy_dylib
)
set -eou pipefail
Copy link
Member

Choose a reason for hiding this comment

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

This means that in case of failure to publish crates, it will stop on the first failure instead of ignoring and continuing for other crates. I don't know the intended behaviour as it's a tool used only by @cart though.

Copy link
Member

Choose a reason for hiding this comment

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

yeah i like the current behavior, lets keep that for now.

@cart
Copy link
Member

cart commented Aug 13, 2021

bors r+

bors bot pushed a commit that referenced this pull request Aug 13, 2021
# Objective
Prevent some possible problem
Found by IntelliJ IDEA

## Solution

Double quoting variable and exit on possible failure of cd command.
@bors bors bot changed the title Small script fix [Merged by Bors] - Small script fix Aug 13, 2021
@bors bors bot closed this Aug 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Meta About the project itself C-Code-Quality A section of code that is hard to understand or change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants