Re: [Gnewsense-dev] Debderive

[Sam Geeraerts]

Stayvoid wrote:
> I wrote another patch:
>
> --- debderive.py
> +++ debderive_patch2.py
> @@ -477,9 +477,17 @@
>
>
>  if __name__ == "__main__":
> -    # Check for reprepro
> -    if not os.path.exists('/usr/bin/reprepro'):
> -        raise IOError, 'Missing dependency: install reprepro and try again.'
> +    try:
> +        open('/usr/bin/reprepro')
> +    except IOError:
> +        sys.stderr.write('Missing dependency: install reprepro and
> try again.\n')
> +#        raise

Better.

> +    try:
> +        open('/etc/debderiver/debderiver.yaml')
> +    except IOError:
> +        sys.stderr.write('Copy `debderiver.yaml\' to
> `/etc/debderiver\' and edit it to suit your needs.\n')
> +#        raise
>
>      CONF_DIR_PATH = '/etc/debderiver'

You should take advantage of CONF_DIR_PATH instead of hard coding the 
same string multiple times.

> Which one should I use: "sys.exit(1)" or "raise"? I've read some
> discussions, but it's unclear to me.
> I'd used "raise" because it provides more information. I'm not sure
> that it's necessary to stop the script this way. That's why I
> commented it out.
> Maybe we should uncomment it if those checks are "critical."

I understand why you'd want to use raise. However, Python's default 
exception handler (i.e. printing the stack) is an automated best effort 
approach that exposes the innards of the program. A user is usually not 
interested in that and just wants a short and clean error message she 
can act on. Therefore, a message + sys.exit is the better solution.

The script can't run without reprepro or debderiver.yaml, so they're 
indeed critical.

> I used "sys.stderr.write" because (AFAIK) it's compatible with the
> third version of Python. Maybe we should replace every "print" with it
> ("print" statements are only used for the error handling). What do you
> think?

Python 2.x is still the default in Parkes, so I've been neglecting 
taking 3.x into account. I welcome a patch for your idea.

> What is the name of the program: "debderive" or "debderiver"?
> You use different names for the directory and for the script.
> Is there a reason for this or it's just a typo?

I thought to name the tool "Debderiver" (as in: it derives a distro from 
Debian (or Debian based distro)) and make the command name an 
imperative. If that doesn't make sense, we can change it. I'm not 
particularly attached to that choice (or the name in general for that 
matter).

> > I'm not a python buff, but isn't The Done Thing to use try blah:, except
> > IOError?
> What about this?
>
> try:
>   open('/usr/local/bin/debderiver.py')
> except IOError:
>   print 'Copy `debderiver.py\' to `/usr/local/bin\'\n'
>
> I've looked through the docs and this one looks right. But it doesn't
> check the actual file.
> And I don't know how to use "if" and "os.path.abspath" with "try."

I'm not sure which file you mean and what you want to do with abspath, 
but not every check needs to be implemented with an exception. 
Exceptions are just an elegant way of handling function calls. If a 
plain if statement is enough, just use that. E.g. check debderive.py for 
"os.path.isdir".