Re: RFS: s3cmd python module

[Jakub Wilk <jwilk@debian.org>]

* Jakub Wilk <jwilk@debian.org>, 2013-06-16, 22:31:
> http://mentors.debian.net/debian/pool/main/s/s3cmd/s3cmd_1.5.0~alpha1-1.dsc

s3cmd.1 reads "Copyright \(co 2007,2008,2009,2010,2011,2012", but 
d/copyright has only "Copyright: 2007-2011".

The new dependency (python-tz) is not documented in the changelog.

Do you know if fix-manpage-typos.patch has been forwarded upstream?

Lintian emits:
I: s3cmd source: vcs-field-not-canonical svn://svn.debian.org/svn/python-apps/packages/s3cmd/trunk svn://anonscm.debian.org/python-apps/packages/s3cmd/trunk
W: s3cmd source: out-of-date-standards-version 3.9.3 (current is 3.9.4)
P: s3cmd: no-upstream-changelog

vcs-field-not-canonical has been already fixed in the DPMT repository.
Upstream provides a NEWS file which you could install as changelog.


Now let's take a look at the upstream part...

lintian4python emits:
x: s3cmd: except-without-exception-type usr/share/s3cmd/S3/CloudFront.py:454
x: s3cmd: except-without-exception-type usr/share/s3cmd/S3/CloudFront.py:756
x: s3cmd: except-without-exception-type usr/share/s3cmd/S3/HashCache.py:19
x: s3cmd: except-without-exception-type usr/share/s3cmd/S3/SortedDict.py:60
x: s3cmd: except-without-exception-type usr/share/s3cmd/S3/SortedDict.py:88
x: s3cmd: except-without-exception-type usr/share/s3cmd/s3cmd:818
x: s3cmd: except-without-exception-type usr/share/s3cmd/s3cmd:831
x: s3cmd: except-without-exception-type usr/share/s3cmd/s3cmd:836
x: s3cmd: except-without-exception-type usr/share/s3cmd/s3cmd:843
x: s3cmd: except-without-exception-type usr/share/s3cmd/s3cmd:1660
e: s3cmd: pyflakes-undefined-name usr/share/s3cmd/S3/Utils.py:380: ex
e: s3cmd: pyflakes-undefined-name usr/share/s3cmd/S3/Progress.py:82: selfself

pyflakes-undefined-name are in this case obvious bugs that'll cause 
NameError exceptions.
You can read more about except-without-exception-type here:
http://docs.python.org/2/howto/doanddont.html#except

Timezones is a tricky business, and in my experience almost everybody 
gets it wrong. This package is not an exception! :P

| def dateS3toPython(date):
|     date = re.compile("(\.\d*)?Z").sub(".000Z", date)
|     return time.strptime(date, "%Y-%m-%dT%H:%M:%S.000Z")
| __all__.append("dateS3toPython")
|
| def dateS3toUnix(date):
|     ## FIXME: This should be timezone-aware.
|     ## Currently the argument to strptime() is GMT but mktime()
|     ## treats it as "localtime". Anyway...
|     return time.mktime(dateS3toPython(date))
| __all__.append("dateS3toUnix")

Yeah, as the FIXME says, this is not right. Fortunately, it's also easy 
to fix: just replace "time.mktime" with "calendar.timegm".


| def dateRFC822toPython(date):
|     return rfc822.parsedate(date)
| __all__.append("dateRFC822toPython")
|
| def dateRFC822toUnix(date):
|     return time.mktime(dateRFC822toPython(date))
| __all__.append("dateRFC822toUnix")

So dateRFC822toUnix() takes an RFC-822 date, throws away timezone 
information, then re-interprets the result as local time. This is 
obviously incorrect. Oh, and the rfc822 module is deprecated.
This is how you can reimplement it correctly:

def dateRFC822toUnix(date):
    parsed_date = email.utils.parsedate_tz(date)
    return email.utils.mktime_tz(parsed_date)


This function is buggy, too:
| def formatDateTime(s3timestamp):
|     try:
|         import pytz
|         timezone = pytz.timezone(os.environ.get('TZ', 'UTC'))
|         utc_dt = datetime.datetime(*dateS3toPython(s3timestamp)[0:6], tzinfo=pytz.timezone('UTC'))
|         dt_object = utc_dt.astimezone(timezone)
|     except ImportError:
|         dt_object = datetime.datetime(*dateS3toPython(s3timestamp)[0:6])
|     return dt_object.strftime("%Y-%m-%d %H:%M")
| __all__.append("formatDateTime")

If TZ is unset, the default should be the timezone from /etc/localtime, 
not UTC.

But anyway, once dateS3toUnix is fixed, formatDateTime can be trivially 
reimplemented without pytz as:

def formatDateTime(s3timestamp):
    unix_timestamp = dateS3toUnix(s3timestamp)
    return time.strftime("%Y-%m-%d %H:%M", time.localtime(unix_timestamp))


Please inform upstream about these problems.

--
Jakub Wilk