Bazaar

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

  1. fix-gmtime-lite
  2. Merge into bzr.dev
Proposed by Florent Gallaire
Status: Merged
Approved by: Vincent Ladeuil
Approved revision: no longer in the source branch.
Merged at revision: 6622
Proposed branch: lp:~fgallaire/bzr/fix-gmtime-lite
Merge into: lp:bzr
Diff against target: 228 lines (+60/-18)
9 files modified
bzrlib/annotate.py (+1/-1)
bzrlib/crash.py (+1/-1)
bzrlib/doc_generate/autodoc_bash_completion.py (+2/-2)
bzrlib/doc_generate/autodoc_man.py (+2/-2)
bzrlib/doc_generate/autodoc_rstx.py (+1/-2)
bzrlib/osutils.py (+15/-4)
bzrlib/tests/blackbox/test_commit.py (+34/-1)
bzrlib/timestamp.py (+1/-5)
doc/en/release-notes/bzr-2.8.txt (+3/-0)
To merge this branch: bzr merge lp:~fgallaire/bzr/fix-gmtime-lite
Reviewer Review Type Date Requested Status
Vincent Ladeuil Approve
Richard Wilbur Approve
Review via email: mp+318653@code.launchpad.net

Commit message

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

Description of the change

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

To post a comment you must log in.
Revision history for this message
Richard Wilbur (richard-wilbur) wrote :

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
Revision history for this message
Florent Gallaire (fgallaire) wrote :

No open bug, it is necessary to file one ?

Revision history for this message
Richard Wilbur (richard-wilbur) wrote :

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.

Revision history for this message
Richard Wilbur (richard-wilbur) wrote :

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.

Revision history for this message
Florent Gallaire (fgallaire) wrote :

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

Revision history for this message
Florent Gallaire (fgallaire) wrote :

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

Revision history for this message
Florent Gallaire (fgallaire) wrote :

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

Revision history for this message
Vincent Ladeuil (vila) wrote :

> 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 ?

Revision history for this message
Florent Gallaire (fgallaire) wrote :

> > 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.

Revision history for this message
Vincent Ladeuil (vila) wrote :

> > > 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.

Revision history for this message
Florent Gallaire (fgallaire) wrote :

> > 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.

Revision history for this message
Vincent Ladeuil (vila) wrote :

> 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.

Revision history for this message
Florent Gallaire (fgallaire) wrote :

tests added, ready to merge

Revision history for this message
Vincent Ladeuil (vila) wrote :

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
Revision history for this message
Florent Gallaire (fgallaire) wrote :

> 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

Revision history for this message
Richard Wilbur (richard-wilbur) wrote :

@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)

Revision history for this message
Vincent Ladeuil (vila) wrote :

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.

Revision history for this message
Florent Gallaire (fgallaire) wrote :

Should be good now.

Revision history for this message
Richard Wilbur (richard-wilbur) wrote :

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
Revision history for this message
Vincent Ladeuil (vila) wrote :

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
Revision history for this message
Florent Gallaire (fgallaire) wrote :

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

Revision history for this message
bzr PQM (bzr-pqm) wrote :
Download full text (16.6 KiB)

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...

Revision history for this message
Florent Gallaire (fgallaire) wrote :

Unremoved the needed time import. Tests run fine now.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bzrlib/annotate.py'
2--- bzrlib/annotate.py 2012-06-26 12:14:56 +0000
3+++ bzrlib/annotate.py 2017-03-17 10:40:31 +0000
4@@ -215,7 +215,7 @@
5 rev = revisions[origin]
6 tz = rev.timezone or 0
7 date_str = time.strftime('%Y%m%d',
8- time.gmtime(rev.timestamp + tz))
9+ osutils.gmtime(rev.timestamp + tz))
10 # a lazy way to get something like the email address
11 # TODO: Get real email address
12 author = rev.get_apparent_authors()[0]
13
14=== modified file 'bzrlib/crash.py'
15--- bzrlib/crash.py 2013-01-30 05:55:38 +0000
16+++ bzrlib/crash.py 2017-03-17 10:40:31 +0000
17@@ -242,7 +242,7 @@
18 # Windows or if it's manually configured it might need to be created,
19 # and then it should be private
20 os.makedirs(crash_dir, mode=0600)
21- date_string = time.strftime('%Y-%m-%dT%H:%M', time.gmtime())
22+ date_string = time.strftime('%Y-%m-%dT%H:%M', osutils.gmtime())
23 # XXX: getuid doesn't work on win32, but the crash directory is per-user
24 if sys.platform == 'win32':
25 user_part = ''
26
27=== modified file 'bzrlib/doc_generate/autodoc_bash_completion.py'
28--- bzrlib/doc_generate/autodoc_bash_completion.py 2011-12-19 13:23:58 +0000
29+++ bzrlib/doc_generate/autodoc_bash_completion.py 2017-03-17 10:40:31 +0000
30@@ -23,6 +23,7 @@
31 import bzrlib
32 import bzrlib.help
33 import bzrlib.commands
34+import bzrlib.osutils
35
36
37 def get_filename(options):
38@@ -30,8 +31,7 @@
39
40
41 def infogen(options, outfile):
42- t = time.time()
43- tt = time.gmtime(t)
44+ tt = bzrlib.osutils.gmtime()
45 params = \
46 { "bzrcmd": options.bzr_name,
47 "datestamp": time.strftime("%Y-%m-%d",tt),
48
49=== modified file 'bzrlib/doc_generate/autodoc_man.py'
50--- bzrlib/doc_generate/autodoc_man.py 2015-03-14 23:44:01 +0000
51+++ bzrlib/doc_generate/autodoc_man.py 2017-03-17 10:40:31 +0000
52@@ -32,6 +32,7 @@
53 import bzrlib.help
54 import bzrlib.help_topics
55 import bzrlib.commands
56+import bzrlib.osutils
57
58 from bzrlib.plugin import load_plugins
59 load_plugins()
60@@ -44,8 +45,7 @@
61
62 def infogen(options, outfile):
63 """Assembles a man page"""
64- t = time.time()
65- tt = time.gmtime(t)
66+ tt = bzrlib.osutils.gmtime()
67 params = \
68 { "bzrcmd": options.bzr_name,
69 "datestamp": time.strftime("%Y-%m-%d",tt),
70
71=== modified file 'bzrlib/doc_generate/autodoc_rstx.py'
72--- bzrlib/doc_generate/autodoc_rstx.py 2015-11-15 02:30:05 +0000
73+++ bzrlib/doc_generate/autodoc_rstx.py 2017-03-17 10:40:31 +0000
74@@ -38,8 +38,7 @@
75
76 def infogen(options, outfile):
77 """Create manual in RSTX format"""
78- t = time.time()
79- tt = time.gmtime(t)
80+ tt = bzrlib.osutils.gmtime()
81 params = \
82 { "bzrcmd": options.bzr_name,
83 "datestamp": time.strftime("%Y-%m-%d",tt),
84
85=== modified file 'bzrlib/osutils.py'
86--- bzrlib/osutils.py 2013-06-24 12:03:12 +0000
87+++ bzrlib/osutils.py 2017-03-17 10:40:31 +0000
88@@ -27,6 +27,7 @@
89 from bzrlib.lazy_import import lazy_import
90 lazy_import(globals(), """
91 from datetime import datetime
92+from datetime import timedelta
93 import getpass
94 import locale
95 import ntpath
96@@ -827,6 +828,16 @@
97 return True
98
99
100+def gmtime(seconds=None):
101+ """Convert seconds since the Epoch to a time tuple expressing UTC (a.k.a.
102+ GMT). When 'seconds' is not passed in, convert the current time instead.
103+ Handy replacement for time.gmtime() buggy on Windows and 32-bit platforms.
104+ """
105+ if seconds is None:
106+ seconds = time.time()
107+ return (datetime(1970, 1, 1) + timedelta(seconds=seconds)).timetuple()
108+
109+
110 def local_time_offset(t=None):
111 """Return offset of local zone from GMT, either at present or at time t."""
112 if t is None:
113@@ -872,7 +883,7 @@
114 """
115 if offset is None:
116 offset = 0
117- tt = time.gmtime(t + offset)
118+ tt = gmtime(t + offset)
119 date_fmt = _default_format_by_weekday_num[tt[6]]
120 date_str = time.strftime(date_fmt, tt)
121 offset_str = _cache.get(offset, None)
122@@ -904,12 +915,12 @@
123
124 def _format_date(t, offset, timezone, date_fmt, show_offset):
125 if timezone == 'utc':
126- tt = time.gmtime(t)
127+ tt = gmtime(t)
128 offset = 0
129 elif timezone == 'original':
130 if offset is None:
131 offset = 0
132- tt = time.gmtime(t + offset)
133+ tt = gmtime(t + offset)
134 elif timezone == 'local':
135 tt = time.localtime(t)
136 offset = local_time_offset(t)
137@@ -925,7 +936,7 @@
138
139
140 def compact_date(when):
141- return time.strftime('%Y%m%d%H%M%S', time.gmtime(when))
142+ return time.strftime('%Y%m%d%H%M%S', gmtime(when))
143
144
145 def format_delta(delta):
146
147=== modified file 'bzrlib/tests/blackbox/test_commit.py'
148--- bzrlib/tests/blackbox/test_commit.py 2016-02-01 18:06:32 +0000
149+++ bzrlib/tests/blackbox/test_commit.py 2017-03-17 10:40:31 +0000
150@@ -713,7 +713,40 @@
151 self.assertEqual(
152 'Sat 2009-10-10 08:00:00 +0100',
153 osutils.format_date(last_rev.timestamp, last_rev.timezone))
154-
155+
156+ def test_commit_time_negative_windows(self):
157+ tree = self.make_branch_and_tree('tree')
158+ self.build_tree(['tree/hello.txt'])
159+ tree.add('hello.txt')
160+ out, err = self.run_bzr("commit -m hello "
161+ "--commit-time='1969-10-10 00:00:00 +0000' tree/hello.txt")
162+ last_rev = tree.branch.repository.get_revision(tree.last_revision())
163+ self.assertEqual(
164+ 'Fri 1969-10-10 00:00:00 +0000',
165+ osutils.format_date(last_rev.timestamp, last_rev.timezone))
166+
167+ def test_commit_time_negative_32bit(self):
168+ tree = self.make_branch_and_tree('tree')
169+ self.build_tree(['tree/hello.txt'])
170+ tree.add('hello.txt')
171+ out, err = self.run_bzr("commit -m hello "
172+ "--commit-time='1900-01-01 00:00:00 +0000' tree/hello.txt")
173+ last_rev = tree.branch.repository.get_revision(tree.last_revision())
174+ self.assertEqual(
175+ 'Mon 1900-01-01 00:00:00 +0000',
176+ osutils.format_date(last_rev.timestamp, last_rev.timezone))
177+
178+ def test_commit_time_positive_32bit(self):
179+ tree = self.make_branch_and_tree('tree')
180+ self.build_tree(['tree/hello.txt'])
181+ tree.add('hello.txt')
182+ out, err = self.run_bzr("commit -m hello "
183+ "--commit-time='2039-01-01 00:00:00 +0000' tree/hello.txt")
184+ last_rev = tree.branch.repository.get_revision(tree.last_revision())
185+ self.assertEqual(
186+ 'Sat 2039-01-01 00:00:00 +0000',
187+ osutils.format_date(last_rev.timestamp, last_rev.timezone))
188+
189 def test_commit_time_bad_time(self):
190 tree = self.make_branch_and_tree('tree')
191 self.build_tree(['tree/hello.txt'])
192
193=== modified file 'bzrlib/timestamp.py'
194--- bzrlib/timestamp.py 2011-12-18 15:28:38 +0000
195+++ bzrlib/timestamp.py 2017-03-17 10:40:31 +0000
196@@ -57,7 +57,7 @@
197 # revision XML entry will be reproduced faithfully.
198 if offset is None:
199 offset = 0
200- tt = time.gmtime(t + offset)
201+ tt = osutils.gmtime(t + offset)
202
203 return (osutils.weekdays[tt[6]] +
204 time.strftime(" %Y-%m-%d %H:%M:%S", tt)
205@@ -119,10 +119,6 @@
206 # give the epoch in utc
207 if secs == 0:
208 offset = 0
209- if secs + offset < 0:
210- from warnings import warn
211- warn("gmtime of negative time (%s, %s) may not work on Windows" %
212- (secs, offset))
213 return osutils.format_date(secs, offset=offset,
214 date_fmt='%Y-%m-%d %H:%M:%S')
215
216
217=== modified file 'doc/en/release-notes/bzr-2.8.txt'
218--- doc/en/release-notes/bzr-2.8.txt 2017-01-17 15:02:46 +0000
219+++ doc/en/release-notes/bzr-2.8.txt 2017-03-17 10:40:31 +0000
220@@ -36,6 +36,9 @@
221 doc/en/user-reference only contains English documentation.
222 (Jelmer Vernooij, #1565503)
223
224+ * Fix for Windows and 32-bit platforms buggy gmtime().
225+ (Florent Gallaire, #1669178, #1670243)
226+
227 Documentation
228 *************
229