Compilation fixed with Qt < 5.11
ClosedPublic

Authored by jjazeix on Mar 28 2019, 8:49 PM.

Details

Reviewers
nmel
Group Reviewers
Krusader
Summary

The CMakeLists.txt requires Qt5.5 minimal but QIODevice::NewOnly is Qt5.11 so it fails to compile with previous version (https://build.kde.org/job/Extragear/job/krusader/job/kf5-qt5%20SUSEQt5.10/ for example).
As I understand in the code, the aim is only to create the file if it does not exist.
According to https://doc.qt.io/qt-5/qfile.html#open: "Note: In WriteOnly or ReadWrite mode, if the relevant file does not already exist, this function will try to create a new file before opening it." so it should do the same.

Diff Detail

Repository
R167 Krusader
Lint
Lint Skipped
Unit
Unit Tests Skipped
jjazeix requested review of this revision.Mar 28 2019, 8:49 PM
jjazeix created this revision.
nmel added a subscriber: nmel.Mar 29 2019, 7:25 AM

I agree that WriteOnly is the best approximation to the NewOnly for older Qt. However, NewOnly is more advanced:

https://doc.qt.io/qt-5/qiodevice.html#OpenModeFlag-enum

QIODevice::WriteOnly 0x0002 The device is open for writing. Note that, for file-system subclasses (e.g. QFile), this mode implies Truncate unless combined with ReadOnly, Append or NewOnly.

QIODevice::NewOnly 0x0040 Fail if the file to be opened already exists. Create and open the file only if it does not exist. There is a guarantee from the operating system that you are the only one creating and opening the file.

i.e. it's atomic operation check existence + create.

I suggest we branch on Qt version. If >= 5.11 we do NewOnly, otherwise WriteOnly.

FYI, we started a discussion about raising min version higher here:

In D19623#427671, @nmel wrote:

Good point, Yuri. We should look into other cases as well.

IMO, QOverload looks cleaner. Why don't we raise minimal requirements to qt-5.7 or even qt-5.9 LTS, since <= qt-5.6 support will expire in a few days, and work around cases that don't compile with minimal version?

5.6 support has already expired, so the 5.9 is the best candidate for min version IMO. I'm going to create a PR for that over the weekend.

jjazeix updated this revision to Diff 55016.Mar 29 2019, 7:38 AM

Thanks,

I'm not planning on being an active contributor, this PR is just to fix the CI. I let the Krusader handle the minimal required Qt version :)

nmel accepted this revision.Mar 29 2019, 7:53 AM
This revision is now accepted and ready to land.Mar 29 2019, 7:53 AM