Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#10933 closed enhancement (implemented)

support for setting binary paths via environment varaibles

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

Description

My branch:
https://github.com/houqp/chutney/tree/env_bin

This makes it easier to switch between different builds for testing purpose.

Child Tickets

Change History (8)

comment:1 Changed 5 years ago by nickm

Status: newneeds_revision

Possible bug: If I have 'tor' and 'tor-gencert' in my path, and I don't set the environment variables, won't this fail?

That is, when you call this code:

+        tor_path = self._env['tor']
+        if not os.path.exists(tor_path):
+            print "Cannot find Tor binary: {0}".format(tor_path)
+            return False

I think that tor_path will be the string "tor", and "os.path.exists(tor_path)" will check whether there is a file named "tor" in the cwd, but it won't check for whether there is a file named "tor" in your path.

One option is to remove those checks, and to allow subprocess.Popen to fail instead. That won't give such a nice error message, though.

comment:2 Changed 5 years ago by dave2008

Oh, good catch. How about remove the check and catch the error from subprocess?

comment:3 Changed 5 years ago by nickm

Better than nothing! Let's try to distinguish that particular error ( os.OSError, errno=ENOENT) from the others. And when we catch it, let's make sure to tell the user what path we were looking for, and how to set the location of tor or tor-gencert.

comment:4 Changed 5 years ago by dave2008

hi nick,

sorry the the late reply, was out for travel :)

I have updated my branch accordingly. There is duplicate code mainly because subprocess is called with different arguments. maybe we should clean up that a little bit and make a wrapper for code reuse on the try/catch code?

Also should we switch to argparse module as well?

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

Resolution: implemented
Status: needs_revisionclosed

Replying to dave2008:

hi nick,

sorry the the late reply, was out for travel :)

I have updated my branch accordingly.

There is duplicate code mainly because subprocess is called with different arguments. maybe we should clean up that a little bit and make a wrapper for code reuse on the try/catch code?

Hm. It's a little irksome, but I think it's not actually the end of the world here. I'd take a patch to remove the redundancy if you want to write one.

In any case, this looks fine, so I merged it.

BTW, I also changed errno.errorcode[e.errno] == 'ENOENT' to e.errno == errno.ENOENT`, and made the log messages include the name of the binary that we tried to open.

Also should we switch to argparse module as well?

The documentation says that module is new in Python 3.2. I think we still have Chutney working with python 2.

comment:6 Changed 5 years ago by dave2008

Argparse is included as standard lib in Python start from 2.7. So if we want to support 2.6, then we might want to use optparse. But I think it's reasonable to assume that most Chuntney users will have at least Python 2.7 and for those who are still using legacy 2.6, they can pip install argparse.

Any comment?

comment:7 Changed 5 years ago by nickm

Let's revisit that once debian squeeze hits end-of-life (in a few months); I believe it shipped Python 2.6 and Python 3.1, and Debian "oldstable" is usually a good guideline for what it's safe to insist on. Want to make a ticket to remind us of that?

(Some people try to only insist on what's in RHEL/CentOS , but that seems unreasonable for a project like Chutney).

comment:8 Changed 5 years ago by dave2008

OK, done, see #11115.

Note: See TracTickets for help on using tickets.