Clean up scripts with shellcheck #77376

pta2002 · 2020-09-30T21:06:37Z

Should take care of #77290, but since there's a good chance I'll do a second pass through the scripts, I'm leaving this here to check with CI.

rust-highfive · 2020-09-30T21:06:40Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

Mark-Simulacrum · 2020-09-30T21:17:06Z

I'm going to r? @pietroalbini in the hopes that you have time to take a look at this, I personally do not feel like I can dedicate bandwidth to being comfortable approving this

pta2002 · 2020-09-30T21:48:17Z

That should be all changes necessary, as far as I know.

src/ci/docker/host-x86_64/disabled/dist-x86_64-dragonfly/build-toolchain.sh

jyn514 · 2020-10-01T02:21:34Z

src/ci/docker/host-x86_64/disabled/dist-x86_64-haiku/build-toolchain.sh

@@ -31,35 +31,35 @@ exit 1
 # First up, build a cross-compiler
 git clone --depth=1 https://git.haiku-os.org/haiku
 git clone --depth=1 https://git.haiku-os.org/buildtools
-cd $BUILDTOOLS/jam
+cd "$BUILDTOOLS/jam"


Hmm, don't you also need to change lines 7 and 9 for this to do anything?

Hi @pta2002, you marked this as resolved but didn't change the lines. If they don't need to be changed, can you explain why?

src/ci/docker/host-x86_64/disabled/dist-x86_64-haiku/build-toolchain.sh

src/ci/docker/host-x86_64/dist-riscv64-linux/crosstool-ng.sh

src/test/run-make/thumb-none-qemu/script.sh

src/tools/clippy/.github/driver.sh

jyn514 · 2020-10-01T02:42:34Z

src/tools/clippy/.github/driver.sh

@@ -28,14 +28,14 @@ diff normalized.stderr tests/ui/cstring.stderr


 # make sure "clippy-driver --rustc --arg" and "rustc --arg" behave the same
-SYSROOT=`rustc --print sysroot`
+SYSROOT=$(rustc --print sysroot)
 diff <(LD_LIBRARY_PATH=${SYSROOT}/lib ./target/debug/clippy-driver --rustc --version --verbose) <(rustc --version --verbose)


Suggested change

diff <(LD_LIBRARY_PATH=${SYSROOT}/lib ./target/debug/clippy-driver --rustc --version --verbose) <(rustc --version --verbose)

diff <(LD_LIBRARY_PATH="${SYSROOT}"/lib ./target/debug/clippy-driver --rustc --version --verbose) <(rustc --version --verbose)

jyn514 · 2020-10-01T02:42:45Z

src/tools/clippy/.github/driver.sh

 diff <(LD_LIBRARY_PATH=${SYSROOT}/lib ./target/debug/clippy-driver --rustc --version --verbose) <(rustc --version --verbose)


 echo "fn main() {}" > target/driver_test.rs
 # we can't run 2 rustcs on the same file at the same time
-CLIPPY=`LD_LIBRARY_PATH=${SYSROOT}/lib ./target/debug/clippy-driver ./target/driver_test.rs --rustc`
-RUSTC=`rustc ./target/driver_test.rs`
+CLIPPY=$(LD_LIBRARY_PATH=${SYSROOT}/lib ./target/debug/clippy-driver ./target/driver_test.rs --rustc)


Suggested change

CLIPPY=$(LD_LIBRARY_PATH=${SYSROOT}/lib ./target/debug/clippy-driver ./target/driver_test.rs --rustc)

CLIPPY=$(LD_LIBRARY_PATH="${SYSROOT}"/lib ./target/debug/clippy-driver ./target/driver_test.rs --rustc)

jyn514 · 2020-10-01T02:43:32Z

src/tools/clippy/.github/driver.sh

-CLIPPY=`LD_LIBRARY_PATH=${SYSROOT}/lib ./target/debug/clippy-driver ./target/driver_test.rs --rustc`
-RUSTC=`rustc ./target/driver_test.rs`
+CLIPPY=$(LD_LIBRARY_PATH=${SYSROOT}/lib ./target/debug/clippy-driver ./target/driver_test.rs --rustc)
+RUSTC=$(rustc ./target/driver_test.rs)
 diff <($CLIPPY) <($RUSTC)


This ... does not look right. Also this is a change to submodule so I'm not sure you should make it here, but clippy uses subtree so it might be special.

Is there any difference between `` and $()? I changed it because shellcheck warned about it and as far as I know they do the same thing.

Oh no your change is fine, I mean running eval on dynamically generated code. Normally I'd suggest to add quotes around the $(...) but since I don't understand this code I'm afraid to change it.

jyn514 · 2020-10-01T02:44:58Z

Updating submodule src/doc/nomicon
error: could not lock config file .git/config: Read-only file system
error: could not lock config file .git/config: Read-only file system
fatal: Failed to register url for submodule path 'src/doc/nomicon'

I think this might be because you made the PR from the master branch? Not sure what's going on here.

jyn514 · 2020-10-01T02:49:29Z

r? @jyn514

I'm positive that pietro doesn't have time ;)

sekineh

I added suggestions.

src/test/run-make/thumb-none-qemu/script.sh

src/test/run-make/x86_64-fortanix-unknown-sgx-lvi/script.sh

src/test/run-make/thumb-none-qemu/script.sh

pta2002 · 2020-10-01T09:05:47Z

Updating submodule src/doc/nomicon
error: could not lock config file .git/config: Read-only file system
error: could not lock config file .git/config: Read-only file system
fatal: Failed to register url for submodule path 'src/doc/nomicon'

I think this might be because you made the PR from the master branch? Not sure what's going on here.

No idea either, the only thing I added to that file was #!/usr/bin/env bash, don't know how it could've made it fail.

pta2002 · 2020-10-01T09:16:23Z

I'll get to work on these suggestions once I get home by the way, can probably finish this today still

src/ci/docker/scripts/android-start-emulator.sh

src/ci/init_repo.sh

src/test/run-make/git_clone_sha1.sh

pta2002 · 2020-10-02T14:10:17Z

Alright, done - I still have no idea why CI is failing with .git being read only

src/ci/init_repo.sh

Co-authored-by: Joshua Nelson <joshua@yottadb.com>

jyn514 · 2020-10-02T14:17:22Z

error: could not lock config file .git/config: Read-only file system
error: could not lock config file .git/config: Read-only file system

Hmm, not sure what would cause this, let me take another look at the changes.

jyn514 · 2020-10-02T14:18:28Z

src/ci/docker/host-x86_64/disabled/dist-x86_64-haiku/build-toolchain.sh

@@ -31,35 +31,35 @@ exit 1
 # First up, build a cross-compiler
 git clone --depth=1 https://git.haiku-os.org/haiku
 git clone --depth=1 https://git.haiku-os.org/buildtools
-cd $BUILDTOOLS/jam
+cd "$BUILDTOOLS/jam"


Hi @pta2002, you marked this as resolved but didn't change the lines. If they don't need to be changed, can you explain why?

src/ci/docker/host-x86_64/disabled/dist-x86_64-dragonfly/build-toolchain.sh

src/ci/docker/run.sh

src/ci/docker/scripts/musl.sh

jyn514 · 2020-10-02T14:23:12Z

src/ci/init_repo.sh

-modules="$(git config --file .gitmodules --get-regexp '\.path$' | cut -d' ' -f2)"
-modules=($modules)
+read -ra modules <<< "$(git config --file .gitmodules --get-regexp '\.path$' | cut -d' ' -f2)"
 use_git=""
-urls="$(git config --file .gitmodules --get-regexp '\.url$' | cut -d' ' -f2)"
-urls=($urls)
-for i in ${!modules[@]}; do
-    module=${modules[$i]}
+read -ra urls <<< "$(git config --file .gitmodules --get-regexp '\.url$' | cut -d' ' -f2)"
+
+for i in "${!modules[@]}"; do


If I had to guess, this is what's causing CI to fail. I would revert the changes for now I think.

pta2002 · 2020-10-02T14:38:32Z

Let's see if this works

pta2002 · 2020-10-02T14:49:25Z

Seems like now it's failing due to not finding a cached build

pta2002 · 2020-10-02T16:16:59Z

Hm from what I can tell, somewhere a .tar.gz got dropped in that URL, but where I have no idea, because I didn't remove any in my changes.

jyn514 · 2020-10-03T03:58:12Z

src/ci/run.sh

  CARGO_INCREMENTAL=0 $PYTHON ../x.py check
  rm -f config.toml
  rm -rf build
 fi

-$SRC/configure $RUST_CONFIGURE_ARGS
+"$SRC"/configure "$RUST_CONFIGURE_ARGS"


This doesn't look correct. This used to be many different arguments, like --set x --set y; now it's one argument all at once. I think you should change it back to how it was before. If you want to silence the shellcheck warning you could trying using an array instead but I'd revert it for now just to make sure it works.

pta2002 · 2020-10-03T08:54:58Z

Oh you're right, I didn't catch that! I'll revert it. A sábado, 3/10/2020, 04:58, Joshua Nelson <notifications@github.com> escreveu:

…

***@***.**** commented on this pull request. ------------------------------ In src/ci/run.sh <#77376 (comment)>: > CARGO_INCREMENTAL=0 $PYTHON ../x.py check rm -f config.toml rm -rf build fi -$SRC/configure $RUST_CONFIGURE_ARGS +"$SRC"/configure "$RUST_CONFIGURE_ARGS" This doesn't look correct. This used to be many different arguments, like --set x --set y; now it's one argument all at once. I think you should change it back to how it was before. If you want to silence the shellcheck warning you could trying using an array instead but I'd revert it for now just to make sure it works. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#77376 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABYZLTEWI2WSNTXLD74U4IDSI2OOBANCNFSM4R7SWJQQ> .

pta2002 · 2020-10-03T10:06:59Z

Well that took a while, looks like it's fixed now though, at least CI has gone off to actually compile rust.

jyn514 · 2020-10-03T13:14:06Z

I personally do not feel like I can dedicate bandwidth to being comfortable approving this

@Mark-Simulacrum I'm reasonably happy with the current code, but I wouldn't stake my life on there being no regressions. How do you want to move forward?

jyn514 · 2020-10-03T13:15:49Z

oh and while we're at it, let's try and catch more issues early

@bors try

bors · 2020-10-03T13:16:01Z

⌛ Trying commit c640a44 with merge a8d42c75c152f5994caaffca294da2b534eade44...

Mark-Simulacrum · 2020-10-03T13:44:55Z

I don't have the time/bandwidth to invest in figuring out the shell script changes here, it's very error prone (as you have already seen) and I have no good ideas on avoiding regressions. IMO, it is questionable whether the improvements in this PR are worth it given the possibility of regressions and likelihood of reintroducing shellcheck-warned code over time.

Largely I just don't know that we get value out of this that outweighs the downsides.

That said, we have already branched beta, so it's as early in the cycle as it's going to get.

jyn514 · 2020-10-03T13:49:16Z

src/etc/cat-and-grep.sh

@@ -59,7 +58,7 @@ if command -v "g${GREPPER}"; then
 fi

 LOG=$(mktemp -t cgrep.XXXXXX)
-trap "rm -f $LOG" EXIT
+trap 'rm -f $LOG' EXIT


Please revert this change in behavior.

Mark-Simulacrum · 2020-10-03T13:51:03Z

src/tools/clippy/.github/driver.sh

-CLIPPY=`LD_LIBRARY_PATH=${SYSROOT}/lib ./target/debug/clippy-driver ./target/driver_test.rs --rustc`
-RUSTC=`rustc ./target/driver_test.rs`
+CLIPPY=$(LD_LIBRARY_PATH=${SYSROOT}/lib ./target/debug/clippy-driver ./target/driver_test.rs --rustc)
+RUSTC=$(rustc ./target/driver_test.rs)
 diff <($CLIPPY) <($RUSTC)

 # TODO: CLIPPY_CONF_DIR / CARGO_MANIFEST_DIR


I would suggest that clippy CI changes are not done in this PR, as you really can't test them without running Clippy's CI. It should be fairly easily to port these to a Clippy PR though.

That makes sense, I'll take these off and submit them to clippy separately

Dylan-DPC-zz · 2020-10-23T11:58:26Z

Closing this as it doesn't look likely it will be approved in this state. If you have a better way of handling this, you can submit a new pull request. Thanks

clean up scripts with shellcheck

79bf1c8

rust-highfive assigned Mark-Simulacrum Sep 30, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 30, 2020

rust-highfive assigned pietroalbini and unassigned Mark-Simulacrum Sep 30, 2020

Clean up more shellcheck warnings

67b73b3

pta2002 changed the title ~~[WIP] Clean up scripts with shellcheck~~ Clean up scripts with shellcheck Sep 30, 2020

jyn514 added the T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. label Oct 1, 2020

jyn514 reviewed Oct 1, 2020

View reviewed changes

jyn514 added the C-cleanup Category: PRs that clean code up or issues documenting cleanup. label Oct 1, 2020

rust-highfive assigned jyn514 and unassigned pietroalbini Oct 1, 2020

sekineh reviewed Oct 1, 2020

View reviewed changes

src/test/run-make/thumb-none-qemu/script.sh Outdated Show resolved Hide resolved

src/test/run-make/x86_64-fortanix-unknown-sgx-lvi/script.sh Outdated Show resolved Hide resolved

src/test/run-make/thumb-none-qemu/script.sh Outdated Show resolved Hide resolved

pta2002 added 2 commits October 2, 2020 14:46

Implement suggestions

4701ac9

Rollback syntax error

439c920

jyn514 reviewed Oct 2, 2020

View reviewed changes

src/ci/docker/scripts/android-start-emulator.sh Outdated Show resolved Hide resolved

src/ci/init_repo.sh Outdated Show resolved Hide resolved

src/test/run-make/git_clone_sha1.sh Outdated Show resolved Hide resolved

Fix suggestions

4755341

jyn514 reviewed Oct 2, 2020

View reviewed changes

src/ci/init_repo.sh Outdated Show resolved Hide resolved

Remove extra "!"

ab7bea4

Co-authored-by: Joshua Nelson <joshua@yottadb.com>

jyn514 reviewed Oct 2, 2020

View reviewed changes

Roll-back some changes

fe2dc40

Merge branch 'master' of github.com:pta2002/rust

3216a6f

jyn514 reviewed Oct 3, 2020

View reviewed changes

Fix rust_configure_args

c640a44

jyn514 reviewed Oct 3, 2020

View reviewed changes

Mark-Simulacrum reviewed Oct 3, 2020

View reviewed changes

pta2002 added 2 commits October 3, 2020 15:07

Revert cat-and-grep.sh

f1605c0

Revert clippy CI changes

540dc94

camelid added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 23, 2020

Dylan-DPC-zz closed this Oct 23, 2020

Dylan-DPC-zz added S-inactive S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. S-inactive labels Oct 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up scripts with shellcheck #77376

Clean up scripts with shellcheck #77376

pta2002 commented Sep 30, 2020

rust-highfive commented Sep 30, 2020

Mark-Simulacrum commented Sep 30, 2020

pta2002 commented Sep 30, 2020

jyn514 Oct 1, 2020

jyn514 Oct 2, 2020

jyn514 Oct 1, 2020

jyn514 Oct 1, 2020

jyn514 Oct 1, 2020

pta2002 Oct 1, 2020

jyn514 Oct 1, 2020

jyn514 commented Oct 1, 2020

jyn514 commented Oct 1, 2020

sekineh left a comment

pta2002 commented Oct 1, 2020

pta2002 commented Oct 1, 2020

pta2002 commented Oct 2, 2020

jyn514 commented Oct 2, 2020

jyn514 Oct 2, 2020

jyn514 Oct 2, 2020

pta2002 commented Oct 2, 2020

pta2002 commented Oct 2, 2020

pta2002 commented Oct 2, 2020

jyn514 Oct 3, 2020

pta2002 commented Oct 3, 2020 via email

pta2002 commented Oct 3, 2020

jyn514 commented Oct 3, 2020

jyn514 commented Oct 3, 2020

bors commented Oct 3, 2020

Mark-Simulacrum commented Oct 3, 2020

jyn514 Oct 3, 2020

Mark-Simulacrum Oct 3, 2020

pta2002 Oct 3, 2020

Dylan-DPC-zz commented Oct 23, 2020

	diff <(LD_LIBRARY_PATH=${SYSROOT}/lib ./target/debug/clippy-driver --rustc --version --verbose) <(rustc --version --verbose)
	diff <(LD_LIBRARY_PATH="${SYSROOT}"/lib ./target/debug/clippy-driver --rustc --version --verbose) <(rustc --version --verbose)

	CLIPPY=$(LD_LIBRARY_PATH=${SYSROOT}/lib ./target/debug/clippy-driver ./target/driver_test.rs --rustc)
	CLIPPY=$(LD_LIBRARY_PATH="${SYSROOT}"/lib ./target/debug/clippy-driver ./target/driver_test.rs --rustc)

Clean up scripts with shellcheck #77376

Clean up scripts with shellcheck #77376

Conversation

pta2002 commented Sep 30, 2020

rust-highfive commented Sep 30, 2020

Mark-Simulacrum commented Sep 30, 2020

pta2002 commented Sep 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyn514 commented Oct 1, 2020

jyn514 commented Oct 1, 2020

sekineh left a comment

Choose a reason for hiding this comment

pta2002 commented Oct 1, 2020

pta2002 commented Oct 1, 2020

pta2002 commented Oct 2, 2020

jyn514 commented Oct 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pta2002 commented Oct 2, 2020

pta2002 commented Oct 2, 2020

pta2002 commented Oct 2, 2020

Choose a reason for hiding this comment

pta2002 commented Oct 3, 2020 via email

pta2002 commented Oct 3, 2020

jyn514 commented Oct 3, 2020

jyn514 commented Oct 3, 2020

bors commented Oct 3, 2020

Mark-Simulacrum commented Oct 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dylan-DPC-zz commented Oct 23, 2020