Re: Review Request 124542: CMake fixes for Windows build
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124542/ --- (Updated Aug. 10, 2015, 6:13 a.m.) Status -- This change has been marked as submitted. Review request for Documentation, KDE Frameworks and Luigi Toscano. Changes --- Submitted with commit 38265304276e6305f72a7e1a68aa1b4193657820 by Kevin Funk to branch master. Bugs: 348061 https://bugs.kde.org/show_bug.cgi?id=348061 Repository: kdoctools Description --- BUG: 348061 Diffs - cmake/uriencode.cmake e5f3c3acd93d3871e44b6e6fb29ad7113e18d751 Diff: https://git.reviewboard.kde.org/r/124542/diff/ Testing --- Adding ':' to the list of escaped characters is probably not an ideal solution, but let me hear your ideas. Thanks, Kevin Funk ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 124542: CMake fixes for Windows build
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124542/#review83574 --- @Luigi: Bump? - Kevin Funk On Aug. 6, 2015, 6:37 a.m., Kevin Funk wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124542/ --- (Updated Aug. 6, 2015, 6:37 a.m.) Review request for Documentation, KDE Frameworks and Luigi Toscano. Bugs: 348061 https://bugs.kde.org/show_bug.cgi?id=348061 Repository: kdoctools Description --- BUG: 348061 Diffs - cmake/uriencode.cmake e5f3c3acd93d3871e44b6e6fb29ad7113e18d751 Diff: https://git.reviewboard.kde.org/r/124542/diff/ Testing --- Adding ':' to the list of escaped characters is probably not an ideal solution, but let me hear your ideas. Thanks, Kevin Funk ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 124542: CMake fixes for Windows build
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124542/#review83576 --- Ship it! Inviala! - Luigi Toscano On Ago. 6, 2015, 8:37 a.m., Kevin Funk wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124542/ --- (Updated Ago. 6, 2015, 8:37 a.m.) Review request for Documentation, KDE Frameworks and Luigi Toscano. Bugs: 348061 https://bugs.kde.org/show_bug.cgi?id=348061 Repository: kdoctools Description --- BUG: 348061 Diffs - cmake/uriencode.cmake e5f3c3acd93d3871e44b6e6fb29ad7113e18d751 Diff: https://git.reviewboard.kde.org/r/124542/diff/ Testing --- Adding ':' to the list of escaped characters is probably not an ideal solution, but let me hear your ideas. Thanks, Kevin Funk ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 124542: CMake fixes for Windows build
On Ago. 8, 2015, 4:40 p.m., Kevin Funk wrote: @Luigi: Bump? I was waiting for D?vis' answer, but given that 5.13 is out, there is enough time to fix it before 5.14. So let's go, and thanks. - Luigi --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124542/#review83574 --- On Ago. 6, 2015, 8:37 a.m., Kevin Funk wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124542/ --- (Updated Ago. 6, 2015, 8:37 a.m.) Review request for Documentation, KDE Frameworks and Luigi Toscano. Bugs: 348061 https://bugs.kde.org/show_bug.cgi?id=348061 Repository: kdoctools Description --- BUG: 348061 Diffs - cmake/uriencode.cmake e5f3c3acd93d3871e44b6e6fb29ad7113e18d751 Diff: https://git.reviewboard.kde.org/r/124542/diff/ Testing --- Adding ':' to the list of escaped characters is probably not an ideal solution, but let me hear your ideas. Thanks, Kevin Funk ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 124542: CMake fixes for Windows build
On Ago. 8, 2015, 4:40 p.m., Kevin Funk wrote: @Luigi: Bump? Luigi Toscano wrote: I was waiting for D?vis' answer, but given that 5.13 is out, there is enough time to fix it before 5.14. So let's go, and thanks. Just please remember to use a more descriptive commit message than just the bug number :) - Luigi --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124542/#review83574 --- On Ago. 6, 2015, 8:37 a.m., Kevin Funk wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124542/ --- (Updated Ago. 6, 2015, 8:37 a.m.) Review request for Documentation, KDE Frameworks and Luigi Toscano. Bugs: 348061 https://bugs.kde.org/show_bug.cgi?id=348061 Repository: kdoctools Description --- BUG: 348061 Diffs - cmake/uriencode.cmake e5f3c3acd93d3871e44b6e6fb29ad7113e18d751 Diff: https://git.reviewboard.kde.org/r/124542/diff/ Testing --- Adding ':' to the list of escaped characters is probably not an ideal solution, but let me hear your ideas. Thanks, Kevin Funk ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 124542: CMake fixes for Windows build
On Aug. 8, 2015, 5:40 p.m., Kevin Funk wrote: @Luigi: Bump? Luigi Toscano wrote: I was waiting for D?vis' answer, but given that 5.13 is out, there is enough time to fix it before 5.14. So let's go, and thanks. Luigi Toscano wrote: Just please remember to use a more descriptive commit message than just the bug number :) Why would you wait for me..., I was just commenting and I don't have any objections as otherwise I would have said something. Of course proper fix would be preferable to any workarounds (I generally really dislike hacks :P) but workaround today is better than nothing (broken app) for ages. - Dāvis --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124542/#review83574 --- On Aug. 6, 2015, 9:37 a.m., Kevin Funk wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124542/ --- (Updated Aug. 6, 2015, 9:37 a.m.) Review request for Documentation, KDE Frameworks and Luigi Toscano. Bugs: 348061 https://bugs.kde.org/show_bug.cgi?id=348061 Repository: kdoctools Description --- BUG: 348061 Diffs - cmake/uriencode.cmake e5f3c3acd93d3871e44b6e6fb29ad7113e18d751 Diff: https://git.reviewboard.kde.org/r/124542/diff/ Testing --- Adding ':' to the list of escaped characters is probably not an ideal solution, but let me hear your ideas. Thanks, Kevin Funk ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 124542: CMake fixes for Windows build
On Ago. 8, 2015, 4:40 p.m., Kevin Funk wrote: @Luigi: Bump? Luigi Toscano wrote: I was waiting for D?vis' answer, but given that 5.13 is out, there is enough time to fix it before 5.14. So let's go, and thanks. Luigi Toscano wrote: Just please remember to use a more descriptive commit message than just the bug number :) Dāvis Mosāns wrote: Why would you wait for me..., I was just commenting and I don't have any objections as otherwise I would have said something. Of course proper fix would be preferable to any workarounds (I generally really dislike hacks :P) but workaround today is better than nothing (broken app) for ages. The proper solution would be better and I totally agree with that, but I suspect it's in the way libxml2 handles this, so it would take a lot more time and libxml2 versions to have this working. I was waiting for you because your comment was interesting and maybe you had a follow-up after my anser :) - Luigi --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124542/#review83574 --- On Ago. 6, 2015, 8:37 a.m., Kevin Funk wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124542/ --- (Updated Ago. 6, 2015, 8:37 a.m.) Review request for Documentation, KDE Frameworks and Luigi Toscano. Bugs: 348061 https://bugs.kde.org/show_bug.cgi?id=348061 Repository: kdoctools Description --- BUG: 348061 Diffs - cmake/uriencode.cmake e5f3c3acd93d3871e44b6e6fb29ad7113e18d751 Diff: https://git.reviewboard.kde.org/r/124542/diff/ Testing --- Adding ':' to the list of escaped characters is probably not an ideal solution, but let me hear your ideas. Thanks, Kevin Funk ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 124542: CMake fixes for Windows build
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124542/ --- (Updated Aug. 6, 2015, 6:37 a.m.) Review request for Documentation, KDE Frameworks and Luigi Toscano. Changes --- Made requested changes Bugs: 348061 https://bugs.kde.org/show_bug.cgi?id=348061 Repository: kdoctools Description --- BUG: 348061 Diffs (updated) - cmake/uriencode.cmake e5f3c3acd93d3871e44b6e6fb29ad7113e18d751 Diff: https://git.reviewboard.kde.org/r/124542/diff/ Testing --- Adding ':' to the list of escaped characters is probably not an ideal solution, but let me hear your ideas. Thanks, Kevin Funk ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 124542: CMake fixes for Windows build
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124542/#review83349 --- Kevin, when you update the patch, and after the ship it (which will come, I'm sure), please update the description with the real commit message which will end up in the git repository (just BUG: xyzt and REVIEW: tzyz is not enough). - Luigi Toscano On Ago. 1, 2015, 11:40 a.m., Kevin Funk wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124542/ --- (Updated Ago. 1, 2015, 11:40 a.m.) Review request for Documentation, KDE Frameworks and Luigi Toscano. Bugs: 348061 https://bugs.kde.org/show_bug.cgi?id=348061 Repository: kdoctools Description --- BUG: 348061 Diffs - cmake/uriencode.cmake e5f3c3acd93d3871e44b6e6fb29ad7113e18d751 Diff: https://git.reviewboard.kde.org/r/124542/diff/ Testing --- Adding ':' to the list of escaped characters is probably not an ideal solution, but let me hear your ideas. Thanks, Kevin Funk ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 124542: CMake fixes for Windows build
On Ago. 1, 2015, 11:35 p.m., Kevin Funk wrote: Looking back at the review which introduced the escaping, namely https://git.reviewboard.kde.org/r/120648/ , I escaped the comma as well in the first review. I don't remember why I removed them. Apart from that usage, comma are invalid characters in NTFS (according https://kb.acronis.com/content/39790) so we shouldn't hit them; they could work on ext[234]/xfs/whatever but I think that libxml2 could handle them. The only thing I would like to test (and if you volunteer for that I wouldn't complain :) is a test where the path to the DTD does contain a comma, to be sure that not escaping it does work on Linux. Or we could just call it as don't do that and forget about it :) Dāvis Mosāns wrote: Comma is not invalid character for NTFS, maybe you meant colon :? Anyway in either case NTFS have 2 file namespaces, one is Win32 where invalid characters are * NUL * / (slash) * \ (backslash) * : (colon) * * (asterisk) * ? (Question mark) * (quote) * (less than) * (greater than) * | (pipe) and other is POSIX namespace where all Unicode code points are valid except NUL and / (slash). On Linux, NTFS-3G by default will create all files in POSIX namespace thus it will allow creating filename with colon : and other illegal characters in NTFS. And on Windows if you use WinAPI directly you can also create such filenames in POSIX namespace. As for which characters to escape in URI, see https://url.spec.whatwg.org/#url-code-points and also RFCs 1738, 2396, 3986. Both comma and colon is allowed in URI so seems bug is somewhere else and this is just a workaround. Yes, sorry, I meant colon. The entire sentence should be read with colon instead of comma. I guess we mostly care for the Win32 namespace, but in any case, colon are needed there for the drive letter. Anyway, as you mentioned, colon are allowed in URI and the proper format for an URI in windows is file:///C:/whatever/etc/etc1/etc2/myfile.txt, so it looks like a problem in libxml2, but given that the timeframe for fixing that would be too long, I would say to workaround it for now by escaping the colon (in the proper place). - Luigi --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124542/#review83305 --- On Ago. 1, 2015, 11:40 a.m., Kevin Funk wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124542/ --- (Updated Ago. 1, 2015, 11:40 a.m.) Review request for Documentation, KDE Frameworks and Luigi Toscano. Bugs: 348061 https://bugs.kde.org/show_bug.cgi?id=348061 Repository: kdoctools Description --- BUG: 348061 Diffs - cmake/uriencode.cmake e5f3c3acd93d3871e44b6e6fb29ad7113e18d751 Diff: https://git.reviewboard.kde.org/r/124542/diff/ Testing --- Adding ':' to the list of escaped characters is probably not an ideal solution, but let me hear your ideas. Thanks, Kevin Funk ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 124542: CMake fixes for Windows build
On Aug. 2, 2015, 12:35 a.m., Kevin Funk wrote: Looking back at the review which introduced the escaping, namely https://git.reviewboard.kde.org/r/120648/ , I escaped the comma as well in the first review. I don't remember why I removed them. Apart from that usage, comma are invalid characters in NTFS (according https://kb.acronis.com/content/39790) so we shouldn't hit them; they could work on ext[234]/xfs/whatever but I think that libxml2 could handle them. The only thing I would like to test (and if you volunteer for that I wouldn't complain :) is a test where the path to the DTD does contain a comma, to be sure that not escaping it does work on Linux. Or we could just call it as don't do that and forget about it :) Comma is not invalid character for NTFS, maybe you meant colon :? Anyway in either case NTFS have 2 file namespaces, one is Win32 where invalid characters are * NUL * / (slash) * \ (backslash) * : (colon) * * (asterisk) * ? (Question mark) * (quote) * (less than) * (greater than) * | (pipe) and other is POSIX namespace where all Unicode code points are valid except NUL and / (slash). On Linux, NTFS-3G by default will create all files in POSIX namespace thus it will allow creating filename with colon : and other illegal characters in NTFS. And on Windows if you use WinAPI directly you can also create such filenames in POSIX namespace. As for which characters to escape in URI, see https://url.spec.whatwg.org/#url-code-points and also RFCs 1738, 2396, 3986. Both comma and colon is allowed in URI so seems bug is somewhere else and this is just a workaround. - Dāvis --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124542/#review83305 --- On Aug. 1, 2015, 12:40 p.m., Kevin Funk wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124542/ --- (Updated Aug. 1, 2015, 12:40 p.m.) Review request for Documentation, KDE Frameworks and Luigi Toscano. Bugs: 348061 https://bugs.kde.org/show_bug.cgi?id=348061 Repository: kdoctools Description --- BUG: 348061 Diffs - cmake/uriencode.cmake e5f3c3acd93d3871e44b6e6fb29ad7113e18d751 Diff: https://git.reviewboard.kde.org/r/124542/diff/ Testing --- Adding ':' to the list of escaped characters is probably not an ideal solution, but let me hear your ideas. Thanks, Kevin Funk ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 124542: CMake fixes for Windows build
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124542/#review83305 --- cmake/uriencode.cmake (line 37) https://git.reviewboard.kde.org/r/124542/#comment57541 The colon should be before the final \. If you parse the final part of the regexp, namely \\-\\._~\\:/\ , it expands to: \\- \\. _ ~ \\/ \ so it should be \\-\\._~\\/:\ Looking back at the review which introduced the escaping, namely https://git.reviewboard.kde.org/r/120648/ , I escaped the comma as well in the first review. I don't remember why I removed them. Apart from that usage, comma are invalid characters in NTFS (according https://kb.acronis.com/content/39790) so we shouldn't hit them; they could work on ext[234]/xfs/whatever but I think that libxml2 could handle them. The only thing I would like to test (and if you volunteer for that I wouldn't complain :) is a test where the path to the DTD does contain a comma, to be sure that not escaping it does work on Linux. Or we could just call it as don't do that and forget about it :) - Luigi Toscano On Ago. 1, 2015, 11:40 a.m., Kevin Funk wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124542/ --- (Updated Ago. 1, 2015, 11:40 a.m.) Review request for Documentation, KDE Frameworks and Luigi Toscano. Bugs: 348061 https://bugs.kde.org/show_bug.cgi?id=348061 Repository: kdoctools Description --- BUG: 348061 Diffs - cmake/uriencode.cmake e5f3c3acd93d3871e44b6e6fb29ad7113e18d751 Diff: https://git.reviewboard.kde.org/r/124542/diff/ Testing --- Adding ':' to the list of escaped characters is probably not an ideal solution, but let me hear your ideas. Thanks, Kevin Funk ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 124542: CMake fixes for Windows build
On July 31, 2015, 10:18 a.m., David Faure wrote: src/CMakeLists.txt, line 20 https://git.reviewboard.kde.org/r/124542/diff/1/?file=388805#file388805line20 no-op? Kevin Funk wrote: Not a no-op, this code is written into cmake_install which is used for the 'install' target. If I don't add this line I get this during 'make install': ``` (...) -- Installing: Z:/kderoot/build/frameworks/kdoctools/image-msvc2015-RelWithDebInfo-master/kderoot/share/man/man7/qt5options.7 -- Found Perl: Z:/kderoot/dev-utils/bin/perl.exe (found version 5.20.2) CMake Error at Z:/kderoot/download/git/kdoctools/cmake/uriencode.cmake:34 (find_package): By not providing FindPerlModules.cmake in CMAKE_MODULE_PATH this project has asked CMake to find a package configuration file provided by PerlModules, but CMake did not find one. ``` (I can also push this separately) This looks like a bug in cmake... maybe you can report it and see what's the outcome. - Aleix --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124542/#review83214 --- On July 31, 2015, 9:34 a.m., Kevin Funk wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124542/ --- (Updated July 31, 2015, 9:34 a.m.) Review request for Documentation, KDE Frameworks and Luigi Toscano. Repository: kdoctools Description --- BUG: 348061 Diffs - cmake/uriencode.cmake e5f3c3acd93d3871e44b6e6fb29ad7113e18d751 src/CMakeLists.txt e3868fc4f22324d25f680c13609bfb92b8b7c41d Diff: https://git.reviewboard.kde.org/r/124542/diff/ Testing --- Adding ':' to the list of escaped characters is probably not an ideal solution, but let me hear your ideas. Thanks, Kevin Funk ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 124542: CMake fixes for Windows build
On July 31, 2015, 8:18 a.m., David Faure wrote: src/CMakeLists.txt, line 20 https://git.reviewboard.kde.org/r/124542/diff/1/?file=388805#file388805line20 no-op? Not a no-op, this code is written into cmake_install which is used for the 'install' target. If I don't add this line I get this during 'make install': ``` (...) -- Installing: Z:/kderoot/build/frameworks/kdoctools/image-msvc2015-RelWithDebInfo-master/kderoot/share/man/man7/qt5options.7 -- Found Perl: Z:/kderoot/dev-utils/bin/perl.exe (found version 5.20.2) CMake Error at Z:/kderoot/download/git/kdoctools/cmake/uriencode.cmake:34 (find_package): By not providing FindPerlModules.cmake in CMAKE_MODULE_PATH this project has asked CMake to find a package configuration file provided by PerlModules, but CMake did not find one. ``` (I can also push this separately) - Kevin --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124542/#review83214 --- On July 31, 2015, 7:34 a.m., Kevin Funk wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124542/ --- (Updated July 31, 2015, 7:34 a.m.) Review request for Documentation, KDE Frameworks and Luigi Toscano. Repository: kdoctools Description --- BUG: 348061 Diffs - cmake/uriencode.cmake e5f3c3acd93d3871e44b6e6fb29ad7113e18d751 src/CMakeLists.txt e3868fc4f22324d25f680c13609bfb92b8b7c41d Diff: https://git.reviewboard.kde.org/r/124542/diff/ Testing --- Adding ':' to the list of escaped characters is probably not an ideal solution, but let me hear your ideas. Thanks, Kevin Funk ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 124542: CMake fixes for Windows build
On July 31, 2015, 5:19 p.m., David Faure wrote:
src/CMakeLists.txt, line 20
https://git.reviewboard.kde.org/r/124542/diff/1/?file=388805#file388805line20
I can't see how setting a variable to itself can possibly change
anything, or make any sense (other than an extremely weird bug in cmake).
Actually it could be that this interprets the string as a list or vice
versa in which case the problem could be before this, at the time where
this module sets CMAKE_MODULE_PATH (toplevel file?)
Alex Merry wrote:
See my comment above. This change is correct.
Alex Merry wrote:
Ah, and you should note the context the variable substitution takes place
in. It is being substituted in the CMakeLists.txt file, but the `set` call is
being evaluated in the install script.
That being said, there should probably be some quotes in there, like
set(CMAKE_MODULE_PATH \${CMAKE_MODULE_PATH}\)
Kevin Funk wrote:
Can do. I'll push this patch within the next hour if noone objects.
I'm certainly willing to give a shipit to this part of the patch - feel free
to put that it was reviewed by me in the commit. I'm not so sure about the
:-escaping for URLs - see what David said originally.
- Alex
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124542/#review83249
---
On July 31, 2015, 5:33 p.m., Kevin Funk wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124542/
---
(Updated July 31, 2015, 5:33 p.m.)
Review request for Documentation, KDE Frameworks and Luigi Toscano.
Bugs: 348061
https://bugs.kde.org/show_bug.cgi?id=348061
Repository: kdoctools
Description
---
BUG: 348061
Diffs
-
cmake/uriencode.cmake e5f3c3acd93d3871e44b6e6fb29ad7113e18d751
src/CMakeLists.txt e3868fc4f22324d25f680c13609bfb92b8b7c41d
Diff: https://git.reviewboard.kde.org/r/124542/diff/
Testing
---
Adding ':' to the list of escaped characters is probably not an ideal
solution, but let me hear your ideas.
Thanks,
Kevin Funk
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 124542: CMake fixes for Windows build
On Lug. 31, 2015, 7:19 p.m., David Faure wrote:
src/CMakeLists.txt, line 20
https://git.reviewboard.kde.org/r/124542/diff/1/?file=388805#file388805line20
I can't see how setting a variable to itself can possibly change
anything, or make any sense (other than an extremely weird bug in cmake).
Actually it could be that this interprets the string as a list or vice
versa in which case the problem could be before this, at the time where
this module sets CMAKE_MODULE_PATH (toplevel file?)
Alex Merry wrote:
See my comment above. This change is correct.
Alex Merry wrote:
Ah, and you should note the context the variable substitution takes place
in. It is being substituted in the CMakeLists.txt file, but the `set` call is
being evaluated in the install script.
That being said, there should probably be some quotes in there, like
set(CMAKE_MODULE_PATH \${CMAKE_MODULE_PATH}\)
Kevin Funk wrote:
Can do. I'll push this patch within the next hour if noone objects.
Alex Merry wrote:
I'm certainly willing to give a shipit to this part of the patch - feel
free to put that it was reviewed by me in the commit. I'm not so sure about
the :-escaping for URLs - see what David said originally.
I agree with the escaping (if you can test on linux and win).
I can't test much right now; for the escaping, if I understand it correctly,
the : should be kept (only) for the drive.
- Luigi
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124542/#review83249
---
On Lug. 31, 2015, 7:33 p.m., Kevin Funk wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124542/
---
(Updated Lug. 31, 2015, 7:33 p.m.)
Review request for Documentation, KDE Frameworks and Luigi Toscano.
Bugs: 348061
https://bugs.kde.org/show_bug.cgi?id=348061
Repository: kdoctools
Description
---
BUG: 348061
Diffs
-
cmake/uriencode.cmake e5f3c3acd93d3871e44b6e6fb29ad7113e18d751
src/CMakeLists.txt e3868fc4f22324d25f680c13609bfb92b8b7c41d
Diff: https://git.reviewboard.kde.org/r/124542/diff/
Testing
---
Adding ':' to the list of escaped characters is probably not an ideal
solution, but let me hear your ideas.
Thanks,
Kevin Funk
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 124542: CMake fixes for Windows build
On July 31, 2015, 5:19 p.m., David Faure wrote: src/CMakeLists.txt, line 20 https://git.reviewboard.kde.org/r/124542/diff/1/?file=388805#file388805line20 I can't see how setting a variable to itself can possibly change anything, or make any sense (other than an extremely weird bug in cmake). Actually it could be that this interprets the string as a list or vice versa in which case the problem could be before this, at the time where this module sets CMAKE_MODULE_PATH (toplevel file?) See my comment above. This change is correct. - Alex --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124542/#review83249 --- On July 31, 2015, 7:34 a.m., Kevin Funk wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124542/ --- (Updated July 31, 2015, 7:34 a.m.) Review request for Documentation, KDE Frameworks and Luigi Toscano. Repository: kdoctools Description --- BUG: 348061 Diffs - cmake/uriencode.cmake e5f3c3acd93d3871e44b6e6fb29ad7113e18d751 src/CMakeLists.txt e3868fc4f22324d25f680c13609bfb92b8b7c41d Diff: https://git.reviewboard.kde.org/r/124542/diff/ Testing --- Adding ':' to the list of escaped characters is probably not an ideal solution, but let me hear your ideas. Thanks, Kevin Funk ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 124542: CMake fixes for Windows build
On July 31, 2015, 8:18 a.m., David Faure wrote: src/CMakeLists.txt, line 20 https://git.reviewboard.kde.org/r/124542/diff/1/?file=388805#file388805line20 no-op? Kevin Funk wrote: Not a no-op, this code is written into cmake_install which is used for the 'install' target. If I don't add this line I get this during 'make install': ``` (...) -- Installing: Z:/kderoot/build/frameworks/kdoctools/image-msvc2015-RelWithDebInfo-master/kderoot/share/man/man7/qt5options.7 -- Found Perl: Z:/kderoot/dev-utils/bin/perl.exe (found version 5.20.2) CMake Error at Z:/kderoot/download/git/kdoctools/cmake/uriencode.cmake:34 (find_package): By not providing FindPerlModules.cmake in CMAKE_MODULE_PATH this project has asked CMake to find a package configuration file provided by PerlModules, but CMake did not find one. ``` (I can also push this separately) Aleix Pol Gonzalez wrote: This looks like a bug in cmake... maybe you can report it and see what's the outcome. Not a bug in CMake, no. As of earlier this week, `kdoctools_encode_uri` uses a find-module that's only provided locally. I hadn't realised it was used from an install script on WIN32. That part of this patch is correct. - Alex --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124542/#review83214 --- On July 31, 2015, 7:34 a.m., Kevin Funk wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124542/ --- (Updated July 31, 2015, 7:34 a.m.) Review request for Documentation, KDE Frameworks and Luigi Toscano. Repository: kdoctools Description --- BUG: 348061 Diffs - cmake/uriencode.cmake e5f3c3acd93d3871e44b6e6fb29ad7113e18d751 src/CMakeLists.txt e3868fc4f22324d25f680c13609bfb92b8b7c41d Diff: https://git.reviewboard.kde.org/r/124542/diff/ Testing --- Adding ':' to the list of escaped characters is probably not an ideal solution, but let me hear your ideas. Thanks, Kevin Funk ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 124542: CMake fixes for Windows build
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124542/ --- (Updated July 31, 2015, 5:33 p.m.) Review request for Documentation, KDE Frameworks and Luigi Toscano. Changes --- Add bug link Bugs: 348061 https://bugs.kde.org/show_bug.cgi?id=348061 Repository: kdoctools Description --- BUG: 348061 Diffs - cmake/uriencode.cmake e5f3c3acd93d3871e44b6e6fb29ad7113e18d751 src/CMakeLists.txt e3868fc4f22324d25f680c13609bfb92b8b7c41d Diff: https://git.reviewboard.kde.org/r/124542/diff/ Testing --- Adding ':' to the list of escaped characters is probably not an ideal solution, but let me hear your ideas. Thanks, Kevin Funk ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 124542: CMake fixes for Windows build
On July 31, 2015, 5:19 p.m., David Faure wrote:
src/CMakeLists.txt, line 20
https://git.reviewboard.kde.org/r/124542/diff/1/?file=388805#file388805line20
I can't see how setting a variable to itself can possibly change
anything, or make any sense (other than an extremely weird bug in cmake).
Actually it could be that this interprets the string as a list or vice
versa in which case the problem could be before this, at the time where
this module sets CMAKE_MODULE_PATH (toplevel file?)
Alex Merry wrote:
See my comment above. This change is correct.
Alex Merry wrote:
Ah, and you should note the context the variable substitution takes place
in. It is being substituted in the CMakeLists.txt file, but the `set` call is
being evaluated in the install script.
That being said, there should probably be some quotes in there, like
set(CMAKE_MODULE_PATH \${CMAKE_MODULE_PATH}\)
Can do. I'll push this patch within the next hour if noone objects.
- Kevin
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124542/#review83249
---
On July 31, 2015, 7:34 a.m., Kevin Funk wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124542/
---
(Updated July 31, 2015, 7:34 a.m.)
Review request for Documentation, KDE Frameworks and Luigi Toscano.
Repository: kdoctools
Description
---
BUG: 348061
Diffs
-
cmake/uriencode.cmake e5f3c3acd93d3871e44b6e6fb29ad7113e18d751
src/CMakeLists.txt e3868fc4f22324d25f680c13609bfb92b8b7c41d
Diff: https://git.reviewboard.kde.org/r/124542/diff/
Testing
---
Adding ':' to the list of escaped characters is probably not an ideal
solution, but let me hear your ideas.
Thanks,
Kevin Funk
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 124542: CMake fixes for Windows build
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124542/#review83249 --- src/CMakeLists.txt (line 20) https://git.reviewboard.kde.org/r/124542/#comment57484 I can't see how setting a variable to itself can possibly change anything, or make any sense (other than an extremely weird bug in cmake). Actually it could be that this interprets the string as a list or vice versa in which case the problem could be before this, at the time where this module sets CMAKE_MODULE_PATH (toplevel file?) - David Faure On July 31, 2015, 7:34 a.m., Kevin Funk wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124542/ --- (Updated July 31, 2015, 7:34 a.m.) Review request for Documentation, KDE Frameworks and Luigi Toscano. Repository: kdoctools Description --- BUG: 348061 Diffs - cmake/uriencode.cmake e5f3c3acd93d3871e44b6e6fb29ad7113e18d751 src/CMakeLists.txt e3868fc4f22324d25f680c13609bfb92b8b7c41d Diff: https://git.reviewboard.kde.org/r/124542/diff/ Testing --- Adding ':' to the list of escaped characters is probably not an ideal solution, but let me hear your ideas. Thanks, Kevin Funk ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 124542: CMake fixes for Windows build
On July 31, 2015, 5:19 p.m., David Faure wrote:
src/CMakeLists.txt, line 20
https://git.reviewboard.kde.org/r/124542/diff/1/?file=388805#file388805line20
I can't see how setting a variable to itself can possibly change
anything, or make any sense (other than an extremely weird bug in cmake).
Actually it could be that this interprets the string as a list or vice
versa in which case the problem could be before this, at the time where
this module sets CMAKE_MODULE_PATH (toplevel file?)
Alex Merry wrote:
See my comment above. This change is correct.
Ah, and you should note the context the variable substitution takes place in.
It is being substituted in the CMakeLists.txt file, but the `set` call is being
evaluated in the install script.
That being said, there should probably be some quotes in there, like
set(CMAKE_MODULE_PATH \${CMAKE_MODULE_PATH}\)
- Alex
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124542/#review83249
---
On July 31, 2015, 7:34 a.m., Kevin Funk wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124542/
---
(Updated July 31, 2015, 7:34 a.m.)
Review request for Documentation, KDE Frameworks and Luigi Toscano.
Repository: kdoctools
Description
---
BUG: 348061
Diffs
-
cmake/uriencode.cmake e5f3c3acd93d3871e44b6e6fb29ad7113e18d751
src/CMakeLists.txt e3868fc4f22324d25f680c13609bfb92b8b7c41d
Diff: https://git.reviewboard.kde.org/r/124542/diff/
Testing
---
Adding ':' to the list of escaped characters is probably not an ideal
solution, but let me hear your ideas.
Thanks,
Kevin Funk
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Review Request 124542: CMake fixes for Windows build
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124542/ --- Review request for Documentation, KDE Frameworks and Luigi Toscano. Repository: kdoctools Description --- BUG: 348061 Diffs - cmake/uriencode.cmake e5f3c3acd93d3871e44b6e6fb29ad7113e18d751 src/CMakeLists.txt e3868fc4f22324d25f680c13609bfb92b8b7c41d Diff: https://git.reviewboard.kde.org/r/124542/diff/ Testing --- Adding ':' to the list of escaped characters is probably not an ideal solution, but let me hear your ideas. Thanks, Kevin Funk ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 124542: CMake fixes for Windows build
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124542/#review83214 --- This is not ideal indeed, because URIs usually contain a real ':' : like the one after the scheme, which I suppose shouldn't be escaped. I think the bug is before this code gets called, because a URL like file:///R:/path is broken in the first place. (do I understand it right that this is what this function is getting as input?) src/CMakeLists.txt (line 20) https://git.reviewboard.kde.org/r/124542/#comment57460 no-op? - David Faure On July 31, 2015, 7:34 a.m., Kevin Funk wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124542/ --- (Updated July 31, 2015, 7:34 a.m.) Review request for Documentation, KDE Frameworks and Luigi Toscano. Repository: kdoctools Description --- BUG: 348061 Diffs - cmake/uriencode.cmake e5f3c3acd93d3871e44b6e6fb29ad7113e18d751 src/CMakeLists.txt e3868fc4f22324d25f680c13609bfb92b8b7c41d Diff: https://git.reviewboard.kde.org/r/124542/diff/ Testing --- Adding ':' to the list of escaped characters is probably not an ideal solution, but let me hear your ideas. Thanks, Kevin Funk ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
