Skip to content

GHA: add shellcheck job and fix warnings, shell tidy-ups - #13307

Closed
vszakats wants to merge 28 commits into
curl:masterfrom
vszakats:ci-shellcheck
Closed

GHA: add shellcheck job and fix warnings, shell tidy-ups#13307
vszakats wants to merge 28 commits into
curl:masterfrom
vszakats:ci-shellcheck

Conversation

@vszakats

@vszakats vszakats commented Apr 7, 2024

Copy link
Copy Markdown
Member

@vszakats vszakats added build tidy-up CI Continuous Integration labels Apr 7, 2024
@github-actions github-actions Bot added the tests label Apr 7, 2024
@vszakats vszakats changed the title GHA: add shellcheck job and fix fallouts, tidy-up GHA: add shellcheck job and fix warnings, tidy-up Apr 7, 2024
Comment thread scripts/contributors.sh Outdated
@vszakats vszakats changed the title GHA: add shellcheck job and fix warnings, tidy-up GHA: add shellcheck job and fix warnings, tidy-ups Apr 8, 2024
@vszakats vszakats changed the title GHA: add shellcheck job and fix warnings, tidy-ups GHA: add shellcheck job and fix warnings, shell tidy-ups Apr 8, 2024
@vszakats vszakats closed this in fa69b41 Apr 8, 2024
@vszakats
vszakats deleted the ci-shellcheck branch April 8, 2024 09:38
vszakats added a commit that referenced this pull request Apr 11, 2024
- use `$()` instead of backticks, and re-arrange double-quotes inside.
- add missing `|| exit 1` to `cd` calls. (could be dropped by using `set -eu`.)
- add `-n` to a few `if`s.
- shorten redirections by using `{} >` (as shellcheck recommended).
- silence warnings where variables were detected as unused (SC2034).
- a couple misc updates to silence warnings.
- switch to bash shebang for `-ot` feature.
- split two lines to unbreak syntax highlighting in my editor. (`$(expr \`, `$(dirname \`)

Also enable CI checks for OS/400 shell scripts.

Ref: #13307
Closes #13309
@asedeno

asedeno commented Jun 4, 2024

Copy link
Copy Markdown
Contributor

While I do perfer the newer command substitution syntax, the old syntax works in more places. I'm going to work around this locally by switching curl-config to use #!/bin/bash on the ancient (SunOS 5.10) system where I care, and am mostly leaving this comment so folks are aware that this does break some folks.

@vszakats

vszakats commented Jun 4, 2024

Copy link
Copy Markdown
Member Author

I think we can revert to the backticks for the few affected lines for maximum compatibility.

PR: #13871

vszakats added a commit to vszakats/curl that referenced this pull request Jun 4, 2024
Make an exception for `curl-config` because this script that may be
running on any target system, including old ones, e.g. SunOS 5.10.

Reported-by: Alejandro R. Sedeño
Ref: curl#13307 (comment)
Follow-up to fa69b41 curl#13307
Closes curl#13871
vszakats added a commit that referenced this pull request Jun 4, 2024
Make an exception for `curl-config` because this script that may be
running on any target system, including old ones, e.g. SunOS 5.10.

Reported-by: Alejandro R. Sedeño
Ref: #13307 (comment)
Follow-up to fa69b41 #13307
Closes #13871
vszakats added a commit to vszakats/curl that referenced this pull request Oct 2, 2024
Fixes intermittend CI failure because the shell-based random generator
generated the same number twice:
```
$ openssl ca -config EdelCurlRoot-ca.cnf -revoke Server-localhost0h-sv.crt
Using configuration from EdelCurlRoot-ca.cnf
ERROR:Already revoked, serial number 66FDB23A
make: *** [../../../tests/certs/Server-localhost0h-sv.pem] Error 1
```
https://github.com/curl/curl/actions/runs/11151401083/job/30994755798?pr=15128#step:10:498

Follow-up to fa461b4 curl#14486
Follow-up to fa69b41 curl#13307
vszakats added a commit that referenced this pull request Oct 2, 2024
Generate the certificate serial numbers automatically instead of doing
from shell (or Perl earlier).

Fixes intermittent CI failures due to the shell-based random generator
generating the same serial number twice:
```
$ openssl ca -config EdelCurlRoot-ca.cnf -revoke Server-localhost0h-sv.crt
Using configuration from EdelCurlRoot-ca.cnf
ERROR:Already revoked, serial number 66FDB23A
make: *** [../../../tests/certs/Server-localhost0h-sv.pem] Error 1
```
https://github.com/curl/curl/actions/runs/11151401083/job/30994755798?pr=15128#step:10:498

Follow-up to fa461b4 #14486
Follow-up to fa69b41 #13307
Closes #15129
pps83 pushed a commit to pps83/curl that referenced this pull request Apr 26, 2025
Generate the certificate serial numbers automatically instead of doing
from shell (or Perl earlier).

Fixes intermittent CI failures due to the shell-based random generator
generating the same serial number twice:
```
$ openssl ca -config EdelCurlRoot-ca.cnf -revoke Server-localhost0h-sv.crt
Using configuration from EdelCurlRoot-ca.cnf
ERROR:Already revoked, serial number 66FDB23A
make: *** [../../../tests/certs/Server-localhost0h-sv.pem] Error 1
```
https://github.com/curl/curl/actions/runs/11151401083/job/30994755798?pr=15128#step:10:498

Follow-up to fa461b4 curl#14486
Follow-up to fa69b41 curl#13307
Closes curl#15129
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

3 participants