Bug#876901: QFINDTESTDATA uses __FILE__

[Pino Toscano]

In data lunedì 13 novembre 2017 20:03:00 CET, Ximin Luo ha scritto:
> Pino Toscano:
> > [..]
> > 
> > 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.
> > 
> Why's it not appropriate?

Because using __DATE__ as timing for a test is extremely fragile, and
it has nothing to do with reproducible builds.  It is just wrong, and
as such __DATE__ is used wrongly.

OTOH, as I already explained, at least what QFINDTESTDATA does with
__FILE__ is perfectly valid and acceptable, since it is about stuff
run during test suites at build time only.  Again, see:
http://doc.qt.io/qt-5/qtest.html#QFINDTESTDATA

> If you ever want to write tests to be runnable outside the build,
> e.g. with autopkgtest, then you're going to have to not use __FILE__
> anyway.

Sure, and it is exactly what I said in point (b) of the solution I
proposed above.  

> (Assuming you install the tests somewhere, rather than
> running the whole build again.)

This will not work with tests using QFINDTESTDATA anyway, as it works
with either the source or build directory of the source using it, or
with the Qt installation prefix.  (Hint: none of them is useful for an
installed test.)

> I can understand that breaking something that used to work is
> annoying,

This is not an annoyance, it is the crux of the problem!  __FILE__ is
something standard, with a very well defined behaviour, upon which
people rely on: you cannot trash it from one day to another like this,
saying "too bad" to all its users, even those using it appropriately.
It does not simply work.

> but the point is that the previous use does not generalise well.

To be honest: I already said multiple times that abuses of __FILE__
*must* be fixed.  However, there are valid use cases, and in this case
the only generalization is done by you, who declared __FILE__ as
"absolutely BAD".

> It's not a sledgehammer solution, it's tweakable and we can try to
> use its flexibility to un-break your tests.

No, it *is* a sledgehammer solution, since you are not even checking
what __FILE__ is used for, and thus blindly changing it without any
pre-research on its effect.  Again, this is not the correct approach
to fix the issue at hand.

> So, what do you think of this alternate solution, that I suggested:

Any of your solutions get a big, fat, and giant nope.

> If this isn't suitable to you, how else do you suppose we should try
> to make builds independent of (i.e. not embed) the build path?
> Suggestions are welcome.

I already wrote it in my email, and I repeat it again:

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

-- 
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/872761cf/attachment.sig>