D22727: allow kio-extras to build with mingw on win32 and remove unnecessary includes

[David Faure]

dfaure added a comment.


  I suggest do look at kio_file (kio/src/ioslaves/file), the above code was 
inspired by kio_file, and then ported differently to Windows.

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D22727

To: brute4s99, vonreth, dfaure, pino
Cc: sredman, pino, kde-frameworks-devel, kfm-devel, iasensio, fprice, 
LeGast00n, MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew, meven, 
michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, mikesomov

D22727: allow kio-extras to build with mingw on win32 and remove unnecessary includes

[Piyush Aggarwal]

brute4s99 added a comment.


  we need this because the symbols being currently used are Unix-only. Porting 
to Qt supplied ones allows this plugin to work on other OSes (my concern: 
Windows) as well. 

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D22727

To: brute4s99, vonreth, dfaure, pino
Cc: sredman, pino, kde-frameworks-devel, kfm-devel, iasensio, fprice, 
LeGast00n, MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew, meven, 
michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, mikesomov

D22727: allow kio-extras to build with mingw on win32 and remove unnecessary includes

[David Faure]

dfaure added a comment.


  I'm not sure what the QFileDevice enum gives us compared to the octal 
permissions as int? Is this just because it looks nicer, to save a conversion, 
or because you actually need the differenciation between ReadUser and 
ReadOwner, on Windows?

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D22727

To: brute4s99, vonreth, dfaure, pino
Cc: sredman, pino, kde-frameworks-devel, kfm-devel, iasensio, fprice, 
LeGast00n, MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew, meven, 
michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, mikesomov

D22727: allow kio-extras to build with mingw on win32 and remove unnecessary includes

[Piyush Aggarwal]

brute4s99 added a comment.


  In D22727#513842 , @dfaure wrote:
  
  > The truth is stronger than "I would not recommend".
  >  put() in SlaveBase-derived classes is called by the KIO library 
(TransferJob), so you CANNOT change the meaning of the arguments. It's part of 
the API/ABI for all slaves, and this cannot change until KF6.
  
  
  can you please add it to https://phabricator.kde.org/tag/kf6/ ?

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D22727

To: brute4s99, vonreth, dfaure, pino
Cc: sredman, pino, kde-frameworks-devel, kfm-devel, iasensio, fprice, 
LeGast00n, MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew, meven, 
michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, mikesomov

D22727: allow kio-extras to build with mingw on win32 and remove unnecessary includes

[Simon Redman]

sredman added a comment.


  In D22727#513842 , @dfaure wrote:
  
  > The truth is stronger than "I would not recommend".
  >  put() in SlaveBase-derived classes is called by the KIO library 
(TransferJob), so you CANNOT change the meaning of the arguments. It's part of 
the API/ABI for all slaves, and this cannot change until KF6.
  
  
  +1
  
  But it is worth making a note of this as something to change (or discuss 
changing) for KF6 since it seems like using the Qt constants would be a better 
design

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D22727

To: brute4s99, vonreth, dfaure, pino
Cc: sredman, pino, kde-frameworks-devel, kfm-devel, aprcela, vmarinescu, 
fprice, LeGast00n, MrPepe, fbampaloukas, alexde, Codezela, feverfew, meven, 
michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, mikesomov

D22727: allow kio-extras to build with mingw on win32 and remove unnecessary includes

[David Faure]

dfaure added a comment.


  The truth is stronger than "I would not recommend".
  put() in SlaveBase-derived classes is called by the KIO library 
(TransferJob), so you CANNOT change the meaning of the arguments. It's part of 
the API/ABI for all slaves, and this cannot change until KF6.

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D22727

To: brute4s99, vonreth, dfaure, pino
Cc: sredman, pino, kde-frameworks-devel, kfm-devel, aprcela, vmarinescu, 
fprice, LeGast00n, MrPepe, fbampaloukas, alexde, Codezela, feverfew, meven, 
michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, mikesomov

D22727: allow kio-extras to build with mingw on win32 and remove unnecessary includes

[Simon Redman]

sredman added a comment.


  In D22727#510698 , @brute4s99 
wrote:
  
  > In D22727#505540 , @dfaure wrote:
  >
  > > But, wait, this code is mixing "int permissions" (*) with the QFileDevice 
enum, that doesn't make any sense to me.
  > >
  > > (*) this comes from KIO::put, which takes unix permissions on unix, not 
sure what it takes on Windows...
  > >
  > > This doesn't match: unix permissions are octal (e.g. group read is 040 in 
octal), QFileDevice enum is hex (0x040).
  >
  >
  > Okay. I'm still unable to understand where/ how sftp::put() is called, or I 
would change everywhere it is called, to Qt way of permission extraction.
  
  
  I would recommend not making API or ABI changes. It's annoying that they're 
not compatible, but you can probably find some way to convert from Qt 
permissions to the expected values in order to not change users

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D22727

To: brute4s99, vonreth, dfaure, pino
Cc: sredman, pino, kde-frameworks-devel, kfm-devel, aprcela, vmarinescu, 
fprice, LeGast00n, MrPepe, fbampaloukas, alexde, Codezela, feverfew, meven, 
michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, mikesomov

D22727: allow kio-extras to build with mingw on win32 and remove unnecessary includes

[Piyush Aggarwal]

brute4s99 added a comment.


  In D22727#505540 , @dfaure wrote:
  
  > The Qt documentation says "Warning: Because of differences in the platforms 
supported by Qt, the semantics of ReadUser, WriteUser and ExeUser are 
platform-dependent: On Unix, the rights of the owner of the file are returned 
and on Windows the rights of the current user are returned. "
  
  
  okay, I believe you mean I should leave these changes as-are in the diff. Am 
I right?
  
  > But, wait, this code is mixing "int permissions" (*) with the QFileDevice 
enum, that doesn't make any sense to me.
  > 
  > (*) this comes from KIO::put, which takes unix permissions on unix, not 
sure what it takes on Windows...
  > 
  > This doesn't match: unix permissions are octal (e.g. group read is 040 in 
octal), QFileDevice enum is hex (0x040).
  
  Okay. I'm still unable to understand where/ how sftp::put() is called, or I 
would change everywhere it is called, to Qt way of permission extraction.

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D22727

To: brute4s99, vonreth, dfaure, pino
Cc: pino, kde-frameworks-devel, kfm-devel, aprcela, vmarinescu, fprice, 
LeGast00n, fbampaloukas, alexde, feverfew, meven, michaelh, spoorun, 
navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, mikesomov

D22727: allow kio-extras to build with mingw on win32 and remove unnecessary includes

[David Faure]

dfaure requested changes to this revision.
dfaure added a comment.


  The Qt documentation says "Warning: Because of differences in the platforms 
supported by Qt, the semantics of ReadUser, WriteUser and ExeUser are 
platform-dependent: On Unix, the rights of the owner of the file are returned 
and on Windows the rights of the current user are returned. "
  
  But, wait, this code is mixing "int permissions" (*) with the QFileDevice 
enum, that doesn't make any sense to me.
  
  (*) this comes from KIO::put, which takes unix permissions on unix, not sure 
what it takes on Windows...
  
  This doesn't match: unix permissions are octal (e.g. group read is 040 in 
octal), QFileDevice enum is hex (0x040).

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D22727

To: brute4s99, vonreth, dfaure, pino
Cc: pino, kde-frameworks-devel, kfm-devel, aprcela, vmarinescu, fprice, 
LeGast00n, sbergeron, fbampaloukas, alexde, feverfew, meven, michaelh, spoorun, 
navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, mikesomov

D22727: allow kio-extras to build with mingw on win32 and remove unnecessary includes

[Piyush Aggarwal]

brute4s99 added inline comments.

INLINE COMMENTS

> pino wrote in kio_sftp.cpp:383
> sounds like you need to use the *User enums, not the *Owner ones

https://www.gnu.org/software/libc/manual/html_node/Permission-Bits.html

The link says these bits refer to the owner of the file. Should I still change 
these to *User ?

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D22727

To: brute4s99, vonreth, dfaure, pino
Cc: pino, kde-frameworks-devel, kfm-devel, aprcela, fprice, LeGast00n, 
sbergeron, fbampaloukas, alexde, feverfew, meven, michaelh, spoorun, 
navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, mikesomov

D22727: allow kio-extras to build with mingw on win32 and remove unnecessary includes

[Pino Toscano]

pino requested changes to this revision.
pino added a comment.
This revision now requires changes to proceed.


  other than building, please also check that it actually still works on Linux

INLINE COMMENTS

> kio_sftp.cpp:383
>  fileType = QT_STAT_MASK - 1;
> -#ifdef Q_OS_WIN
> -access = static_cast(perms::owner_all | perms::group_all | 
> perms::others_all);
> -#else
> -access = S_IRWXU | S_IRWXG | S_IRWXO;
> -#endif
> +access = QFileDevice::Permission::ReadOwner | 
> QFileDevice::Permission::WriteOwner | QFileDevice::Permission::ExeOwner
> +| QFileDevice::Permission::ReadGroup | 
> QFileDevice::Permission::WriteGroup | QFileDevice::Permission::ExeGroup

sounds like you need to use the *User enums, not the *Owner ones

> kio_sftp.cpp:1757
>  if (permissions != -1) {
> -#ifdef Q_OS_WIN
> -initialMode = permissions | 
> static_cast(perms::owner_write | perms::owner_read);
> -#else
> -initialMode = permissions | S_IWUSR | S_IRUSR;
> -#endif
> +initialMode = permissions | 
> QFileDevice::Permission::WriteOwner | QFileDevice::Permission::ReadOwner;
>  } else {

ditto

> kio_sftp.cpp:1964
>  if (permissions != -1)
> -#ifdef Q_OS_WIN
> -initialMode = permissions | static_cast(perms::owner_write);
> -#else
> -initialMode = permissions | S_IWUSR;
> -#endif
> +initialMode = permissions | QFileDevice::Permission::WriteOwner;
>  else

ditto

> kio_sftp.cpp:
>  fileType = QT_STAT_MASK - 1;
> -#ifdef Q_OS_WIN
> -access = static_cast(perms::owner_all | perms::group_all 
> | perms::others_all);
> -#else
> -access = S_IRWXU | S_IRWXG | S_IRWXO;
> -#endif
> +access = QFileDevice::Permission::ReadOwner | 
> QFileDevice::Permission::WriteOwner | QFileDevice::Permission::ExeOwner
> +| QFileDevice::Permission::ReadGroup | 
> QFileDevice::Permission::WriteGroup | QFileDevice::Permission::ExeGroup

ditto

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D22727

To: brute4s99, vonreth, dfaure, pino
Cc: pino, kde-frameworks-devel, kfm-devel, aprcela, fprice, LeGast00n, 
sbergeron, fbampaloukas, alexde, feverfew, meven, michaelh, spoorun, 
navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, mikesomov

D22727: allow kio-extras to build with mingw on win32 and remove unnecessary includes

[Piyush Aggarwal]

brute4s99 added a comment.


  In D22727#501824 , @vonreth wrote:
  
  > Looks good.
  >  Why not the first time 
  
  
  I was afraid of reverts 

REPOSITORY
  R320 KIO Extras

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D22727

To: brute4s99, vonreth, dfaure
Cc: kde-frameworks-devel, kfm-devel, aprcela, fprice, LeGast00n, sbergeron, 
fbampaloukas, alexde, feverfew, meven, michaelh, spoorun, navarromorales, 
firef, ngraham, andrebarros, bruns, emmanuelp, mikesomov

D22727: allow kio-extras to build with mingw on win32 and remove unnecessary includes

[Hannah von Reth]

vonreth accepted this revision.
vonreth added a reviewer: dfaure.
vonreth added a comment.
This revision is now accepted and ready to land.


  Looks good.
  Why not the first time 

REPOSITORY
  R320 KIO Extras

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D22727

To: brute4s99, vonreth, dfaure
Cc: kde-frameworks-devel, kfm-devel, aprcela, fprice, LeGast00n, sbergeron, 
fbampaloukas, alexde, feverfew, meven, michaelh, spoorun, navarromorales, 
firef, ngraham, andrebarros, bruns, emmanuelp, mikesomov

D22727: allow kio-extras to build with mingw on win32 and remove unnecessary includes

[Piyush Aggarwal]

brute4s99 created this revision.
brute4s99 added a reviewer: vonreth.
Herald added projects: Dolphin, Frameworks.
Herald added subscribers: kfm-devel, kde-frameworks-devel.
brute4s99 requested review of this revision.

TEST PLAN
  should build on windows with mingw as compiler
  
  should build on linux with gcc

REPOSITORY
  R320 KIO Extras

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D22727

AFFECTED FILES
  sftp/kio_sftp.cpp

To: brute4s99, vonreth
Cc: kde-frameworks-devel, kfm-devel, aprcela, fprice, LeGast00n, sbergeron, 
fbampaloukas, alexde, feverfew, meven, michaelh, spoorun, navarromorales, 
firef, ngraham, andrebarros, bruns, emmanuelp, mikesomov