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

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

No open bug, it is necessary to file one ?

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.

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.

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

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

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

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

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

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

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

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

tests added, ready to merge

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

> 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

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

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.

Should be good now.

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?

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.

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

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

Unremoved the needed time import. Tests run fine now.