Opened 5 years ago

Closed 5 years ago

#17631 closed defect (fixed)

chutney fails on systems with python2, but no python

Reported by: teor Owned by: nickm
Priority: Medium Milestone:
Component: Core Tor/Chutney Version:
Severity: Normal Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

I forgot brackets when I rewrite the command checks.

Child Tickets

Attachments (1)

0001-Rewrite-Python-detection-in-Chutney-shell-script.patch (2.5 KB) - added by cypherpunks 5 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 5 years ago by teor

Status: newneeds_review

See my branch bug17631-python on https://github.com/teor2345/chutney.git

comment:2 in reply to:  1 Changed 5 years ago by cypherpunks

Status: needs_reviewneeds_information

Replying to teor:

See my branch bug17631-python on https://github.com/teor2345/chutney.git

I'm rewriting the Python detection to use a loop and run through a list of python versions which easier to extend with future Python releases and less error-prone than the current one-liner.

During the rewrite i stumbled upon

# Use python2 if the checks that use "command" fail
${PYTHON:=python2} -m chutney.TorNet "$@

which is at the end of the chutney shell script. I believe this isn't going to work if the previous command checks didn't find python2 already. Isn't it better to display an error message and abort further execution of Chutney?

comment:3 Changed 5 years ago by teor

It's a last-ditch measure in case "command" is broken in the shell.

comment:4 in reply to:  3 ; Changed 5 years ago by cypherpunks

Replying to teor:

It's a last-ditch measure in case "command" is broken in the shell.

I get what it's trying to do but I'm doubting it's effectiveness. Are there shells where command is broken?

comment:5 in reply to:  4 Changed 5 years ago by teor

Status: needs_informationneeds_revision

Replying to cypherpunks:

Replying to teor:

It's a last-ditch measure in case "command" is broken in the shell.

I get what it's trying to do but I'm doubting it's effectiveness. Are there shells where command is broken?

It's hard to say, but historically, different shells used different builtins to implement the functionality of "command".

So any shell that only conforms to Issue 3 of "The Open Group Base Specifications", and any shell that doesn't fully conform to Issues 4 and onwards when implementing "command".

http://pubs.opengroup.org/onlinepubs/9699919799/utilities/command.html

And perhaps there are environments where the configuration breaks "command", but not normal execution. For example, if you delete /usr/bin/command on my machine, it stops working for some reason.

comment:6 Changed 5 years ago by cypherpunks

I have looked into how Autoconf checks for programs in order to avoid the possible issues with command.

Autoconf can check for programs through the AC_CHECK_PROG function (which is implemented in _AC_CHECK_PROG) which uses _AS_PATH_WALK for walking through PATH and AS_EXECUTABLE_P (which is implemented in _AS_TEST_PREPARE) for checking if the file is an executable regular file.

As these functions do not use command i believe this may be the solution for checking for Python versions in a portable fashion. If you agree i will change the Python detection to use this solution.

comment:7 Changed 5 years ago by teor

Sure, that sounds ok.

comment:8 Changed 5 years ago by cypherpunks

Status: needs_revisionneeds_review

Here's a patch for review.

I have removed detection for Python 3 since it was commented out already. It can be added again by adding it the binaries variable once #16904 is fixed.

Additionally i found PEP-0394 which has some recommendations on how to maintain cross-platform support now that Arch Linux links python to python3. I believe that in our case that means we should change the detection order to python2 python3 python once #16904 is fixed.

comment:9 Changed 5 years ago by teor

Thanks, looks good, let's get this merged!

comment:10 Changed 5 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Merged it; thanks!

Note: See TracTickets for help on using tickets.