tst_xdgdesktopfile fails

yan12125 commented 4 months ago

Expected Behavior

All tests pass

Current Behavior

tst_xdgdesktopfile fails:

$ ./build/test/tst_xdgdesktopfile
********* Start testing of tst_xdgdesktopfile *********
Config: Using QtTest library 6.7.0, Qt 6.7.0 (x86_64-little_endian-lp64 shared (dynamic) release build; by GCC 13.2.1 20230801), arch unknown
PASS   : tst_xdgdesktopfile::initTestCase()
FAIL!  : tst_xdgdesktopfile::testRead() 'df.isValid()' returned FALSE. ()
   Loc: [/home/yen/Computer/lxqt/libqtxdg/test/tst_xdgdesktopfile.cpp(64)]
FAIL!  : tst_xdgdesktopfile::testReadLocalized(pt) 'df.isValid()' returned FALSE. ()
   Loc: [/home/yen/Computer/lxqt/libqtxdg/test/tst_xdgdesktopfile.cpp(108)]
FAIL!  : tst_xdgdesktopfile::testReadLocalized(pt_PT) 'df.isValid()' returned FALSE. ()
   Loc: [/home/yen/Computer/lxqt/libqtxdg/test/tst_xdgdesktopfile.cpp(108)]
FAIL!  : tst_xdgdesktopfile::testReadLocalized(pt_BR) 'df.isValid()' returned FALSE. ()
   Loc: [/home/yen/Computer/lxqt/libqtxdg/test/tst_xdgdesktopfile.cpp(108)]
FAIL!  : tst_xdgdesktopfile::testReadLocalized(de) 'df.isValid()' returned FALSE. ()
   Loc: [/home/yen/Computer/lxqt/libqtxdg/test/tst_xdgdesktopfile.cpp(108)]
PASS   : tst_xdgdesktopfile::cleanupTestCase()
Totals: 2 passed, 5 failed, 0 skipped, 0 blacklisted, 2ms
********* Finished testing of tst_xdgdesktopfile *********

I remember all tests can pass when Qt 6 support was still in a WIP branch. Maybe something changed in Qt

Possible Solution

Not sure

Steps to Reproduce (for bugs)

cmake -B build -D BUILD_TESTS=Yes
make -C build
./build/test/tst_xdgdesktopfile

Context

Testing Qt 6 migration for LXQt

System Information

Distribution & Version: Arch Linux up-to-date
Kernel: 6.8.4-arch1-1
Qt Version: 6.7.0-2
cmake Version: 3.29.2
lxqt-build-tools Version: git
Package version: git

tsujan commented 4 months ago

Confirming.

The tests will pass if I use QDir::tempPath() when making QTemporaryFile in tst_xdgdesktopfile.cpp, like:

    QTemporaryFile file(QDir::tempPath() + QStringLiteral("/testReadXXXXXX.desktop"));

But it needs more investigation.

I remember all tests can pass when Qt 6 support was still in a WIP branch.

There is a change in qtemporaryfile.cpp → QTemporaryFileName::QTemporaryFileName from Qt 6.7, but I don't know if it's relevant.

tsujan commented 4 months ago

OK, this is a new Qt bug because Qt doc still says that the path will be relative to the current working directory in QTemporaryFile::QTemporaryFile(const QString&) if it isn't given, while that's not true anymore. With Qt5, I get a real path with fileName(), but with Qt 6.7 I just get something like "testReadttIVSx.desktop": no path at all.

We can do as I suggested in my previous comment, or we can leave it to Qt. @yan12125?

marcusbritanicus commented 4 months ago

Confirming.

The tests will pass if I use QDir::tempPath() when making QTemporaryFile in tst_xdgdesktopfile.cpp, like:
    QTemporaryFile file(QDir::tempPath() + QStringLiteral("/testReadXXXXXX.desktop"));

I think it would be slightly more efficient if we use

     QTemporaryFile file(QDir::temp().filePath( QStringLiteral("testReadXXXXXX.desktop")));

tsujan commented 4 months ago

I think it would be slightly more efficient if we use

Yes, but the question is whether we want to hide this Qt regression in a test file. It doesn't affect functionality.

yan12125 commented 4 months ago

There is a change in qtemporaryfile.cpp → QTemporaryFileName::QTemporaryFileName from Qt 6.7, but I don't know if it's relevant.

Do you mean https://codereview.qt-project.org/c/qt/qtbase/+/512780? Yes that commit appears related. The commit message says:

This class will now return relative file paths in fileName() if the file template was also a relative path (it used to always return an absolute path).

From the commit message, it seems an intended change. The only upstream issue might be missing updates for documentation. The change also appears in release notes: https://code.qt.io/cgit/qt/qtreleasenotes.git/tree/qt/6.7.0/release-note.md, so it's likely libqtxdg needs to adapt to it.

By the way, Qt's own project is also affected, and they chose to adapt to it: https://codereview.qt-project.org/c/qt/qtapplicationmanager/+/514646

tsujan commented 4 months ago

Do you mean https://codereview.qt-project.org/c/qt/qtbase/+/512780?

Yes, thanks! I download Qt's sources and search in them offline when needed; hence not knowing about commit links.

By the way, Qt's own project is also affected,

So, we could also do so. Will make a PR before releasing.

tsujan commented 4 months ago

Did it in the way @marcusbritanicus suggested: https://github.com/lxqt/libqtxdg/pull/303

lxqt / libqtxdg