| dfaure | |
| fvogt | |
| bruns | |
| broulik |
| Frameworks |
The birthtime can be retrieved using using the statx function, available since glibc 2.28. In case the kernel lacks support for the underlying syscall, glibc falls back to stat internally. The validity of the btime field is indicated in the mask field, e.g. when the kernel or the filesystem lacks support for btime.
For details, see http://man7.org/linux/man-pages/man2/statx.2.html
In dolphin, the creation time column is now filled in linux :
FEATURE: 381367
FIXED-IN: 5.58
Tested using kioslavetest and local build of dolphin.
Tested without HAVE_STATX under linux.
| Automatic diff as part of commit; lint not applicable. |
| Automatic diff as part of commit; unit tests not applicable. |
| src/ioslaves/file/file.cpp | ||
|---|---|---|
| 1027 | Thanks for the feedback, I read about stx_mask after writing this. | |
I think the code itself is good to go now.
Can you please update the summary somewhat (and better do it directly in phabricator, as arc will throw away any changes in the commit message, unless you do a - arc amend, update the commit message in git, arc diff --verbatim - cycle).
The birthtime can be retrieved using using the statx function, available since glibc 2.28. In case the kernel lacks support for the underlying syscall, glibc falls back to stat internally. The validity of the btime field is indicated in the mask field, e.g. when the kernel or the filesystem lacks support for btime. For details, see <manpage url>.
Yeah, we should land this after 5.57 tagging so we have a month of pre-release testing, so it should be FIXED-IN: 5.58.
| src/ioslaves/file/file.cpp | ||
|---|---|---|
| 72–74 | TBH, instead of this static define, I'd do a proper cmake check (see ConfigureChecks.cmake, and config-kioslave-file.h.cmake in src/ioslaves/file). | |
BTW Frameworks 5.57 has been tagged, so whenever folks are good with this, it can be landed at any time. :)
Use proper cmake check, fix an issue with KIO::UDSEntry::UDS_DEVICE_ID having value of stat_mode instead of stat_dev
| src/ioslaves/file/file.cpp | ||
|---|---|---|
| 72–74 | It was not too easy to do. | |
I think this would be best to wait for this for 5.58 to have a month of testing as you suggested.
| src/ioslaves/file/ConfigureChecks.cmake | ||
|---|---|---|
| 20 | I am not sure this is acceptable, this variable is set by ECM in extra-cmake-modules/kde-modules/KDECompilerSettings.cmake | |
The month of testing starts today. Code pushed to master starting from today will end up in 5.58.
| src/ioslaves/file/ConfigureChecks.cmake | ||
|---|---|---|
| 20 | just do the test unconditionally | |
| 25 | other than this define, try to do a simple usage of statx: struct statx buf; statx(AT_FDCWD, "/foo", AT_EMPTY_PATH, STATX_BASIC_STATS, &buf); (it will not be run anyway) | |
| src/ioslaves/file/file.cpp | ||
| 68 | no need for the Q_OS_LINUX check here | |
| src/ioslaves/file/ConfigureChecks.cmake | ||
|---|---|---|
| 20 | sorry, i realized that my sentence was ambiguous: i meant to check for the existance of statx no matter the platform | |
| src/ioslaves/file/ConfigureChecks.cmake | ||
|---|---|---|
| 20 | We discussed previously that we would restrict using statx to the platform where it was tested i.e GLIBC to avoid future bugs with different potential different implementation of statx. | |
| src/ioslaves/file/file.cpp | ||
|---|---|---|
| 850–870 | We can get all stat_xxx functions buf as reference, struct is not needed when you use C++. | |
| src/ioslaves/file/file.cpp | ||
|---|---|---|
| 850–870 | Do you mean the struct keyword in the argument in "inline static uint16_t stat_mode(struct statx buf) { return buf.stx_mode; } " for instance ? | |
| src/ioslaves/file/file.cpp | ||
|---|---|---|
| 850–870 | Unfortunately this is not possible here : statx is also a function, the compiler gets messed up when removing the struct keyword interpreting it as a function call.
| |
| src/ioslaves/file/file.cpp | ||
|---|---|---|
| 850–870 | No, he means using a const& for the argument, e.g: inline static uint16_t stat_mode(struct statx &buf) { return buf.stx_mode; } | |
| src/ioslaves/file/file.cpp | ||
|---|---|---|
| 850–870 | Thanks | |
| src/ioslaves/file/file.cpp | ||
|---|---|---|
| 890 | Thanks | |
I think its better go give it sufficient testing time than to wait for more comments. The code has been around for sufficient time, and the last changes were just minor.
Thanks for working on this!
I am very glad :)
Thank you for your great reviews. I learned quite a bit along the way.
There is still a couple raw QT_LSTAT in KFileItem https://cgit.kde.org/kio.git/tree/src/core/kfileitem.cpp#n224 .
Do you think we should apply the same kind of solution there, with #idef ?
I would be in favor of reusing code instead.
Eventually moving FileProtocol::createUDSEntry out of FileProtocol in some other util so that it can be reused by KFileitem.
MSVC doesn't like this change i'm afraid - see https://build.kde.org/view/OS%20-%20Windows/job/Frameworks/job/kio/job/kf5-qt5%20WindowsMSVCQt5.11/259/console
Looks like you're making use of C++17 features?
Nah, no C++17 here, the compiler is just confused because it doesn't know the types uid_t and gid_t.
I'll add some #ifndef Q_OS_WIN.
I am a bit surprised. I used std:c++11 as for the rest of current kde compli
@bcooksley
Could you test https://phabricator.kde.org/D20599 ?
This change also introduced regressions in two unittests: jobtest and kdirmodeltest. I'll let you look into those :-)
https://build.kde.org/view/OS%20-%20Windows/job/Frameworks/job/kio/job/kf5-qt5%20SUSEQt5.10/87/testReport/junit/projectroot/autotests/kiocore_jobtest/
https://build.kde.org/view/OS%20-%20Windows/job/Frameworks/job/kio/job/kf5-qt5%20SUSEQt5.10/87/testReport/projectroot/autotests/kiowidgets_kdirmodeltest/
The first one is probably just an extra file in the dir (I know, bad isolation of the tests), the second one is more.... unexpected.
The tests failures are not regressions as they are happened before the merge : https://build.kde.org/view/OS%20-%20Windows/job/Frameworks/job/kio/job/kf5-qt5%20SUSEQt5.12/72/testReport/ although on Qt 5.12.
Failures are:
FAIL! : KFileItemTest::testListProperties(two (text+html) files) Compared values are not the same
Actual (props.mimeGroup()): "" Expected (expectedMimeGroup): "text" Loc: [/home/jenkins/workspace/Frameworks/kio/kf5-qt5 SUSEQt5.10/autotests/kfileitemtest.cpp(590)]
I don't reproduce locally.
FAIL! : KDirListerTest::testRenameItem() Compared values are not the same
Actual (entry.second.mimetype()): "application/xhtml+xml"
Expected (QString("text/html")) : "text/html"
Loc: [/home/jenkins/workspace/Frameworks/kio/kf5-qt5 SUSEQt5.10/autotests/kdirlistertest.cpp(557)]I didn't reproduce locally, but I could see some issue in the test.
Here is a fix proposal for this test : D20692
Is it possible to test it on jenkins before merging ?
So it seems to me as sporadic failures of slightly unconsistent tests.
I'm talking about jobtest and kdirmodeltest regressing exactly in build 87, which is where this commit landed.
If you click on history for a given test you can see that those aren't sporadic failures: https://build.kde.org/view/OS%20-%20Windows/job/Frameworks/job/kio/job/kf5-qt5%20SUSEQt5.10/87/testReport/junit/projectroot/autotests/kiowidgets_kdirmodeltest/history/
and
https://build.kde.org/view/OS%20-%20Windows/job/Frameworks/job/kio/job/kf5-qt5%20SUSEQt5.10/87/testReport/junit/projectroot/autotests/kiocore_jobtest/history/
The failures in kfileitemtest and kdirlistertest (note: model != lister) are unrelated, I'll look into those.
Thank you for making this clear.
The regression are associated with symlink resolution.
I think I have identified the issue.
Please see D20694 and could we test this on CI.
It makes me think that for some reviews, it would be good to test it on CI before merging.
At my past place of work we used this extensively, it was quite helpful.