Opened 8 months ago

Closed 5 months ago

#29060 closed defect (implemented)

shellcheck: test-network.sh issues

Reported by: rl1987 Owned by: rl1987
Priority: Medium Milestone: Tor: 0.4.1.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: technical-debt, regression, 041-must
Cc: Actual Points:
Parent ID: Points:
Reviewer: ahf Sponsor:

Description

In test-network.sh line 8:
if [ -d "$CHUTNEY_PATH" -a -x "$TEST_NETWORK" ]; then
                        ^-- SC2166: Prefer [ p ] && [ q ] as [ p -a q ] is not well defined.


In test-network.sh line 19:
myname=$(basename $0)
                  ^-- SC2086: Double quote to prevent globbing and word splitting.


In test-network.sh line 23:
ORIGINAL_ARGS="$@"
              ^-- SC2124: Assigning an array to a string! Assign as array, or use * instead of @ to concatenate.


In test-network.sh line 55:
    if [ -d "$BUILDDIR/src/core/or" -a -d "$BUILDDIR/src/tools" ]; then
                                    ^-- SC2166: Prefer [ p ] && [ q ] as [ p -a q ] is not well defined.


In test-network.sh line 60:
    elif [ -d "$PWD/src/core/or" -a -d "$PWD/src/tools" ]; then
                                 ^-- SC2166: Prefer [ p ] && [ q ] as [ p -a q ] is not well defined.


In test-network.sh line 76:
if [ ! -d "$CHUTNEY_PATH" -o ! -x "$CHUTNEY_PATH/chutney" ]; then
                          ^-- SC2166: Prefer [ p ] || [ q ] as [ p -o q ] is not well defined.


In test-network.sh line 80:
    elif [ -d "$TOR_DIR" -a -d "$TOR_DIR/../chutney" -a \
                         ^-- SC2166: Prefer [ p ] && [ q ] as [ p -a q ] is not well defined.
                                                     ^-- SC2166: Prefer [ p ] && [ q ] as [ p -a q ] is not well defined.


In test-network.sh line 97:
if [ -d "$CHUTNEY_PATH" -a -x "$TEST_NETWORK" ]; then
                        ^-- SC2166: Prefer [ p ] && [ q ] as [ p -a q ] is not well defined.


In test-network.sh line 102:
    exec "$TEST_NETWORK" $ORIGINAL_ARGS
                         ^-- SC2086: Double quote to prevent globbing and word splitting.

Child Tickets

Change History (30)

comment:1 Changed 8 months ago by rl1987

Owner: set to rl1987
Status: newaccepted

comment:2 Changed 8 months ago by rl1987

Status: acceptedneeds_review

comment:3 Changed 8 months ago by nickm

Milestone: Tor: 0.4.1.x-final

comment:4 Changed 7 months ago by dgoulet

Reviewer: teor

comment:5 Changed 7 months ago by dgoulet

Reviewer: teorahf

(Mistake while assigning. These should go to ahf as a Reviewer.)

comment:6 Changed 7 months ago by ahf

Status: needs_reviewneeds_revision

I think these patches looks good with 2 things that we need to figure out. I just asked these questions on IRC as well.

  1. bash isn't always located in /bin/bash so it might be smarter if we use /usr/bin/env bash for the shebang line. On FreeBSD things installed from ports go to /usr/local/ as prefix.
  2. Should we use .bash instead of .sh for bash scripts? I think yes, but I'm not sure what everyone else thinks here.

The second question is a bit more open. Let's fix the first one now and then we can figure out if we should do question 2 as well later :-)

comment:7 in reply to:  6 Changed 7 months ago by rl1987

Replying to ahf:

I think these patches looks good with 2 things that we need to figure out. I just asked these questions on IRC as well.

  1. bash isn't always located in /bin/bash so it might be smarter if we use /usr/bin/env bash for the shebang line. On FreeBSD things installed from ports go to /usr/local/ as prefix.

Did so in 4f9061868b04724bf3eaecddf9b536c189bd34da.

  1. Should we use .bash instead of .sh for bash scripts? I think yes, but I'm not sure what everyone else thinks here.

The second question is a bit more open. Let's fix the first one now and then we can figure out if we should do question 2 as well later :-)

Opened #29488 for this.

comment:8 Changed 7 months ago by rl1987

Status: needs_revisionneeds_review

comment:9 Changed 7 months ago by rl1987

Status: needs_reviewneeds_revision

Will look into making this pass shellcheck without requiring bash here.

comment:10 Changed 7 months ago by rl1987

Status: needs_revisionneeds_review

In c346eff223e94b5fbeb6e751a99393fc5f7dd4b0 I'm trying to walk away from requiring bash by removing the ORIGINAL_ARGS array/variable.

Not sure I'm not breaking something here. Why did we save $@ into a variable in the first place?

comment:11 Changed 7 months ago by ahf

Status: needs_reviewmerge_ready

I cannot see a reason for why we saved $@ - maybe it was to give it a more explicit name?

I think the code looks good now from what I can tell. Moving to merge_ready and then let's get an additional set of eyes on it from the merger :-)

Thanks for doing this work!

comment:12 Changed 7 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

Merged to master!

comment:13 in reply to:  11 Changed 7 months ago by teor

Keywords: regression 041-must added
Resolution: fixed
Status: closedreopened

Replying to rl1987:

In c346eff223e94b5fbeb6e751a99393fc5f7dd4b0 I'm trying to walk away from requiring bash by removing the ORIGINAL_ARGS array/variable.

Not sure I'm not breaking something here. Why did we save $@ into a variable in the first place?

Because test-network.sh uses shift to search the arguments for the chutney path and tor dir. And shift (partially) destroys $@.

Please revert commit c346eff223.

comment:14 Changed 7 months ago by teor

Status: reopenedneeds_revision

comment:15 Changed 7 months ago by teor

What's our policy on reverts?
Do they need a pull request? (Yes, so we know CI works)
Do they need a review? (Not really?)
Who can merge them?

comment:16 Changed 7 months ago by rl1987

Pull request to revert bad commit: https://github.com/torproject/tor/pull/779

However we still don't want to require bash for this script.

We could use POSIX getopts [0] here, but that would require us to downgrade --chutney-path, --tor-path and --quiet to single-character option names. There's also getopt(1), but that seems to be different between Linux and BSD-derived systems. What do you think? Any other ideas?

[0] http://pubs.opengroup.org/onlinepubs/9699919799.2018edition/utilities/getopts.html

comment:17 Changed 7 months ago by nickm

Status: needs_revisionneeds_review

comment:18 in reply to:  16 Changed 6 months ago by teor

Status: needs_reviewmerge_ready

Replying to rl1987:

Pull request to revert bad commit: https://github.com/torproject/tor/pull/779

Looks good to me.

Let's merge soon, we need this script to work for Tor's chutney CI.

However we still don't want to require bash for this script.

We could use POSIX getopts [0] here, but that would require us to downgrade --chutney-path, --tor-path and --quiet to single-character option names.

Breaking compatibility with existing scripts and users is not ok.

There's also getopt(1), but that seems to be different between Linux and BSD-derived systems. What do you think? Any other ideas?

[0] http://pubs.opengroup.org/onlinepubs/9699919799.2018edition/utilities/getopts.html

Does sh support accessing positional arguments by reference?
(for example: ${$i} or the equivalent syntax.)
If it does, we could use a for loop to iterate through the arguments?

comment:19 Changed 6 months ago by teor

Keywords: asn-merge dgoulet-merge nickm-merge added

Can you leave the ticket open after you merge https://github.com/torproject/tor/pull/779 ?

We still have to decide what to do here.

comment:20 Changed 6 months ago by nickm

Status: merge_readyneeds_information

Merged PR 779; putting this back into "new"

comment:21 Changed 6 months ago by nickm

Status: needs_informationnew

comment:22 Changed 6 months ago by asn

Keywords: asn-merge dgoulet-merge nickm-merge removed

Removing merge keywords since the thing got merged by tim.

comment:23 Changed 6 months ago by rl1987

Status: newneeds_review

comment:24 Changed 6 months ago by teor

Thanks for the update.
I like the idea of using awk, rather than messing with shell expansions.
I left one awk question on the pull request.

comment:25 Changed 6 months ago by ahf

Status: needs_reviewneeds_revision

Looks like teor already had a question here :-) This is mix of needs_review and needs_information.

comment:26 Changed 6 months ago by teor

Status: needs_revisionneeds_review

I am happy with the answer to my question.

comment:27 Changed 5 months ago by ahf

Status: needs_reviewmerge_ready

Looks good.

comment:28 Changed 5 months ago by nickm

Teor, will this conflict with any other pending test-network changes? If not, I'm happy to merge this.

comment:29 in reply to:  28 Changed 5 months ago by teor

Replying to nickm:

Teor, will this conflict with any other pending test-network changes? If not, I'm happy to merge this.

We don't have any pending changes in test-network.sh in tor: it's a minimal wrapper that exec's chutney's test-network.sh.
And we only add features to test-network.sh in chutney.

So these changes are fine.
We'll make further changes in #29689 when we copy the initial parts of chutney's test-network.sh into tor's test-network.sh.

comment:30 Changed 5 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

ok; merged to master.

Note: See TracTickets for help on using tickets.