Bug#876901: QFINDTESTDATA uses __FILE__

[Pino Toscano]

In data lunedì 13 novembre 2017 17:35:00 CET, Ximin Luo ha scritto:
> Adrian Bunk:
> > Control: reassign -1 qtbase5-dev
> > Control: reassign 876917 qtbase5-dev
> > Control: reassign 876933 qtbase5-dev
> > Control: forcemerge -1 876917 876933
> > Control: retitle -1 QFINDTESTDATA uses __FILE__
> > Control: severity -1 normal
> > Control: affects -1 src:karchive src:ki18n src:kcodecs src:kparts
> > thanks
> > 
> > The problem is the following implementation in
> > /usr/include/x86_64-linux-gnu/qt5/QtTest/qtestcase.h:
> > # define QFINDTESTDATA(basepath)\
> >     QTest::qFindTestData(basepath, __FILE__, __LINE__)
> > 
> > With the patched gcc in the unstable reproducible builds setting
> > __FILE__ to something other value, this does no longer find the
> > test data.
> > 
> > I do not really see any reason for blaming the users here for using
> > a documented public Qt API for accesssing test data in the source
> > directory:
> >   http://doc.qt.io/qt-5/qtest.html#QFINDTESTDATA
> > 
> > I've added reproducible-builds at lists.alioth.debian.org to Cc for giving 
> > input what a reproducible and portable implementation might be for Qt.
> > 
> 
> Our patched dpkg tells GCC to map $PWD to "$srcpkg-$version" when
> expanding __FILE__. That is the source of the problem, because this
> path doesn't exist at test-time.

Exactly, this is the source of the problem.  OTOH, the problem was
created by changing __FILE__: it has a well-defined meaning:
https://gcc.gnu.org/onlinedocs/cpp/Standard-Predefined-Macros.html
changing this behaviour is only going to create problems, because
people rely on such well-defined (and standard) behaviours.

QFINDTESTDATA is a macro used mostly (if not only) in unit tests, so
the binaries built with it are usually not installed.  QFINDTESTDATA
uses __FILE__ as one of the methods to locate files in the source
directory of the file being built.  AFAICS, this usage is *valid*, so
the reproducible changes done to __FILE__ are only a regression.

> The problem stems from the fact that we assume __FILE__ represents a
> build-time path and not a run-time path, so we rewrite it
> indiscriminately with BUILD_PATH_PREFIX_MAP.

Exactly -- but in the case of QFINDTESTDATA this is wanted.

> This assumption is broken in the specific case where you have some
> source code that uses __FILE__, that are specifically only meant to
> be run at build-time, so that they are in fact run-time paths (that
> are also build-time paths). The assumption is fine in all other cases.

So this is fine for QFINDTESTDATA.

> Therefore, the only solution to fix this problem, that also preserves
> reproducible builds, is to make those tests *not use __FILE__*. Or
> option (2), which makes the non-existent rewritten paths, into an
> actually-existing build-time path.

No, the solution is:
a) *not* break what __FILE__ means
b) remove the misuses of __FILE__ in packages (not the case of
   QFINDTESTDATA)

> I am not "blaming the user", but pointing to the fact that __FILE__
> is being used in a surprising way here, which is not good for
> reproducible builds.

What I see it is happening here is: you (= people working on
reproducible builds) see __FILE__, and the problems that arise from its
abuse; to overcome this issue, you use the sledgehammer solution,
basically changing what __FILE__ means, and thus breaking even valid
use cases.  Sorry, but I do not see how this is useful.

A better approach here is to work on removing the invalid & abusing
usages of __FILE__ from packages, just like it was done for __DATE__.

> An analogy would be to write your program to execute something at
> time "__DATE__ + 30 seconds". This is obviously ridiculous, but works
> "by accident" if done inside a test case.

This is clearly a misuse, and thus it must be fixed.  OTOH, the
comparison with __FILE__ is not appropriate.

-- 
Pino Toscano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.alioth.debian.org/pipermail/reproducible-builds/attachments/20171113/537dacda/attachment.sig>