Opened 2 months ago

Last modified 7 days ago

#29060 new defect

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 (22)

comment:1 Changed 6 weeks ago by rl1987

Owner: set to rl1987
Status: newaccepted

comment:2 Changed 6 weeks ago by rl1987

Status: acceptedneeds_review

comment:3 Changed 6 weeks ago by nickm

Milestone: Tor: 0.4.1.x-final

comment:4 Changed 5 weeks ago by dgoulet

Reviewer: teor

comment:5 Changed 5 weeks ago by dgoulet

Reviewer: teorahf

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

comment:6 Changed 5 weeks 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 5 weeks 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 5 weeks ago by rl1987

Status: needs_revisionneeds_review

comment:9 Changed 4 weeks ago by rl1987

Status: needs_reviewneeds_revision

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

comment:10 Changed 4 weeks 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 2 weeks 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 2 weeks ago by nickm

Resolution: fixed
Status: merge_readyclosed

Merged to master!

comment:13 in reply to:  11 Changed 11 days 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 11 days ago by teor

Status: reopenedneeds_revision

comment:15 Changed 11 days 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 9 days 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 9 days ago by nickm

Status: needs_revisionneeds_review

comment:18 in reply to:  16 Changed 9 days 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 9 days 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 7 days ago by nickm

Status: merge_readyneeds_information

Merged PR 779; putting this back into "new"

comment:21 Changed 7 days ago by nickm

Status: needs_informationnew

comment:22 Changed 7 days ago by asn

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

Removing merge keywords since the thing got merged by tim.

Note: See TracTickets for help on using tickets.