Merge lp:~fgallaire/bzr/fix-gmtime-lite into lp:bzr

[Florent Gallaire]

Fix for Windows and 32-bit platforms buggy gmtime().

Fix for Windows buggy gmtime()
Fix for 32-bit platforms gmtime()
Light implementation

[Richard Wilbur (richard-wilbur)]

Have you filed a bug report for this problem? Or if it is already filed as a bug, let's link it here.

review: Needs Information

[Florent Gallaire (fgallaire)]

No open bug, it is necessary to file one ?

[Richard Wilbur (richard-wilbur)]

We like to keep track of defects using bugs: steps to reproduce help developers diagnose the issue and also write good tests. Since we use Test-Driven Development we prefer to demonstrate an issue with a failing test case. Then a good fix solves the failing test without making any others fail.

[Richard Wilbur (richard-wilbur)]

Thank you for coding up a solution.

Did you notice this as a bug in the operation of bzr or are you fixing the issue mentioned in the code?

If you would like assistance filing a bug report or creating a test case, please let me know.

[Florent Gallaire (fgallaire)]

Richard, I didn't notice this in the operation of bzr as I don't use Windows, but you can easily test it.

This is the bug mentioned in the code, I have already fixed it in Mercurial:
https://www.mercurial-scm.org/repo/hg/rev/87c6ad2251d8

[Florent Gallaire (fgallaire)]

I have filed a bug report:
https://bugs.launchpad.net/bzr/+bug/1669178

[Florent Gallaire (fgallaire)]

Very unhappy with your Test-Driven Development practice because I was trying to make code cleaner and found that:
http://bazaar.launchpad.net/~bzr-pqm/bzr/bzr.dev/revision/3754

[Vincent Ladeuil (vila)]

> but you can easily test it.

No we can't (at least I and Richard can't).
That's the issue here, without testing on windows, nothing can really be fixed.

> Very unhappy with your Test-Driven Development practice because I was trying to make code cleaner and found that:

What is making you unhappy ?

[Florent Gallaire (fgallaire)]

> > but you can easily test it.
>
> No we can't (at least I and Richard can't).
> That's the issue here, without testing on windows, nothing can really be
> fixed.

I have set up a Windows VM, you can see a screenshot of the bug in the report I filed.

> > Very unhappy with your Test-Driven Development practice because I was trying
> to make code cleaner and found that:
>
> What is making you unhappy ?

Look at the commit: awful replicated and spaghetti code justified by an useless test.

[Vincent Ladeuil (vila)]

> > > but you can easily test it.
> >
> > No we can't (at least I and Richard can't).
> > That's the issue here, without testing on windows, nothing can really be
> > fixed.
>
> I have set up a Windows VM, you can see a screenshot of the bug in the report
> I filed.

Ha, that explains it, I receive bug reports by mail and rarely looked at attachment.

Copy/pasting the text was not an option ? It would make citing your bug report easier rather than having to type what is in the screenshot:

> bzr 2.6b1 python 2.6.6 (Windows-7-6.1.7600)

Neither bzr-2.6 nor python2.6 are supported anymore.

> > > Very unhappy with your Test-Driven Development practice because I was
> trying
> > to make code cleaner and found that:
> >
> > What is making you unhappy ?
>
> Look at the commit: awful replicated and spaghetti code justified by an
> useless test.

Oook.

So, I didn't write that code nor that test so I won't take that personally.

Now, if you expect any project contributor to welcome your own contribution, it may be a good idea to not insult their work to start with.

Since you have a way to reproduce the issue though, and know how to TDD better than us, we'll welcome a a test reproducing that issue with the current code base (as mentioned above 2.6b1 has EOL'ed long ago) and a fix making the test pass.

[Florent Gallaire (fgallaire)]

> > bzr 2.6b1 python 2.6.6 (Windows-7-6.1.7600)
>
> Neither bzr-2.6 nor python2.6 are supported anymore.

I have installed the last version packaged for Windows to show you the bug:
http://wiki.bazaar.canonical.com/WindowsDownloads

So in fact, is bazaar still supporting the Windows platform ?

> So, I didn't write that code nor that test so I won't take that personally.
>
> Now, if you expect any project contributor to welcome your own contribution,
> it may be a good idea to not insult their work to start with.

No offence, and no insult here. The author of the commit himself has commented that this was replicated code and for the only reason to make the test working... anyway it isn't the point.

[Vincent Ladeuil (vila)]

> So in fact, is bazaar still supporting the Windows platform ?

See mailing archives for details. At this point the last installer is for 2.6b1 (b for beta).

A contributor willing to build a new installer and as such restore the release pipeline for windows would be highly welcome.

[Florent Gallaire (fgallaire)]

tests added, ready to merge

[Vincent Ladeuil (vila)]

Good work thanks !

I'm (pleasantly) surprised that this ends up being so lite (and focused).

As a side effect, it shows that bzr doesn't care about dates (which can't be guaranteed to be accurate in a distributed environment where host clocks can't be trusted).

grepping for 'gmtime(' I found a few references you probably want to fix too:

./bzrlib/doc_generate/autodoc_rstx.py:42: tt = time.gmtime(t)
./bzrlib/doc_generate/autodoc_man.py:48: tt = time.gmtime(t)
./bzrlib/doc_generate/autodoc_bash_completion.py:34: tt = time.gmtime(t)
./bzrlib/crash.py:245: date_string = time.strftime('%Y-%m-%dT%H:%M', time.gmtime())

(I don't think they matter as much but being consistent and using osutils.gmtime() instead of time.gmtime() remove confusion about why the code base would use two different versions).

This bug fix is worth mentioning in the news in doc/en/release-notes/bzr-2.8.txt

Final administrativia: please sign the CLA https://www.ubuntu.com/legal/contributors

review: Needs Fixing

[Florent Gallaire (fgallaire)]

> Good work thanks !

Thanks

> ./bzrlib/doc_generate/autodoc_rstx.py:42: tt = time.gmtime(t)
> ./bzrlib/doc_generate/autodoc_man.py:48: tt = time.gmtime(t)
> ./bzrlib/doc_generate/autodoc_bash_completion.py:34: tt = time.gmtime(t)
> ./bzrlib/crash.py:245: date_string = time.strftime('%Y-%m-%dT%H:%M',
> time.gmtime())
>
> (I don't think they matter as much but being consistent and using
> osutils.gmtime() instead of time.gmtime() remove confusion about why the code
> base would use two different versions).

I haven't replaced time.gmtime() and time.gmtime(time.time()) because they run against the present time which is not buggy, will only be if we still use bazaar on 32-bit platforms in more than 20 years.

Do you want I fix it even so ?

> Final administrativia: please sign the CLA
> https://www.ubuntu.com/legal/contributors

Done

[Richard Wilbur (richard-wilbur)]

@Florent: Thank you for filing the bugs and creating the tests.

Since these problems have our attention presently, go ahead and fix the 4 files still using time.gmtime to use your new implementation in osutils.gmtime. Then we will be consistently using osutils.gmtime and won't have this particular issue in 20+ years should we still have users running bzr on 32-bit platforms at that time. (fairly likely)

[Vincent Ladeuil (vila)]

In addition to what Richard said, having a single call makes it easier for newcomers to use the right one without having to research which one is appropriate for their case.

By the way, adding a docstring to osutils.gmtime() seems like the right way to achieve that. Nothing exotic, just mentioning why using time.gmtime() is not appropriate and why using osutils.gmtime() is better.

[Florent Gallaire (fgallaire)]

Should be good now.

[Richard Wilbur (richard-wilbur)]

Thank you for the fix to some problems I didn't know we had. Thank you also for the explanatory docstring and updating the rest of the code to use the fix.

I have no further reservations about merging this.

+1

@Vincent: Should we target 7.1 and then merge up to trunk?

review: Approve

[Vincent Ladeuil (vila)]

Thanks !

> Should we target 7.1 and then merge up to trunk?

No, it's not a critical issue and almost a new feature, so... trunk is appropriate.

Will land asap.

review: Approve

[Florent Gallaire (fgallaire)]

Thanks @Richard and @Vincent, happy to work with you.

[bzr PQM (bzr-pqm)]

The attempt to merge lp:~fgallaire/bzr/fix-gmtime-lite into lp:bzr failed. Below is the output from the failed tests.

python tools/rst2html.py --link-stylesheet --footnote-references=superscript --halt=warning --stylesheet=../../default.css doc/en/tutorials/tutorial.txt "doc/en/tutorials/tutorial.html"
python tools/rst2html.py --link-stylesheet --footnote-references=superscript --halt=warning --stylesheet=../../default.css doc/en/tutorials/using_bazaar_with_launchpad.txt "doc/en/tutorials/using_bazaar_with_launchpad.html"
python tools/rst2html.py --link-stylesheet --footnote-references=superscript --halt=warning --stylesheet=../../default.css doc/en/tutorials/centralized_workflow.txt "doc/en/tutorials/centralized_workflow.html"
python tools/rst2html.py --link-stylesheet --footnote-references=superscript --halt=warning --stylesheet=../../default.css doc/ru/tutorials/centralized_workflow.txt "doc/ru/tutorials/centralized_workflow.html"
python tools/rst2html.py --link-stylesheet --footnote-references=superscript --halt=warning --stylesheet=../../default.css doc/ru/tutorials/using_bazaar_with_launchpad.txt "doc/ru/tutorials/using_bazaar_with_launchpad.html"
python tools/rst2html.py --link-stylesheet --footnote-references=superscript --halt=warning --stylesheet=../../default.css doc/ru/tutorials/tutorial.txt "doc/ru/tutorials/tutorial.html"
python tools/rst2html.py --link-stylesheet --footnote-references=superscript --halt=warning --stylesheet=../../default.css doc/ja/tutorials/tutorial.txt "doc/ja/tutorials/tutorial.html"
python tools/rst2html.py --link-stylesheet --footnote-references=superscript --halt=warning --stylesheet=../../default.css doc/ja/tutorials/using_bazaar_with_launchpad.txt "doc/ja/tutorials/using_bazaar_with_launchpad.html"
python tools/rst2html.py --link-stylesheet --footnote-references=superscript --halt=warning --stylesheet=../../default.css doc/ja/tutorials/centralized_workflow.txt "doc/ja/tutorials/centralized_workflow.html"
python tools/rst2html.py --link-stylesheet --footnote-references=superscript --halt=warning --stylesheet=../../default.css doc/en/mini-tutorial/index.txt "doc/en/mini-tutorial/index.html"
python tools/rst2html.py --link-stylesheet --footnote-references=superscript --halt=warning --stylesheet=../../default.css doc/ja/mini-tutorial/index.txt "doc/ja/mini-tutorial/index.html"
python tools/rst2html.py --link-stylesheet --footnote-references=superscript --halt=warning --stylesheet=../../default.css doc/ru/mini-tutorial/index.txt "doc/ru/mini-tutorial/index.html"
python tools/rst2html.py --link-stylesheet --footnote-references=superscript --halt=warning --stylesheet=../../default.css doc/es/mini-tutorial/index.txt "doc/es/mini-tutorial/index.html"
python tools/rst2html.py --link-stylesheet --footnote-references=superscript --halt=warning --stylesheet=../../default.css doc/en/user-guide/index-plain.txt doc/en/user-guide/index-plain.html
python tools/rst2html.py --link-stylesheet --footnote-references=superscript --halt=warning --stylesheet=../../default.css doc/ja/user-guide/index-plain.txt "doc/ja/user-guide/index-plain.html"
python tools/rst2html.py --link-stylesheet --footnote-references=superscript --halt...

[Florent Gallaire (fgallaire)]

Unremoved the needed time import. Tests run fine now.

=== modified file 'bzrlib/annotate.py'
--- bzrlib/annotate.py	2012-06-26 12:14:56 +0000
+++ bzrlib/annotate.py	2017-03-17 10:40:31 +0000
@@ -215,7 +215,7 @@
             rev = revisions[origin]
             tz = rev.timezone or 0
             date_str = time.strftime('%Y%m%d',
-                                     time.gmtime(rev.timestamp + tz))
+                                     osutils.gmtime(rev.timestamp + tz))
             # a lazy way to get something like the email address
             # TODO: Get real email address
             author = rev.get_apparent_authors()[0]
 
=== modified file 'bzrlib/crash.py'
--- bzrlib/crash.py	2013-01-30 05:55:38 +0000
+++ bzrlib/crash.py	2017-03-17 10:40:31 +0000
@@ -242,7 +242,7 @@
         # Windows or if it's manually configured it might need to be created,
         # and then it should be private
         os.makedirs(crash_dir, mode=0600)
-    date_string = time.strftime('%Y-%m-%dT%H:%M', time.gmtime())
+    date_string = time.strftime('%Y-%m-%dT%H:%M', osutils.gmtime())
     # XXX: getuid doesn't work on win32, but the crash directory is per-user
     if sys.platform == 'win32':
         user_part = ''
 
=== modified file 'bzrlib/doc_generate/autodoc_bash_completion.py'
--- bzrlib/doc_generate/autodoc_bash_completion.py	2011-12-19 13:23:58 +0000
+++ bzrlib/doc_generate/autodoc_bash_completion.py	2017-03-17 10:40:31 +0000
@@ -23,6 +23,7 @@
 import bzrlib
 import bzrlib.help
 import bzrlib.commands
+import bzrlib.osutils
 
 
 def get_filename(options):
@@ -30,8 +31,7 @@
 
 
 def infogen(options, outfile):
-    t = time.time()
-    tt = time.gmtime(t)
+    tt = bzrlib.osutils.gmtime()
     params = \
            { "bzrcmd": options.bzr_name,
              "datestamp": time.strftime("%Y-%m-%d",tt),
 
=== modified file 'bzrlib/doc_generate/autodoc_man.py'
--- bzrlib/doc_generate/autodoc_man.py	2015-03-14 23:44:01 +0000
+++ bzrlib/doc_generate/autodoc_man.py	2017-03-17 10:40:31 +0000
@@ -32,6 +32,7 @@
 import bzrlib.help
 import bzrlib.help_topics
 import bzrlib.commands
+import bzrlib.osutils
 
 from bzrlib.plugin import load_plugins
 load_plugins()
@@ -44,8 +45,7 @@
 
 def infogen(options, outfile):
     """Assembles a man page"""
-    t = time.time()
-    tt = time.gmtime(t)
+    tt = bzrlib.osutils.gmtime()
     params = \
            { "bzrcmd": options.bzr_name,
              "datestamp": time.strftime("%Y-%m-%d",tt),
 
=== modified file 'bzrlib/doc_generate/autodoc_rstx.py'
--- bzrlib/doc_generate/autodoc_rstx.py	2015-11-15 02:30:05 +0000
+++ bzrlib/doc_generate/autodoc_rstx.py	2017-03-17 10:40:31 +0000
@@ -38,8 +38,7 @@
 
 def infogen(options, outfile):
     """Create manual in RSTX format"""
-    t = time.time()
-    tt = time.gmtime(t)
+    tt = bzrlib.osutils.gmtime()
     params = \
            { "bzrcmd": options.bzr_name,
              "datestamp": time.strftime("%Y-%m-%d",tt),
 
=== modified file 'bzrlib/osutils.py'
--- bzrlib/osutils.py	2013-06-24 12:03:12 +0000
+++ bzrlib/osutils.py	2017-03-17 10:40:31 +0000
@@ -27,6 +27,7 @@
 from bzrlib.lazy_import import lazy_import
 lazy_import(globals(), """
 from datetime import datetime
+from datetime import timedelta
 import getpass
 import locale
 import ntpath
@@ -827,6 +828,16 @@
             return True
 
 
+def gmtime(seconds=None):
+    """Convert seconds since the Epoch to a time tuple expressing UTC (a.k.a.
+    GMT). When 'seconds' is not passed in, convert the current time instead.
+    Handy replacement for time.gmtime() buggy on Windows and 32-bit platforms.
+    """
+    if seconds is None:
+        seconds = time.time()
+    return (datetime(1970, 1, 1) + timedelta(seconds=seconds)).timetuple()
+
+
 def local_time_offset(t=None):
     """Return offset of local zone from GMT, either at present or at time t."""
     if t is None:
@@ -872,7 +883,7 @@
     """
     if offset is None:
         offset = 0
-    tt = time.gmtime(t + offset)
+    tt = gmtime(t + offset)
     date_fmt = _default_format_by_weekday_num[tt[6]]
     date_str = time.strftime(date_fmt, tt)
     offset_str = _cache.get(offset, None)
@@ -904,12 +915,12 @@
 
 def _format_date(t, offset, timezone, date_fmt, show_offset):
     if timezone == 'utc':
-        tt = time.gmtime(t)
+        tt = gmtime(t)
         offset = 0
     elif timezone == 'original':
         if offset is None:
             offset = 0
-        tt = time.gmtime(t + offset)
+        tt = gmtime(t + offset)
     elif timezone == 'local':
         tt = time.localtime(t)
         offset = local_time_offset(t)
@@ -925,7 +936,7 @@
 
 
 def compact_date(when):
-    return time.strftime('%Y%m%d%H%M%S', time.gmtime(when))
+    return time.strftime('%Y%m%d%H%M%S', gmtime(when))
 
 
 def format_delta(delta):
 
=== modified file 'bzrlib/tests/blackbox/test_commit.py'
--- bzrlib/tests/blackbox/test_commit.py	2016-02-01 18:06:32 +0000
+++ bzrlib/tests/blackbox/test_commit.py	2017-03-17 10:40:31 +0000
@@ -713,7 +713,40 @@
         self.assertEqual(
             'Sat 2009-10-10 08:00:00 +0100',
             osutils.format_date(last_rev.timestamp, last_rev.timezone))
-        
+
+    def test_commit_time_negative_windows(self):
+        tree = self.make_branch_and_tree('tree')
+        self.build_tree(['tree/hello.txt'])
+        tree.add('hello.txt')
+        out, err = self.run_bzr("commit -m hello "
+            "--commit-time='1969-10-10 00:00:00 +0000' tree/hello.txt")
+        last_rev = tree.branch.repository.get_revision(tree.last_revision())
+        self.assertEqual(
+            'Fri 1969-10-10 00:00:00 +0000',
+            osutils.format_date(last_rev.timestamp, last_rev.timezone))
+
+    def test_commit_time_negative_32bit(self):
+        tree = self.make_branch_and_tree('tree')
+        self.build_tree(['tree/hello.txt'])
+        tree.add('hello.txt')
+        out, err = self.run_bzr("commit -m hello "
+            "--commit-time='1900-01-01 00:00:00 +0000' tree/hello.txt")
+        last_rev = tree.branch.repository.get_revision(tree.last_revision())
+        self.assertEqual(
+            'Mon 1900-01-01 00:00:00 +0000',
+            osutils.format_date(last_rev.timestamp, last_rev.timezone))
+
+    def test_commit_time_positive_32bit(self):
+        tree = self.make_branch_and_tree('tree')
+        self.build_tree(['tree/hello.txt'])
+        tree.add('hello.txt')
+        out, err = self.run_bzr("commit -m hello "
+            "--commit-time='2039-01-01 00:00:00 +0000' tree/hello.txt")
+        last_rev = tree.branch.repository.get_revision(tree.last_revision())
+        self.assertEqual(
+            'Sat 2039-01-01 00:00:00 +0000',
+            osutils.format_date(last_rev.timestamp, last_rev.timezone))
+
     def test_commit_time_bad_time(self):
         tree = self.make_branch_and_tree('tree')
         self.build_tree(['tree/hello.txt'])
 
=== modified file 'bzrlib/timestamp.py'
--- bzrlib/timestamp.py	2011-12-18 15:28:38 +0000
+++ bzrlib/timestamp.py	2017-03-17 10:40:31 +0000
@@ -57,7 +57,7 @@
     # revision XML entry will be reproduced faithfully.
     if offset is None:
         offset = 0
-    tt = time.gmtime(t + offset)
+    tt = osutils.gmtime(t + offset)
 
     return (osutils.weekdays[tt[6]] +
             time.strftime(" %Y-%m-%d %H:%M:%S", tt)
@@ -119,10 +119,6 @@
     # give the epoch in utc
     if secs == 0:
         offset = 0
-    if secs + offset < 0:
-        from warnings import warn
-        warn("gmtime of negative time (%s, %s) may not work on Windows" %
-                (secs, offset))
     return osutils.format_date(secs, offset=offset,
             date_fmt='%Y-%m-%d %H:%M:%S')
 
 
=== modified file 'doc/en/release-notes/bzr-2.8.txt'
--- doc/en/release-notes/bzr-2.8.txt	2017-01-17 15:02:46 +0000
+++ doc/en/release-notes/bzr-2.8.txt	2017-03-17 10:40:31 +0000
@@ -36,6 +36,9 @@
    doc/en/user-reference only contains English documentation.
    (Jelmer Vernooij, #1565503)
 
+ * Fix for Windows and 32-bit platforms buggy gmtime().
+   (Florent Gallaire, #1669178, #1670243)
+
 Documentation
 *************