ci: shellcheck checks #697

storopoli · 2024-05-19T09:54:50Z

Following rust-bitcoin/rust-bitcoin#2762,
adding CI shellcheck cheks here as well.

I also did all fixes that I could find with

shellcheck **/*.sh

If I've missed any please let me know.

contrib/_test.sh

tcharding · 2024-05-19T21:24:54Z

CI needs re-writing here to use the new run_task script.

storopoli · 2024-05-20T08:19:19Z

CI needs re-writing here to use the new run_task script.

This is not related to the PR right? Should this be followed in a new issue?

apoelstra · 2024-05-20T14:00:01Z

This is not related to the PR right? Should this be followed in a new issue?

Yes, but if we're tweaking shellscripts here, and we are planning to replace the scripts, it will cause us rebasing trouble later and the benefit is much less.

tcharding · 2024-05-20T20:48:03Z

Sorry I was not very clear. Mine was just a flippant comment and meant to imply "not worth putting too much effort in here since we need to re-write to use the new scripts".

storopoli · 2024-05-20T21:20:51Z

Oh ok so should I move this to a draft and then rebase later?

tcharding · 2024-05-20T22:39:06Z

You can do that, or if CI is all green I'll just ack it and re-base #699 on top (once I have that one working).

storopoli · 2024-05-20T22:51:16Z

Yeah sorry there's a little delay in shellcheck CI notifications since I don't get them because I've never contributed to this repo.
I'll fix'em.

tcharding · 2024-05-22T22:58:30Z

CI fails are unrelated to this PR. One is the cfg thing same as in rust-bitcoin and one is a windows thing that I've hit elsewhere and have not debugged.

storopoli · 2024-08-03T11:10:53Z

@tcharding let me know when I should rebase this?

tcharding · 2024-08-04T23:16:27Z

You can rebase and force push mate. FTR you can always push changes in front of me, I don't mind rebasing. If something is particularly bothering me I'll explicitly ask it not to merge. Thanks for checking!

storopoli · 2024-08-05T12:42:58Z

rebased, I have not yet made a contribution here, hence I need approval to run CI 😞

storopoli · 2024-08-05T17:57:38Z

Ok CI is still broken unrelated to this PR. I'll rebase sometime in the future when we fix that.

apoelstra · 2024-08-05T21:03:37Z

Weird. CI seems to be failing because of a bug in #709 where we updated the MSRV but didn't update some tests .... but CI passed on #709 itself.

apoelstra · 2024-08-05T21:04:31Z

Oh, no, the failure is the fault of this PR. The rand-std and hashes-std features do not exist anymore.

storopoli · 2024-08-06T07:46:02Z

Oh, no, the failure is the fault of this PR. The rand-std and hashes-std features do not exist anymore.

Good catch! Removed. I need new CI authorization...

tcharding

ACK bb3f500

apoelstra · 2024-08-06T20:39:59Z

secp256k1-sys/depend/secp256k1/ci/ci.sh

@@ -80,7 +80,7 @@ esac
    --enable-examples="$EXAMPLES" \
    --enable-ctime-tests="$CTIMETESTS" \
    --with-valgrind="$WITH_VALGRIND" \
-    --host="$HOST" $EXTRAFLAGS
+    --host="$HOST" "$EXTRAFLAGS"


In bb3f500:

If you want to change this file you'll need to PR upstream, or change the vendoring script here to modify it using sed. But you can't edit vendored code inline like this.

FTR I believe $EXTRAFLAGS is unquoted on purpose.

tcharding · 2024-08-06T21:14:49Z

oooo, bad review by me :)

apoelstra · 2024-08-06T22:05:38Z

Heh, I didn't notice it either. But my local CI scripts did.

FWIW we could change our vendoring code to just delete this file. It's not like we use it.

Kixunil · 2024-08-07T04:04:25Z

We should delete the whole ci directory I think.

storopoli · 2024-08-07T20:45:39Z

Ok I removed all changes inside secp256k1-sys and added it to the ignore_paths according to the action-shellcheck README

tcharding

ACK 52ff1c2

.github/workflows/shellcheck.yml

apoelstra

ACK 888c75d successfully ran local tests

apoelstra · 2024-08-07T22:25:20Z

Will let @tcharding take one more look before merging.

tcharding · 2024-08-08T03:32:21Z

ACK b3faab6

Not that I've caught any mistakes in this PR yet with my reviews :)

githooks/pre-commit

Kixunil

ACK 359a89b

storopoli · 2024-08-08T10:10:01Z

I cannot decode the error...

Kixunil

ACK bfdf5dc

Kixunil · 2024-08-08T10:26:39Z

The CI failure looks unrelated, IIRC we don't pin nightly here.

a0febf7 githooks: remove unnecessary shellcheck disables (Jose Storopoli) Pull request description: We don't need to worry about nested quoting in SC2046. Thanks to Kixunil in rust-bitcoin/rust-secp256k1#697 for pointing that out. ACKs for top commit: Kixunil: ACK a0febf7 tcharding: ACK a0febf7 apoelstra: ACK a0febf7 successfully ran local tests Tree-SHA512: 83cdcc55c7e7922c05f5e78305086595a96745f34ea68ee99ed1c08c97683bd45d3c62e8d8b4bcba6b0572c9c65178ff4e21abd7178e2e8b0c9e42f4b25508fb

tcharding · 2024-08-08T23:06:36Z

Maybe just wait for #699 to go in and see if that fixes the CI failure.

storopoli · 2024-09-02T15:32:40Z

contrib/extra_tests.sh

-say_err() {
-    say "$1" >&2
-}
-


Not being used

storopoli · 2024-09-02T15:32:53Z

contrib/sanitizer.sh

-say_err() {
-    say "$1" >&2
-}
-


Not being used

storopoli · 2024-09-02T15:33:14Z

rebased after #699 was merged, cc @tcharding

Kixunil

ACK ae0a304

tcharding

ACK ae0a304

apoelstra

ACK ae0a304 successfully ran local tests

storopoli commented May 19, 2024

View reviewed changes

contrib/_test.sh Outdated Show resolved Hide resolved

storopoli force-pushed the ci/shellcheck branch from ec769d2 to 71ee666 Compare May 20, 2024 08:18

storopoli force-pushed the ci/shellcheck branch from 71ee666 to f26c1c6 Compare May 20, 2024 22:53

storopoli force-pushed the ci/shellcheck branch from f26c1c6 to cf99ecd Compare August 5, 2024 12:42

storopoli force-pushed the ci/shellcheck branch from cf99ecd to bb3f500 Compare August 6, 2024 07:45

tcharding previously approved these changes Aug 6, 2024

View reviewed changes

apoelstra reviewed Aug 6, 2024

View reviewed changes

storopoli dismissed tcharding’s stale review via f3bd705 August 7, 2024 20:43

storopoli force-pushed the ci/shellcheck branch 2 times, most recently from f3bd705 to 52ff1c2 Compare August 7, 2024 20:44

tcharding previously approved these changes Aug 7, 2024

View reviewed changes

.github/workflows/shellcheck.yml Outdated Show resolved Hide resolved

storopoli requested review from apoelstra and tcharding August 7, 2024 22:19

apoelstra previously approved these changes Aug 7, 2024

View reviewed changes

Kixunil reviewed Aug 8, 2024

View reviewed changes

githooks/pre-commit Outdated Show resolved Hide resolved

storopoli dismissed apoelstra’s stale review via 359a89b August 8, 2024 09:01

storopoli force-pushed the ci/shellcheck branch from b3faab6 to 359a89b Compare August 8, 2024 09:01

storopoli mentioned this pull request Aug 8, 2024

githooks: remove unnecessary shellcheck disables rust-bitcoin/rust-bitcoin#3144

Merged

Kixunil previously approved these changes Aug 8, 2024

View reviewed changes

storopoli dismissed Kixunil’s stale review via 6c71cef August 8, 2024 10:02

storopoli force-pushed the ci/shellcheck branch 2 times, most recently from 6c71cef to bfdf5dc Compare August 8, 2024 10:09

Kixunil previously approved these changes Aug 8, 2024

View reviewed changes

storopoli added 2 commits September 2, 2024 12:31

shellcheck: fix warnings

9f9bd34

ci: add shellcheck job

ae0a304

storopoli dismissed Kixunil’s stale review via ae0a304 September 2, 2024 15:32

storopoli force-pushed the ci/shellcheck branch from bfdf5dc to ae0a304 Compare September 2, 2024 15:32

storopoli commented Sep 2, 2024

View reviewed changes

contrib/extra_tests.sh

Comment on lines -45 to -48

say_err() {

say "$1" >&2

}

Copy link

Contributor Author

storopoli Sep 2, 2024

Choose a reason for hiding this comment

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

Not being used

storopoli commented Sep 2, 2024

View reviewed changes

contrib/sanitizer.sh

Comment on lines -51 to -54

say_err() {

say "$1" >&2

}

Copy link

Contributor Author

storopoli Sep 2, 2024

Choose a reason for hiding this comment

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

Not being used

Kixunil approved these changes Sep 2, 2024

View reviewed changes

tcharding approved these changes Sep 2, 2024

View reviewed changes

apoelstra approved these changes Sep 2, 2024

View reviewed changes

apoelstra merged commit 59f122d into rust-bitcoin:master Sep 2, 2024
30 checks passed

storopoli deleted the ci/shellcheck branch September 3, 2024 08:42

ci: shellcheck checks #697

ci: shellcheck checks #697

Conversation

storopoli commented May 19, 2024

tcharding commented May 19, 2024

storopoli commented May 20, 2024

apoelstra commented May 20, 2024

tcharding commented May 20, 2024

storopoli commented May 20, 2024

tcharding commented May 20, 2024

storopoli commented May 20, 2024

tcharding commented May 22, 2024

storopoli commented Aug 3, 2024

tcharding commented Aug 4, 2024

storopoli commented Aug 5, 2024

storopoli commented Aug 5, 2024

apoelstra commented Aug 5, 2024

apoelstra commented Aug 5, 2024

storopoli commented Aug 6, 2024

tcharding left a comment

Choose a reason for hiding this comment

apoelstra Aug 6, 2024

Choose a reason for hiding this comment

Kixunil Aug 7, 2024

Choose a reason for hiding this comment

tcharding commented Aug 6, 2024

apoelstra commented Aug 6, 2024

Kixunil commented Aug 7, 2024

storopoli commented Aug 7, 2024

tcharding left a comment

Choose a reason for hiding this comment

apoelstra left a comment

Choose a reason for hiding this comment

apoelstra commented Aug 7, 2024

tcharding commented Aug 8, 2024

Kixunil left a comment

Choose a reason for hiding this comment

storopoli commented Aug 8, 2024

Kixunil left a comment

Choose a reason for hiding this comment

Kixunil commented Aug 8, 2024

tcharding commented Aug 8, 2024

storopoli Sep 2, 2024

Choose a reason for hiding this comment

storopoli Sep 2, 2024

Choose a reason for hiding this comment

storopoli commented Sep 2, 2024

Kixunil left a comment

Choose a reason for hiding this comment

tcharding left a comment

Choose a reason for hiding this comment

apoelstra left a comment

Choose a reason for hiding this comment