> On May 4, 2017, 12:40 p.m., Albert Astals Cid wrote: > > Lamarque, you broke the build. > > Lamarque Souza wrote: > Fixed. Thanks for the quick report about the broken build and sorry for > not adding all files to the commit. > > Ben Cooksley wrote: > This patch broke the MSVC build and will be reverted shortly. > Please see > https://build-sandbox.kde.org/job/Frameworks%20solid%20kf5-qt5%20WindowsQt5.7/2/consoleText > for the build log. > > Lamarque Souza wrote: > Well, the simpler solution is removing all those #if, #include and #error > clauses (lines 31 oto 39 of autotests/solidudisks2test.cpp), they are not > required. I do not have a MSVC machine with frameworks installed to test that > now. I will test that after next frameworks release. > > KJ Tsanaktsidis wrote: > They're needed to actually find the right header file to include for > these macros on the various BSD's I think. Actually I think we should just > disable this test for windows, this feature is totally meaningless on > windows. Another hacky option is to code in the fallback macros (including > makedev) in solidudisks2test.cpp in the same way they are defined in > udisksblock.cpp, which will probably make that test compile and "work". It > wouldn't really be testing anything on windows though. > > The original reason I put the #error case in that test was to make sure > we found out if there was some platform that was supposed to be supported > that didn't have these macros in any of the places I looked for them. I guess > we found out.
If you post a revised patch i'm happy to test it on the Windows CI setup. - Ben ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/130090/#review103182 ----------------------------------------------------------- On May 4, 2017, 12:32 p.m., KJ Tsanaktsidis wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/130090/ > ----------------------------------------------------------- > > (Updated May 4, 2017, 12:32 p.m.) > > > Review request for KDE Frameworks. > > > Repository: solid > > > Description > ------- > > Previously, udesksblock.cpp was attempting to find a definition for > major/minor on Linux in <sys/kdev_t.h> by checking Q_OS_LINUX before > importing the header. Q_OS_LINUX is however only set when > qsystemdetection.h is included, and the macro was being checked first. > > Even had this check worked, it would still be wrong. On a modern version > of the userspace linux-headers, <sys/kdev_t.h> includes definitions for > major and minor that assume each is limited to 8 bits and that dev_t is > 16 bits. This is no longer true anymore; on Linux, major numbers can be > up to 12 bits at present and minor numbers up to 20. Calling these > macros with dev_t values > 2^16 would give incorrect results. > > Because the Q_OS_LINUX check failed, a fallback version of the macros > were defined for use on all platforms. The code is allegedly copied from > kdev_t.h, except it is copied from the *kernel* version of the header, > not the userspace version. Linux internally uses a different > representation of dev_t than it exposes to userspace - the kernelspace > version is 20 bits of minor/12 bits of major contiguously, but the > userspace version packs the bits in a different order to maintain > compatability with old 16-bit device numbers. Thus, this code also does > not work for dev_t values > 2^16. > > To fix this, we add CMake rules to search for a system-provided > definition of the major/minor macros - on various systems, these can be > in a few different places. As a fallback, we assume old-style 16-bit > dev_t (although I suspect that is only used for Windows, where > major/minor numbers are pretty meaningless anyway). > > > Diffs > ----- > > autotests/CMakeLists.txt 54adeea62b954b9169b37f1eab8fa3e215fafafa > autotests/fakeUdisks2.h PRE-CREATION > autotests/fakeUdisks2.cpp PRE-CREATION > autotests/solidudisks2test.cpp PRE-CREATION > src/solid/devices/backends/udisks2/CMakeLists.txt > 34390064af29ace07cbb3470945be098cc606d04 > src/solid/devices/backends/udisks2/udisksblock.cpp > 0622ec77fcf670a2005d34b7a6c31ca8b53a18d8 > > Diff: https://git.reviewboard.kde.org/r/130090/diff/ > > > Testing > ------- > > I've written a little snippet to iterate through block devices, print their > major/minor number, and their device properties. It was previously > incorrectly labeling all my disks with major 0 and minor == device_number > (since it was using the first 20 bits for the minor). It now correctly > identifies their major/minor number. > > > Thanks, > > KJ Tsanaktsidis > >