Bug#848249: diffoscope: set_locale needs to call tzset

[Chris Lamb]

Hi Brett,

> So the whole thing with the set_locale test fixture is a little subtle.  The
> best reference I found for this is
> .

Very nice research, thank you. :)

> If you want to make sure it runs before any test, we can do that too.  I've
> attached a patch for that as well.

Applied with thanks. I think this better than relying on the import doing the
right thing, especially as its likely that someone armed with pylint is likely
to remove such an apparent no-op.


Regards,

-- 
  ,''`.
 : :'  : Chris Lamb
 `. `'`  la...@debian.org / chris-lamb.co.uk
   `-

Bug#848249: diffoscope: set_locale needs to call tzset

[Brett Smith]

So the whole thing with the set_locale test fixture is a little subtle.  The
best reference I found for this is
.

Before I touched this, the autouse=True argument meant it applied to all
other fixtures in the same utils module as it.  In systems where the bug
could occur, the test_gzip metadata test would fail, because the timezone
wasn't necessarily set when python-magic generated a timestamp string for
the gzip1 and gzip2 fixtures.

With my first patch, importing set_locale into the test_gzip module,
combined with autouse=True, meant that it now applied to all the tests and
fixtures in that module too.  This made the tests reliable by making sure
the gzip1 and gzip2 fixtures always rendered a timestamp in UTC.

If you want to make sure it runs before any test, we can do that too.  I've
attached a patch for that as well.
>From b4d0a395066f4b8f86d520e58acdd11dd4eb1a5e Mon Sep 17 00:00:00 2001
From: Brett Smith 
Date: Thu, 15 Dec 2016 16:14:29 -0500
Subject: [PATCH] tests: Ensure set_locale fixture runs before all tests.

See

for details about the old and new behavior.
---
 tests/comparators/utils.py | 4 
 tests/conftest.py  | 4 
 2 files changed, 4 insertions(+), 4 deletions(-)
 create mode 100644 tests/conftest.py

diff --git a/tests/comparators/utils.py b/tests/comparators/utils.py
index f796378..0b60826 100644
--- a/tests/comparators/utils.py
+++ b/tests/comparators/utils.py
@@ -31,10 +31,6 @@ from diffoscope.presenters.text import output_text
 from diffoscope.comparators.binary import FilesystemFile, NonExistingFile
 
 
-@pytest.fixture(autouse=True)
-def set_locale():
-diffoscope.set_locale()
-
 def tools_missing(*required):
 return not required or any(find_executable(x) is None for x in required)
 
diff --git a/tests/conftest.py b/tests/conftest.py
new file mode 100644
index 000..e238f1b
--- /dev/null
+++ b/tests/conftest.py
@@ -0,0 +1,4 @@
+import diffoscope
+import pytest
+
+set_locale = pytest.fixture(autouse=True, scope='session')(diffoscope.set_locale)
-- 
2.1.4

Bug#848249: diffoscope: set_locale needs to call tzset

[Chris Lamb]

tags 848249 + pending
thanks

Chris Lamb wrote:

> Whilst you could "just" add this, I was wondering if you could ensure that
> pytest *always* calls this so other tests are not affected. I believe there
> is some magic fixture name/decorator in pytest for this.

So, there is indeed some magic and we already have it:

  
https://anonscm.debian.org/git/reproducible/diffoscope.git/tree/tests/comparators/utils.py#n33

… so no need to do anything. Many thanks!


Regards,

-- 
  ,''`.
 : :'  : Chris Lamb
 `. `'`  la...@debian.org / chris-lamb.co.uk
   `-

Bug#848249: diffoscope: set_locale needs to call tzset

[Chris Lamb]

Hi Brett,

> Tags: patch

Thanks for the patch! However, you do not actually call set_locale in the
gzip test, you merely import it.

Whilst you could "just" add this, I was wondering if you could ensure that
pytest *always* calls this so other tests are not affected. I believe there
is some magic fixture name/decorator in pytest for this.

If you could provide this as a separate patch to the tzset bit, then that
would be ideal.

(I'll apply just the tzset bit now, however.)


Regards,

-- 
  ,''`.
 : :'  : Chris Lamb
 `. `'`  la...@debian.org / chris-lamb.co.uk
   `-

Bug#848249: diffoscope: set_locale needs to call tzset

[Brett Smith]

Source: diffoscope
Severity: minor
Tags: patch

diffoscope sets the timezone to UTC to ensure that timezones are consistent
from diff output tools.  However, it does not call tzset after it does this.
Because of this, C code running in the same process may continue to see and
use the user's original timezone.  In particular, the python-magic module
that wraps libmagic is susceptible to this.

I believe this is the source of test_gzip.metadata test failures that have
been reported in, e.g., #817193.  Test failures look inconsistent because
you'll only see it if (a) you're using the python-magic wrapper module, and
(b) your current timezone does not align with UTC.

A patch is attached.

-- System Information:
Debian Release: 8.6
  APT prefers stable-updates
  APT policy: (500, 'stable-updates'), (500, 'stable')
Architecture: amd64 (x86_64)

Kernel: Linux 4.9.0-rc6+ (SMP w/4 CPU cores)
Locale: LANG=en_US.UTF-8, LC_CTYPE=en_US.UTF-8 (charmap=UTF-8) (ignored: LC_ALL 
set to en_US.utf8)
Shell: /bin/sh linked to /bin/dash
Init: systemd (via /run/systemd/system)
>From de145d41ed76e57dcae8dbfc1c7404171ac9c2d8 Mon Sep 17 00:00:00 2001
From: Brett Smith 
Date: Thu, 15 Dec 2016 12:14:42 -0500
Subject: [PATCH] diffoscope: set_locale calls tzset.

This is necessary to update timezone information for C code we've pulled in
that might've already called it.  In particular, the python-magic libmagic
wrapper can be affected by this, and print out gzip mtimes in the user's
timezone.

Accordingly, we call set_locale as a test fixture in test_gzip to make sure
things are set up correctly for the metadata difference tests.
---
 diffoscope/__init__.py | 2 ++
 tests/comparators/test_gzip.py | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/diffoscope/__init__.py b/diffoscope/__init__.py
index b1bb1d4..827445f 100644
--- a/diffoscope/__init__.py
+++ b/diffoscope/__init__.py
@@ -23,6 +23,7 @@ import logging
 import platform
 import tempfile
 import functools
+import time
 
 from distutils.spawn import find_executable
 
@@ -90,6 +91,7 @@ def set_locale():
 os.environ[var] = 'C'
 os.environ['LC_CTYPE'] = 'C.UTF-8'
 os.environ['TZ'] = 'UTC'
+time.tzset()
 
 
 temp_files = []
diff --git a/tests/comparators/test_gzip.py b/tests/comparators/test_gzip.py
index a448820..6481ef6 100644
--- a/tests/comparators/test_gzip.py
+++ b/tests/comparators/test_gzip.py
@@ -25,7 +25,7 @@ from diffoscope.comparators import specialize
 from diffoscope.comparators.gzip import GzipFile
 from diffoscope.comparators.binary import FilesystemFile, NonExistingFile
 
-from utils import data, load_fixture
+from utils import data, load_fixture, set_locale
 
 TEST_FILE1_PATH = data('test1.gz')
 TEST_FILE2_PATH = data('test2.gz')
-- 
2.1.4