Re: version numbers towards kf6

[Jos van den Oever]

On 26/06/2023 17.27, Heiko Becker wrote:

On Monday, 26 June 2023 14:06:04 CEST, Jos van den Oever wrote:

On 26/06/2023 13.15, Heiko Becker wrote:

On Monday, 26 June 2023 11:13:56 CEST, Jos van den Oever wrote:
The new versions of frameworks, plasma and gear presumably all start 
with '6'. Following Fedora versioning for snapshots [0] gives this:


 6^20230627git5328c27e3


Like Jonathan said, versioning of snapshots is a downstream thing. 
But if I understand the ^ operator correctly, doesn't sort 6^20230.. 
*after* 6.0?


Indeed, it should be 6~20230627git5328c27e3 or 
6.0.0~20230627git5328c27e3.


And no, new versions of Gear won't start with '6'. They follow a 
different versioning scheme and some projects will probably switch to 
Qt6/KF6 with 22.12, some with 23.04 or later.


I guess you mean 23.12 and 24.04. Time flies.


Yeah, indeed.


What is the best way to override the version numbers of KF6?
  cmake -DPROJECT_VERSION_MAJOR=6 ?
Since there is no suffix support this would falsely indicate that it's 
already version 6 instead of a snapshot working towards it.


Why do you want to override it it?


So that the user can see that they are running a snapshot and not an 
official release and to make it easy to report the correct 
version/snapshot in bug reports. Getting users to test and submit useful 
bug reports is the goal of these snapshot builds.


Best regards,
Jos


OpenPGP_signature
Description: OpenPGP digital signature

Re: version numbers towards kf6

[Jos van den Oever]

On 26/06/2023 13.15, Heiko Becker wrote:

On Monday, 26 June 2023 11:13:56 CEST, Jos van den Oever wrote:
The new versions of frameworks, plasma and gear presumably all start 
with '6'. Following Fedora versioning for snapshots [0] gives this:


 6^20230627git5328c27e3


Like Jonathan said, versioning of snapshots is a downstream thing. But 
if I understand the ^ operator correctly, doesn't sort 6^20230.. *after* 
6.0?


Indeed, it should be 6~20230627git5328c27e3 or 6.0.0~20230627git5328c27e3.

And no, new versions of Gear won't start with '6'. They follow a 
different versioning scheme and some projects will probably switch to 
Qt6/KF6 with 22.12, some with 23.04 or later.


I guess you mean 23.12 and 24.04. Time flies.

Then for gear it makes sense to use $current^20230627git5328c27e3 e.g. 
23.04^20230627git5328c27e3.


What is the best way to override the version numbers of KF6?
  cmake -DPROJECT_VERSION_MAJOR=6 ?
Since there is no suffix support this would falsely indicate that it's 
already version 6 instead of a snapshot working towards it.


Best regards,
Jos




OpenPGP_signature
Description: OpenPGP digital signature

Re: version numbers towards kf6

[Jos van den Oever]

The new versions of frameworks, plasma and gear presumably all start 
with '6'. Following Fedora versioning for snapshots [0] gives this:


 6^20230627git5328c27e3

Having the commit date and the git revision in there helps with bug reports.

But this disregards the context (build options, versions of other packages).

The version number in CMakeLists.txt determines what users see in the 
about box and that is still e.g.

  set(PROJECT_VERSION "5.27.80")
  set(PROJECT_VERSION_MAJOR 5)

CMake has no support for ^ or ~ with version suffixes [1].

Best regards,
Jos

[0] https://docs.fedoraproject.org/en-US/packaging-guidelines/Versioning/
[1] https://gitlab.kitware.com/cmake/cmake/-/issues/16716

On 26/06/2023 10.36, Jonathan Riddell wrote:
Use whatever you like for your distro but for neon we have the not very 
elegant


5.92.0+p22.04+tunstable+git20230613.2104-0 for git master frameworks build

frameworks version, ubuntu version, neon edition, date.time, buildnumber.

Of course the frameworks version is meaningless there, it's just where I 
took the first packaging from, I'll update that once the frameworks get 
releases.


Jonathan


OpenPGP_signature
Description: OpenPGP digital signature

version numbers towards kf6

[Jos van den Oever]

Hello all,

I'm packaging KF6 software in a nix flake [0]. I'm wondering what 
version numbers to use for packages built from the latest git commit 
from master.


Best regards,
Jos

[0] https://invent.kde.org/vandenoever/kde-nix-flake/-/tree/kf6-qt6


OpenPGP_signature
Description: OpenPGP digital signature

D19263: Add more highlighting for nested languages in markdown.

[Jos van den Oever]

vandenoever added a comment.


  done

REPOSITORY
  R216 Syntax Highlighting

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

To: vandenoever, #framework_syntax_highlighting, dhaumann
Cc: dhaumann, kwrite-devel, kde-frameworks-devel, domson, michaelh, ngraham, 
bruns, demsking, cullmann, sars

D19263: Add more highlighting for nested languages in markdown.

[Jos van den Oever]

vandenoever added a comment.


  I see that one test fails after committing this patch. The example output was 
modified by `/autotests/update-reference-data.sh` but I did not submit it. I 
thought it was not related to the markdown change.

REPOSITORY
  R216 Syntax Highlighting

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

To: vandenoever, #framework_syntax_highlighting, dhaumann
Cc: dhaumann, kwrite-devel, kde-frameworks-devel, domson, michaelh, ngraham, 
bruns, demsking, cullmann, sars

D19263: Add more highlighting for nested languages in markdown.

[Jos van den Oever]

This revision was automatically updated to reflect the committed changes.
Closed by commit R216:bbb5188d9e12: Add more highlighting for nested languages 
in markdown. (authored by vandenoever).

REPOSITORY
  R216 Syntax Highlighting

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19263?vs=52430=52431

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

AFFECTED FILES
  autotests/folding/test.markdown.fold
  autotests/html/test.markdown.html
  autotests/input/test.markdown
  autotests/reference/test.markdown.ref
  data/syntax/markdown.xml

To: vandenoever, #framework_syntax_highlighting, dhaumann
Cc: dhaumann, kwrite-devel, kde-frameworks-devel, domson, michaelh, ngraham, 
bruns, demsking, cullmann, sars

D19263: Add more highlighting for nested languages in markdown.

[Jos van den Oever]

vandenoever updated this revision to Diff 52430.
vandenoever added a comment.


  Increased kateversion to 5.0.

REPOSITORY
  R216 Syntax Highlighting

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19263?vs=52410=52430

BRANCH
  nested (branched from master)

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

AFFECTED FILES
  autotests/folding/test.markdown.fold
  autotests/html/test.markdown.html
  autotests/input/test.markdown
  autotests/reference/test.markdown.ref
  data/syntax/markdown.xml

To: vandenoever, #framework_syntax_highlighting, dhaumann
Cc: dhaumann, kwrite-devel, kde-frameworks-devel, domson, michaelh, ngraham, 
bruns, demsking, cullmann, sars

D19263: Add more highlighting for nested languages in markdown.

[Jos van den Oever]

vandenoever added a comment.


  Even folding works inside fenced blocks.
  
  After running `./autotests/update-reference-data.sh` 
`autotests/folding/example.rmd.fold` changed. I did not commit that change.
  
  The language version is now updated and I added a few more languages.

REPOSITORY
  R216 Syntax Highlighting

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

To: vandenoever, #framework_syntax_highlighting
Cc: dhaumann, kwrite-devel, kde-frameworks-devel, domson, michaelh, ngraham, 
bruns, demsking, cullmann, sars

D19263: Add more highlighting for nested languages in markdown.

[Jos van den Oever]

vandenoever updated this revision to Diff 52410.
vandenoever added a comment.


  - added more languages,
  - updated language version
  - updated test files

REPOSITORY
  R216 Syntax Highlighting

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19263?vs=52395=52410

BRANCH
  nested (branched from master)

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

AFFECTED FILES
  autotests/folding/test.markdown.fold
  autotests/html/test.markdown.html
  autotests/input/test.markdown
  autotests/reference/test.markdown.ref
  data/syntax/markdown.xml

To: vandenoever, #framework_syntax_highlighting
Cc: dhaumann, kwrite-devel, kde-frameworks-devel, domson, michaelh, ngraham, 
bruns, demsking, cullmann, sars

D19263: Add more highlighting for nested languages in markdown.

[Jos van den Oever]

vandenoever retitled this revision from "yAdd more highlighting for nested 
languages in markdown." to "Add more highlighting for nested languages in 
markdown.".

REPOSITORY
  R216 Syntax Highlighting

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

To: vandenoever, #framework_syntax_highlighting
Cc: kwrite-devel, kde-frameworks-devel, domson, michaelh, ngraham, bruns, 
demsking, cullmann, sars, dhaumann

D19263: yAdd more highlighting for nested languages in markdown.

[Jos van den Oever]

vandenoever added a reviewer: Framework: Syntax Highlighting.

REPOSITORY
  R216 Syntax Highlighting

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

To: vandenoever, #framework_syntax_highlighting
Cc: kwrite-devel, kde-frameworks-devel, domson, michaelh, ngraham, bruns, 
demsking, cullmann, sars, dhaumann

D19263: yAdd more highlighting for nested languages in markdown.

[Jos van den Oever]

vandenoever created this revision.
Herald added projects: Kate, Frameworks.
Herald added subscribers: kde-frameworks-devel, kwrite-devel.
vandenoever requested review of this revision.

REPOSITORY
  R216 Syntax Highlighting

BRANCH
  nested (branched from master)

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

AFFECTED FILES
  data/syntax/markdown.xml

To: vandenoever
Cc: kwrite-devel, kde-frameworks-devel, domson, michaelh, ngraham, bruns, 
demsking, cullmann, sars, dhaumann

D19107: Write valid UTF8 characters without escaping.

[Jos van den Oever]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit R237:2cdcd4f30666: Write valid UTF8 characters without 
escaping. (authored by vandenoever).

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19107?vs=52096=52159

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

AFFECTED FILES
  autotests/kconfigtest.cpp
  autotests/kconfigtest.h
  src/core/kconfigini.cpp

To: vandenoever, dfaure, arichardson, apol, #frameworks, thiago
Cc: rapiteanu, kde-frameworks-devel, michaelh, ngraham, bruns

D19107: Write valid UTF8 characters without escaping.

[Jos van den Oever]

vandenoever added a comment.


  So it's accepted?

REPOSITORY
  R237 KConfig

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

To: vandenoever, dfaure, arichardson, apol, #frameworks, thiago
Cc: rapiteanu, kde-frameworks-devel, michaelh, ngraham, bruns

D19107: Write valid UTF8 characters without escaping.

[Jos van den Oever]

vandenoever added inline comments.

INLINE COMMENTS

> thiago wrote in kconfigini.cpp:683
> This function does operate properly to find valid syntax UTF-8 sequences, but 
> it is neither catching overlong sequences nor UTF-8 content above U+10 
> (UTF-8 can encode 0x11000 in 4 bytes).
> 
> See 
> https://code.woboq.org/qt5/qtbase/tests/auto/corelib/codecs/utf8/utf8data.cpp.html#_Z19loadInvalidUtf8Rowsv
>  for potential UTF-8 pitfalls.

Thanks for the unicode explanation. I've added the checks for out of range and 
overlong now.

REPOSITORY
  R237 KConfig

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

To: vandenoever, dfaure, arichardson, apol, #frameworks, thiago
Cc: rapiteanu, kde-frameworks-devel, michaelh, ngraham, bruns

D19107: Write valid UTF8 characters without escaping.

[Jos van den Oever]

vandenoever updated this revision to Diff 52096.
vandenoever added a comment.


  Clean up test by using QTest data.

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19107?vs=52091=52096

BRANCH
  utf8

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

AFFECTED FILES
  autotests/kconfigtest.cpp
  autotests/kconfigtest.h
  src/core/kconfigini.cpp

To: vandenoever, dfaure, arichardson, apol, #frameworks, thiago
Cc: rapiteanu, kde-frameworks-devel, michaelh, ngraham, bruns

D19107: Write valid UTF8 characters without escaping.

[Jos van den Oever]

vandenoever updated this revision to Diff 52091.
vandenoever added a comment.


  Remove duplicate if statement.

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19107?vs=52090=52091

BRANCH
  utf8

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

AFFECTED FILES
  autotests/kconfigtest.cpp
  autotests/kconfigtest.h
  src/core/kconfigini.cpp

To: vandenoever, dfaure, arichardson, apol, #frameworks, thiago
Cc: rapiteanu, kde-frameworks-devel, michaelh, ngraham, bruns

D19107: Write valid UTF8 characters without escaping.

[Jos van den Oever]

vandenoever updated this revision to Diff 52090.
vandenoever added a comment.


  Added tests and code fixes to deal with overlong sequences and content 
  above U+10.

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19107?vs=52015=52090

BRANCH
  utf8

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

AFFECTED FILES
  autotests/kconfigtest.cpp
  autotests/kconfigtest.h
  src/core/kconfigini.cpp

To: vandenoever, dfaure, arichardson, apol, #frameworks, thiago
Cc: rapiteanu, kde-frameworks-devel, michaelh, ngraham, bruns

D19107: Write valid UTF8 characters without escaping.

[Jos van den Oever]

vandenoever added inline comments.

INLINE COMMENTS

> thiago wrote in kconfigini.cpp:683
> This function does operate properly to find valid syntax UTF-8 sequences, but 
> it is neither catching overlong sequences nor UTF-8 content above U+10 
> (UTF-8 can encode 0x11000 in 4 bytes).
> 
> See 
> https://code.woboq.org/qt5/qtbase/tests/auto/corelib/codecs/utf8/utf8data.cpp.html#_Z19loadInvalidUtf8Rowsv
>  for potential UTF-8 pitfalls.

My understanding of overlong sequences was wrong. I understand now what is 
meant from your link.

REPOSITORY
  R237 KConfig

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

To: vandenoever, dfaure, arichardson, apol, #frameworks, thiago
Cc: rapiteanu, kde-frameworks-devel, michaelh, ngraham, bruns

D19107: Write valid UTF8 characters without escaping.

[Jos van den Oever]

vandenoever added inline comments.

INLINE COMMENTS

> thiago wrote in kconfigini.cpp:683
> This function does operate properly to find valid syntax UTF-8 sequences, but 
> it is neither catching overlong sequences nor UTF-8 content above U+10 
> (UTF-8 can encode 0x11000 in 4 bytes).
> 
> See 
> https://code.woboq.org/qt5/qtbase/tests/auto/corelib/codecs/utf8/utf8data.cpp.html#_Z19loadInvalidUtf8Rowsv
>  for potential UTF-8 pitfalls.

A check for U+10 > value is needed.
Overlong sequences are caught on line 696 (count < 4).

REPOSITORY
  R237 KConfig

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

To: vandenoever, dfaure, arichardson, apol, #frameworks, thiago
Cc: rapiteanu, kde-frameworks-devel, michaelh, ngraham, bruns

D19107: Write valid UTF8 characters without escaping.

[Jos van den Oever]

vandenoever updated this revision to Diff 52015.
vandenoever added a comment.


  - Remove VALUE define.
  - Spelling fix.

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19107?vs=51933=52015

BRANCH
  utf8

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

AFFECTED FILES
  autotests/kconfigtest.cpp
  autotests/kconfigtest.h
  src/core/kconfigini.cpp

To: vandenoever, dfaure, arichardson, apol, #frameworks, thiago
Cc: rapiteanu, kde-frameworks-devel, michaelh, ngraham, bruns

D19107: Write valid UTF8 characters without escaping.

[Jos van den Oever]

vandenoever added inline comments.

INLINE COMMENTS

> dfaure wrote in kconfigtest.cpp:1774
> What's the purpose of this very short-lived define, compared to just inlining 
> this into the next line?

Right, copy-paste leftover. I'll fix it.

REPOSITORY
  R237 KConfig

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

To: vandenoever, dfaure, arichardson, apol, #frameworks, thiago
Cc: rapiteanu, kde-frameworks-devel, michaelh, ngraham, bruns

D19107: Write valid UTF8 characters without escaping.

[Jos van den Oever]

vandenoever added a reviewer: thiago.

REPOSITORY
  R237 KConfig

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

To: vandenoever, dfaure, arichardson, apol, #frameworks, thiago
Cc: rapiteanu, kde-frameworks-devel, michaelh, ngraham, bruns

D19107: Write valid UTF8 characters without escaping.

[Jos van den Oever]

vandenoever created this revision.
vandenoever added reviewers: dfaure, arichardson, apol.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
vandenoever requested review of this revision.

REVISION SUMMARY
  commit 6a18528 
 
introduced escaping of bytes >= 127 to ensure that
  KConfig files are valid UTF8.
  The simplistic approach with a cutoff results in many escaped bytes
  where it is not required. Especially non-western configuration files
  would have many escapes.
  
  This commit fixes that by only escaping bytes that are not valid UTF8.

TEST PLAN
  ninja && ninja test

REPOSITORY
  R237 KConfig

BRANCH
  utf8

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

AFFECTED FILES
  autotests/kconfigtest.cpp
  autotests/kconfigtest.h
  src/core/kconfigini.cpp

To: vandenoever, dfaure, arichardson, apol
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17856: Fix a regression introduced in 6a1852

[Jos van den Oever]

This revision was automatically updated to reflect the committed changes.
Closed by commit R237:f403d09d0b49: Fix a regression introduced in 6a1852 
(authored by vandenoever).

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17856?vs=48363=48393

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

AFFECTED FILES
  src/core/kconfigini.cpp

To: vandenoever, dfaure, arichardson, apol, aacid, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17856: Fix a regression introduced in 6a1852

[Jos van den Oever]

vandenoever added reviewers: dfaure, arichardson, apol, aacid, ngraham.

REPOSITORY
  R237 KConfig

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

To: vandenoever, dfaure, arichardson, apol, aacid, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17651: Escape bytes that are larger than or equal to 127 in config files

[Jos van den Oever]

vandenoever added a comment.


  Sure. It's here https://phabricator.kde.org/D17856

REPOSITORY
  R237 KConfig

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

To: vandenoever, dfaure, arichardson, apol
Cc: ngraham, aacid, apol, kde-frameworks-devel, michaelh, bruns

D17856: Fix a regression introduced in 6a1852

[Jos van den Oever]

vandenoever created this revision.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
vandenoever requested review of this revision.

REVISION SUMMARY
  Bytes from 'Strings' of type GroupString and KeyString should not be
  escaped because they are valid UTF-8. Only instances of ValueString
  should be escaped.
  
  This fixes the failing test KConfigTest::testEncoding

TEST PLAN
  Ran `ninja test` and found no errors.

REPOSITORY
  R237 KConfig

BRANCH
  fixutf8 (branched from master)

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

AFFECTED FILES
  src/core/kconfigini.cpp

To: vandenoever
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17651: Escape bytes that are larger than or equal to 127 in config files

[Jos van den Oever]

vandenoever added a comment.


  Here is a patch that solves the problem:
  
diff --git a/src/core/kconfigini.cpp b/src/core/kconfigini.cpp
index 39e5936..b674973 100644
--- a/src/core/kconfigini.cpp
+++ b/src/core/kconfigini.cpp
@@ -673,7 +673,12 @@ QByteArray KConfigIniBackend::stringToPrintable(const 
QByteArray , Strin
 switch (s[i]) {
 default:
 // The \n, \t, \r cases (all < 32) are handled below; we can 
ignore them here
-if (((unsigned char)s[i]) < 32 || ((unsigned char)s[i]) >= 
127) {
+if (((unsigned char)s[i]) < 32) {
+goto doEscape;
+}
+// GroupString and KeyString should be valid UTF-8, but 
ValueString
+// can be a bytearray with non-UTF-8 bytes that should be 
escaped.
+if (type == ValueString && ((unsigned char)s[i]) >= 127) {
 goto doEscape;
 }
 *data++ = s[i];

REPOSITORY
  R237 KConfig

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

To: vandenoever, dfaure, arichardson, apol
Cc: aacid, apol, kde-frameworks-devel, michaelh, ngraham, bruns

D17651: Escape bytes that are larger than or equal to 127 in config files

[Jos van den Oever]

vandenoever added a comment.


  Found it. This is the failing test.
  
FAIL!  : KConfigTest::testEncoding() Compared values are not the same
   Actual   (lines.first()): "[UTF-8:\\xc3\\xb6l]\n"
   Expected (QByteArray("[UTF-8:\xc3\xb6l]\n")): "[UTF-8:\xC3\xB6l]\n"
   Loc: [/home/jenkins/workspace/Frameworks/kconfig/kf5-qt5 
SUSEQt5.11/autotests/kconfigtest.cpp(463)]
  
  The test writes a config file with this group header:
  
  [UTF-8:öl]
  
  The above patch breaks that. Instead it outpus the escaped bytes:
  
  [UTF-8:\xc3\xb6]
  
  I'm working on a fix now.

REPOSITORY
  R237 KConfig

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

To: vandenoever, dfaure, arichardson, apol
Cc: aacid, apol, kde-frameworks-devel, michaelh, ngraham, bruns

D17651: Escape bytes that are larger than or equal to 127 in config files

[Jos van den Oever]

vandenoever added a comment.


  Can you give some more details? I see problem on Jenkins. 
https://build.kde.org/job/Frameworks/job/kconfig/
  
  Since my commits there was one more patch, but it's not related. 
https://cgit.kde.org/kconfig.git/commit/?id=d90a37b5a1d892d7b0ff7cc3b56c8a6e8c4bfe1a

REPOSITORY
  R237 KConfig

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

To: vandenoever, dfaure, arichardson, apol
Cc: aacid, apol, kde-frameworks-devel, michaelh, ngraham, bruns

D17651: Escape bytes that are larger than or equal to 127 in config files

[Jos van den Oever]

vandenoever marked 2 inline comments as done.
vandenoever added a comment.


  I pushed two encore commits fixing the two raised issues.

REPOSITORY
  R237 KConfig

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

To: vandenoever, dfaure, arichardson, apol
Cc: apol, kde-frameworks-devel, michaelh, ngraham, bruns

D17651: Escape bytes that are larger than or equal to 127 in config files

[Jos van den Oever]

This revision was automatically updated to reflect the committed changes.
Closed by commit R237:6a185285ae44: Escape bytes that are larger than or equal 
to 127 in config files (authored by vandenoever).

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17651?vs=47790=47796

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

AFFECTED FILES
  autotests/kconfigtest.cpp
  autotests/kconfigtest.h
  src/core/kconfigini.cpp

To: vandenoever, dfaure, arichardson, apol
Cc: apol, kde-frameworks-devel, michaelh, ngraham, bruns

D17651: Escape bytes that are larger than or equal to 127 in config files

[Jos van den Oever]

vandenoever updated this revision to Diff 47790.
vandenoever added a comment.


  Add a check for reading in the unit test.

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17651?vs=47788=47790

BRANCH
  utf8 (branched from master)

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

AFFECTED FILES
  autotests/kconfigtest.cpp
  autotests/kconfigtest.h
  src/core/kconfigini.cpp

To: vandenoever, dfaure, arichardson
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17651: Escape bytes that are larger than or equal to 127 in config files

[Jos van den Oever]

vandenoever retitled this revision from "Escape bytes that are larger than 127 
in config files" to "Escape bytes that are larger than or equal to 127 in 
config files".
vandenoever edited the summary of this revision.

REPOSITORY
  R237 KConfig

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

To: vandenoever, dfaure, arichardson
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17651: Escape bytes that are larger than 127 in config files

[Jos van den Oever]

vandenoever updated this revision to Diff 47788.
vandenoever edited the summary of this revision.
vandenoever added a comment.


  Added a unit test and changed the code so that 0x7f (DEL) is also 
  escaped.

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17651?vs=47759=47788

BRANCH
  utf8 (branched from master)

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

AFFECTED FILES
  autotests/kconfigtest.cpp
  autotests/kconfigtest.h
  src/core/kconfigini.cpp

To: vandenoever, dfaure, arichardson
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17651: Escape bytes that are larger than 127 in config files

[Jos van den Oever]

vandenoever retitled this revision from "Escape bytes that larger than 127" to 
"Escape bytes that are larger than 127 in config files".

REPOSITORY
  R237 KConfig

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

To: vandenoever, dfaure, arichardson
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17651: Escape bytes that larger than 127

[Jos van den Oever]

vandenoever added reviewers: dfaure, arichardson.

REPOSITORY
  R237 KConfig

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

To: vandenoever, dfaure, arichardson
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17651: Escape bytes that larger than 127

[Jos van den Oever]

vandenoever edited the summary of this revision.

REPOSITORY
  R237 KConfig

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

To: vandenoever
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17651: Escape bytes that larger than 127

[Jos van den Oever]

vandenoever edited the summary of this revision.

REPOSITORY
  R237 KConfig

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

To: vandenoever
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17651: Escape bytes that larger than 127

[Jos van den Oever]

vandenoever created this revision.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
vandenoever requested review of this revision.

REVISION SUMMARY
  UserBase tells me that KDE configuration files are encoded in UTF-8.
  https://userbase.kde.org/KDE_System_Administration/Configuration_Files
  
  In practice some *rc files have bytes outside that encoding.
  The problem is that there is no check for bytes larger than 127.

REPOSITORY
  R237 KConfig

BRANCH
  utf8 (branched from master)

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

AFFECTED FILES
  src/core/kconfigini.cpp

To: vandenoever
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13719: OdfExtractor: deal with non-common prefix names

[Jos van den Oever]

vandenoever accepted this revision.
vandenoever added a comment.
This revision is now accepted and ready to land.


  Good fixes. Just some small naming suggestions.

INLINE COMMENTS

> odfextractor.cpp:37
> +
> +QDomElement namedItemNS(const QDomNode , const QString , const 
> QString )
> +{

Perhaps name this `elementByTagNameNS`.

> odfextractor.cpp:39
> +{
> +for (auto n = node.firstChildElement(); !n.isNull(); n = 
> n.nextSiblingElement()) {
> +if (n.localName() == localName && n.namespaceURI() == nsURI) {

Variable name `e` for element is more logical.

REPOSITORY
  R286 KFileMetaData

BRANCH
  noncommonprefixodf

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

To: kossebau, vandenoever
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D6963: Don't block starting notification service

[Jos van den Oever]

vandenoever added a comment.


  The patch looks clean, but I do not fully understand how is supposed to work. 
Perhaps an explanation of the different scenarios would help future readers of 
this code.

REPOSITORY
  R289 KNotifications

BRANCH
  master

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

To: davidedmundson, mart
Cc: alexeymin, marten, vandenoever, mck182, dvratil, #frameworks

D7533: KIO: port the URI filter plugins from KServiceTypeTrader to json+KPluginMetaData

[Jos van den Oever]

vandenoever added a comment.


  > "X-KDE-InitialPreference": "5"
  
  is a string
  "X-KDE-InitialPreference": 5
  is a number

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

To: dfaure, apol, davidedmundson, arichardson
Cc: vandenoever, elvisangelaccio, bshah, #frameworks

D6963: Don't block starting notification service

[Jos van den Oever]

vandenoever added a comment.


  Removing this call is in line with the advice in the dbus spec.
  
  An improvement to the patch would be to remove `bool startfdo`.
  
  Q_WS_WIN seems to suggest that windows always needs to start the service 
manually. That case would break with this patch.

REPOSITORY
  R289 KNotifications

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

To: davidedmundson
Cc: vandenoever, mck182, dvratil, #frameworks

Re: Scrap baloo?

[Jos van den Oever]

On Wednesday 28 September 2016 18:09:12 David Edmundson wrote:
> A sizable part of your argument is based on problems with NFS . SQlite
> (that tracker uses) will surely have the same problems.
> Surely If file locks don't work, then file locks don't work?

CLucene does not have the same problem. It has files to which it appends. Once 
the files grow too large, they are merged into a new one, but the old ones 
remain available as long as they are used by an IndexReader.

As to the idea of pluggable indexes, that is what Strigi uses. It adds 
development overhead, but the tests can be shared.

Cheers,
Jos

normalizing svg files

[Jos van den Oever]

Hi all,

Icons in breeze are stored as uncompressed SVG. That should make diffs more 
readable. In practice, diffs can be quite hard to read due to different ways of 
writing out XML by different applications, application versions and settings.

A solution to this is to use XML normalization (c14n). xmllint provides this.

I've attached a script that will normalize XML files. I've used a similar 
script for quite some time.

  "$xmllint" --nonet --c14n11 "$1" \
| XMLLINT_INDENT=' ' \
  "$xmllint" --nonet --format --encode utf-8 --xmlout --nsclean - \
| "$perl" -p -e 's/[ \t]+$//g' - \
> "$tmpfile" || "$rm" "$tmpfile"

The script follows the c14n11 transform with a formatting. The formatting 
changes  to  and adds an initial .

The most notable feature is that attributes are sorted and placed on one line.

Like whitespace discussions, the actual choice is not that important, but for 
nice diffs, consistency is.

There's a slight space saving (1MB out of 64MB).

Cheers,
Jos

normalize.sh.gz
Description: application/gzip

Re: Review Request 128834: Check SVG icons for wellformedness

[Jos van den Oever]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128834/
---

(Updated sep 5, 2016, 9:12 p.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks, Plasma and Andreas Kainz.


Repository: breeze-icons


Description
---

Check SVG icons for wellformedness


Diffs
-

  CMakeLists.txt 1c18f3386106537b0573a7ba5a450bd5a4ee37c3 
  validate_svg.sh PRE-CREATION 

Diff: https://git.reviewboard.kde.org/r/128834/diff/


Testing
---

This detect a number of broken svg files at the moment. A patch is coming to 
fix those.


Thanks,

Jos van den Oever

Review Request 128836: Add missing namespace prefix definitions

[Jos van den Oever]

-gandhi.svg 
2779dc17f54ae135052472aa9d663c8e15418104 
  icons/applets/128/user-mowgli.svg d3631ec605411424c1b22c35c04a782b951c5fe4 
  icons/applets/128/user-none.svg 738a14e063c3a199685ee463c1f3a9ad5084 
  icons/applets/256/applets-template.svg 
1bb116339689241517ece0a6b689a71a0f689a0a 
  icons/applets/256/org.kde.plasma.binaryclock.svg 
1567be2a647dbad33373854595eaa88eb26dc43f 
  icons/applets/256/org.kde.plasma.calculator.svg 
62aee1768f85344dff909681dd39d93c0cae1ed0 
  icons/applets/256/org.kde.plasma.calendar.svg 
ea36210254ca9acf5045f1b7d2fb25eb2e2b4ce7 
  icons/applets/256/org.kde.plasma.networkmanagement.svg 
f1b93353b484ec24f55a1ec17e3c8bcd621d459c 
  icons/applets/256/org.kde.plasma.systemtray.svg 
69c1f19657a1911547e1dfbeefc7195e2b54915e 
  icons/applets/256/org.kde.plasma.userswitcher.svg 
3c1a2546d0d65a46713837534f3a717bf675d0a8 
  icons/mimetypes/32/text-x-apport.svg 0044532e31f35c74b3950282dd896d33e6d18db8 
  icons/mimetypes/64/text-x-apport.svg 8b2c46469e5f68e7c8ca7cbb6d7ce5d49f69b134 

Diff: https://git.reviewboard.kde.org/r/128836/diff/


Testing
---

The svg files were not wellformed because of missing namespace definitions.


Thanks,

Jos van den Oever

Review Request 128834: Check SVG icons for wellformedness

[Jos van den Oever]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128834/
---

Review request for KDE Frameworks.


Repository: breeze-icons


Description
---

Check SVG icons for wellformedness


Diffs
-

  CMakeLists.txt 1c18f3386106537b0573a7ba5a450bd5a4ee37c3 
  validate_svg.sh PRE-CREATION 

Diff: https://git.reviewboard.kde.org/r/128834/diff/


Testing
---

This detect a number of broken svg files at the moment. A patch is coming to 
fix those.


Thanks,

Jos van den Oever

Re: Review Request 127786: Remove custom read functions for QString and QStringList

[Jos van den Oever]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127786/
---

(Updated April 30, 2016, 1:50 a.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks, David Faure and Milian Wolff.


Changes
---

Submitted with commit b4fea4e48455fa65c0ea530df30500a15bfd1452 by Jos van den 
Oever to branch master.


Repository: kservice


Description
---

Writing KBuildSycoca is done with <<. Up till now there were special 'safe' 
functions for reading QString and QStringList. They only limited the size of 
QString and the number of allowed entries in QStringList. The cache file is 
created by the trusted system. If file size is an attack vector, these safe 
functions are useful and we should keep them.

This patch is three commits:

1)  Use the standard read function for reading QStringList


2)  Use the standard read function for reading QString


3)  Remove redundant #include

ksycocaentry.h is included via kservice.h


Diffs
-

  src/CMakeLists.txt f4d09d5 
  src/services/kservicegroup.h c046314 
  src/services/kservicetypefactory.cpp 2edc57c 
  src/sycoca/kctimefactory.cpp a8c7846 
  src/sycoca/ksycoca.cpp 5d43ef4 
  src/sycoca/ksycoca_p.h 119c3ee 
  src/sycoca/ksycocaentry.cpp 5fbd158 
  src/sycoca/ksycocautils.cpp 84998b7 
  src/sycoca/ksycocautils_p.h aad9d50 

Diff: https://git.reviewboard.kde.org/r/127786/diff/


Testing
---

All tests still pass.


Thanks,

Jos van den Oever

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127770: Increase maximum string length in KSycoca database

[Jos van den Oever]

> On apr 28, 2016, 9:12 a.m., Milian Wolff wrote:
> > src/sycoca/ksycocautils.cpp, line 29
> > <https://git.reviewboard.kde.org/r/127770/diff/2/?file=461459#file461459line29>
> >
> > as a follow-up cleanup I suggest to introduce an enum to hold this 
> > magic value

I can do that if the code stays. I made another rr to remove it: 
https://git.reviewboard.kde.org/r/127786/


- Jos


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127770/#review94946
-----------


On apr 28, 2016, 7:01 a.m., Jos van den Oever wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127770/
> ---
> 
> (Updated apr 28, 2016, 7:01 a.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Repository: kservice
> 
> 
> Description
> ---
> 
> The code for reading and writing of strings in KSycoca is not symmetrical. 
> Strings of any length can be written, but only strings of less than 8192 
> bytes may be read. This limit is set in KSycocaUtilsPrivate::read. The limit 
> is probably there to avoid out-of-memory situations.
> 
> On my system I have a lot of XDG data dirs. The length of the environment 
> variable is currently 4092 bytes. KSycocaBuild saves that as UTF-16 which 
> needs 8184 bytes. KBuildSycoca save that without complaint but complains when 
> reading it.
> 
> 
> The simplest solution here is to simply increase the magic number 8192 to 
> e.g. 65528. This is just a temporary buffer.
> 
> Or we just check the size of the whole cache file (e.g. < 100M) and remove 
> all other limits. That would simplify
> 
>KSycocaUtilsPrivate::read(*str, header.prefixes);
> 
> to
> 
>*str >> header.prefixes;
> 
> This patch chooses the first option.
> 
> 
> Diffs
> -
> 
>   src/sycoca/ksycocautils.cpp 1ba75e8 
> 
> Diff: https://git.reviewboard.kde.org/r/127770/diff/
> 
> 
> Testing
> ---
> 
> Ran unit tests on KService. All but one of the previously failing tests on 
> NixOS is now fixed.
> 
> 
> Thanks,
> 
> Jos van den Oever
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127786: Remove custom read functions for QString and QStringList

[Jos van den Oever]

> On apr 29, 2016, 10:56 a.m., David Faure wrote:
> > This is not about trust and attacks, this is about not allocating 4 GB of 
> > RAM when reading a corrupted binary file.
> 
> Jos van den Oever wrote:
> That will only happen if the file or stream is 4 GB. `QDataStream 
> >>(QDataStream , QString )` allocates while reading in 1 MiB 
> chunks.

http://code.qt.io/cgit/qt/qtbase.git/tree/src/corelib/tools/qstring.cpp#n8637


- Jos


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127786/#review95012
-------


On apr 29, 2016, 10:22 a.m., Jos van den Oever wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127786/
> ---
> 
> (Updated apr 29, 2016, 10:22 a.m.)
> 
> 
> Review request for KDE Frameworks, David Faure and Milian Wolff.
> 
> 
> Repository: kservice
> 
> 
> Description
> ---
> 
> Writing KBuildSycoca is done with <<. Up till now there were special 'safe' 
> functions for reading QString and QStringList. They only limited the size of 
> QString and the number of allowed entries in QStringList. The cache file is 
> created by the trusted system. If file size is an attack vector, these safe 
> functions are useful and we should keep them.
> 
> This patch is three commits:
> 
> 1)  Use the standard read function for reading QStringList
> 
> 
> 2)  Use the standard read function for reading QString
> 
> 
> 3)  Remove redundant #include
> 
> ksycocaentry.h is included via kservice.h
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt f4d09d5 
>   src/services/kservicegroup.h c046314 
>   src/services/kservicetypefactory.cpp 2edc57c 
>   src/sycoca/kctimefactory.cpp a8c7846 
>   src/sycoca/ksycoca.cpp 5d43ef4 
>   src/sycoca/ksycoca_p.h 119c3ee 
>   src/sycoca/ksycocaentry.cpp 5fbd158 
>   src/sycoca/ksycocautils.cpp 84998b7 
>   src/sycoca/ksycocautils_p.h aad9d50 
> 
> Diff: https://git.reviewboard.kde.org/r/127786/diff/
> 
> 
> Testing
> ---
> 
> All tests still pass.
> 
> 
> Thanks,
> 
> Jos van den Oever
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127786: Remove custom read functions for QString and QStringList

[Jos van den Oever]

> On apr 29, 2016, 10:56 a.m., David Faure wrote:
> > This is not about trust and attacks, this is about not allocating 4 GB of 
> > RAM when reading a corrupted binary file.

That will only happen if the file or stream is 4 GB. `QDataStream 
>>(QDataStream , QString )` allocates while reading in 1 MiB 
chunks.


- Jos


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127786/#review95012
---


On apr 29, 2016, 10:22 a.m., Jos van den Oever wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127786/
> ---
> 
> (Updated apr 29, 2016, 10:22 a.m.)
> 
> 
> Review request for KDE Frameworks, David Faure and Milian Wolff.
> 
> 
> Repository: kservice
> 
> 
> Description
> ---
> 
> Writing KBuildSycoca is done with <<. Up till now there were special 'safe' 
> functions for reading QString and QStringList. They only limited the size of 
> QString and the number of allowed entries in QStringList. The cache file is 
> created by the trusted system. If file size is an attack vector, these safe 
> functions are useful and we should keep them.
> 
> This patch is three commits:
> 
> 1)  Use the standard read function for reading QStringList
> 
> 
> 2)  Use the standard read function for reading QString
> 
> 
> 3)  Remove redundant #include
> 
> ksycocaentry.h is included via kservice.h
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt f4d09d5 
>   src/services/kservicegroup.h c046314 
>   src/services/kservicetypefactory.cpp 2edc57c 
>   src/sycoca/kctimefactory.cpp a8c7846 
>   src/sycoca/ksycoca.cpp 5d43ef4 
>   src/sycoca/ksycoca_p.h 119c3ee 
>   src/sycoca/ksycocaentry.cpp 5fbd158 
>   src/sycoca/ksycocautils.cpp 84998b7 
>   src/sycoca/ksycocautils_p.h aad9d50 
> 
> Diff: https://git.reviewboard.kde.org/r/127786/diff/
> 
> 
> Testing
> ---
> 
> All tests still pass.
> 
> 
> Thanks,
> 
> Jos van den Oever
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Review Request 127786: Remove custom read functions for QString and QStringList

[Jos van den Oever]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127786/
---

Review request for KDE Frameworks, David Faure and Milian Wolff.


Repository: kservice


Description
---

Writing KBuildSycoca is done with <<. Up till now there were special 'safe' 
functions for reading QString and QStringList. They only limited the size of 
QString and the number of allowed entries in QStringList. The cache file is 
created by the trusted system. If file size is an attack vector, these safe 
functions are useful and we should keep them.

This patch is three commits:

1)  Use the standard read function for reading QStringList


2)  Use the standard read function for reading QString


3)  Remove redundant #include

ksycocaentry.h is included via kservice.h


Diffs
-

  src/CMakeLists.txt f4d09d5 
  src/services/kservicegroup.h c046314 
  src/services/kservicetypefactory.cpp 2edc57c 
  src/sycoca/kctimefactory.cpp a8c7846 
  src/sycoca/ksycoca.cpp 5d43ef4 
  src/sycoca/ksycoca_p.h 119c3ee 
  src/sycoca/ksycocaentry.cpp 5fbd158 
  src/sycoca/ksycocautils.cpp 84998b7 
  src/sycoca/ksycocautils_p.h aad9d50 

Diff: https://git.reviewboard.kde.org/r/127786/diff/


Testing
---

All tests still pass.


Thanks,

Jos van den Oever

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127770: Increase maximum string length in KSycoca database

[Jos van den Oever]

> On apr 27, 2016, 8:36 p.m., David Faure wrote:
> > src/sycoca/ksycocautils.cpp, line 39
> > <https://git.reviewboard.kde.org/r/127770/diff/1/?file=461435#file461435line39>
> >
> > Variable-length arrays are invalid C++.
> > 
> > gcc might accept it, but -pedantic won't, and other compilers might not.
> > 
> > See e.g. 
> > http://stackoverflow.com/questions/1887097/variable-length-arrays-in-c 
> > (which is about C++, the '+'s got removed from the URL)
> > 
> > You can use an enum value to factorize the 8192. I think "static const 
> > int" won't do, if I read http://en.cppreference.com/w/cpp/language/array 
> > correctly.
> 
> Albert Astals Cid wrote:
> I'm not sure if it's up to standard or not but both g++ and clang++ 
> compile https://paste.kde.org/pfrytc0no with -pendantic
> 
> I wouldn't call it "variable-length" since the length is known at compile 
> time.
> 
> Also 
> http://stackoverflow.com/questions/1327806/do-all-c-compilers-allow-using-a-static-const-int-class-member-variable-as-an
>  seems to say it's fine.

i chose to keep it simple and stick to literals


> On apr 27, 2016, 8:36 p.m., David Faure wrote:
> > src/sycoca/ksycocautils.cpp, line 27
> > <https://git.reviewboard.kde.org/r/127770/diff/1/?file=461435#file461435line27>
> >
> > static ? But see next item.

i chose to keep it simple and stick to literals


- Jos


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127770/#review94914
---


On apr 27, 2016, 8:44 p.m., Jos van den Oever wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127770/
> ---
> 
> (Updated apr 27, 2016, 8:44 p.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Repository: kservice
> 
> 
> Description
> ---
> 
> The code for reading and writing of strings in KSycoca is not symmetrical. 
> Strings of any length can be written, but only strings of less than 8192 
> bytes may be read. This limit is set in KSycocaUtilsPrivate::read. The limit 
> is probably there to avoid out-of-memory situations.
> 
> On my system I have a lot of XDG data dirs. The length of the environment 
> variable is currently 4092 bytes. KSycocaBuild saves that as UTF-16 which 
> needs 8184 bytes. KBuildSycoca save that without complaint but complains when 
> reading it.
> 
> 
> The simplest solution here is to simply increase the magic number 8192 to 
> e.g. 65528. This is just a temporary buffer.
> 
> Or we just check the size of the whole cache file (e.g. < 100M) and remove 
> all other limits. That would simplify
> 
>KSycocaUtilsPrivate::read(*str, header.prefixes);
> 
> to
> 
>*str >> header.prefixes;
> 
> This patch chooses the first option.
> 
> 
> Diffs
> -
> 
>   src/sycoca/ksycocautils.cpp 1ba75e8 
> 
> Diff: https://git.reviewboard.kde.org/r/127770/diff/
> 
> 
> Testing
> ---
> 
> Ran unit tests on KService. All but one of the previously failing tests on 
> NixOS is now fixed.
> 
> 
> Thanks,
> 
> Jos van den Oever
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127770: Increase maximum string length in KSycoca database

[Jos van den Oever]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127770/
---

(Updated apr 27, 2016, 8:44 p.m.)


Review request for KDE Frameworks and David Faure.


Changes
---

Use literals instead of variable to be on the safe side wrt compiler 
compatibility.


Repository: kservice


Description
---

The code for reading and writing of strings in KSycoca is not symmetrical. 
Strings of any length can be written, but only strings of less than 8192 bytes 
may be read. This limit is set in KSycocaUtilsPrivate::read. The limit is 
probably there to avoid out-of-memory situations.

On my system I have a lot of XDG data dirs. The length of the environment 
variable is currently 4092 bytes. KSycocaBuild saves that as UTF-16 which needs 
8184 bytes. KBuildSycoca save that without complaint but complains when reading 
it.


The simplest solution here is to simply increase the magic number 8192 to e.g. 
65528. This is just a temporary buffer.

Or we just check the size of the whole cache file (e.g. < 100M) and remove all 
other limits. That would simplify

   KSycocaUtilsPrivate::read(*str, header.prefixes);

to

   *str >> header.prefixes;

This patch chooses the first option.


Diffs (updated)
-

  src/sycoca/ksycocautils.cpp 1ba75e8 

Diff: https://git.reviewboard.kde.org/r/127770/diff/


Testing
---

Ran unit tests on KService. All but one of the previously failing tests on 
NixOS is now fixed.


Thanks,

Jos van den Oever

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

solving 'KSycoca database corruption!'

[Jos van den Oever]

Hi all,

A few days back dvratil reported errors in the KDE PIM unit tests. The tests 
were writing

  QWARN  : TestBlogger1::testNetwork() kf5.kservice.sycoca: ERROR: KSycoca 
database corruption!

On my laptop, I was seeing the same error when starting KF5 applications. I've 
spent some time in the debugger today and I know what is going on in my 
laptop.

The KSycoca (KDE System Configuration Cache) database is a file with cached 
information. It is created in KBuildSycoca::save. The file currently contains 
four types of information: KMimeType, KServiceType, KService and 
KServiceGroup. The top of the KSycoca file lists
 - the format version
 - the offsets for the four different parts
 - XDG_DATA_DIRS for which it was created
 - timestamp
 - locale
 - some hash
 - paths to the cached directories with their mtimes

The problems is this: the code for reading and writing of strings is not 
symmetrical. Strings of any length can be written, but only strings of less 
than 8192 bytes may be read. This limit is set in KSycocaUtilsPrivate::read. 
The limit is probably there to avoid out-of-memory situations.

On my laptop I have a lot of XDG data dirs. The length of the environment 
variable is currently 4092 bytes. KSycocaBuild saves that as UTF-16 which 
needs 8184 bytes. KBuildSycoca save that without complaint but complains when 
reading it.

Why would i need such a long XDG_DATA_DIRS? That is because on NixOS each 
package has its own directory. This leads to this:

 XDG_DATA_DIRS=
  /nix/store/0qm2gkssnfslm6nwhd3klwm54yhqx5rx-dbus-1.8.20/share
 :/nix/store/r6j1sqknviayvqicy8zw049am0q6wywj-kcoreaddons-5.21.0/share
 :/nix/store/qiq2vxxxsj0clv209xsv7w7yv2wrsbqb-breeze-icons-5.21.0/share
 :/nix/store/l4ggdlcnj3sfky4zjlirf0hmqphzzc3y-kjobwidgets-5.21.0/share
 .

In such a setup a working KSycoca really helps.

The simplest solution here is to simply increase the magic number 8192 to e.g. 
65528. This is just a temporary buffer.

Or we just check the size of the whole cache file (e.g. < 100M) and remove all 
other limits. That would simplify

   KSycocaUtilsPrivate::read(*str, header.prefixes);

to

   *str >> header.prefixes;

Increasing the buffer size [1] fixes all but one of the failing unit test of 
KService on NixOS.

Cheers,
Jos

[1] https://git.reviewboard.kde.org/r/127770/

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Review Request 127770: Increase maximum string length in KSycoca database

[Jos van den Oever]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127770/
---

Review request for KDE Frameworks and David Faure.


Repository: kservice


Description
---

The code for reading and writing of strings in KSycoca is not symmetrical. 
Strings of any length can be written, but only strings of less than 8192 bytes 
may be read. This limit is set in KSycocaUtilsPrivate::read. The limit is 
probably there to avoid out-of-memory situations.

On my system I have a lot of XDG data dirs. The length of the environment 
variable is currently 4092 bytes. KSycocaBuild saves that as UTF-16 which needs 
8184 bytes. KBuildSycoca save that without complaint but complains when reading 
it.


The simplest solution here is to simply increase the magic number 8192 to e.g. 
65528. This is just a temporary buffer.

Or we just check the size of the whole cache file (e.g. < 100M) and remove all 
other limits. That would simplify

   KSycocaUtilsPrivate::read(*str, header.prefixes);

to

   *str >> header.prefixes;

This patch chooses the first option.


Diffs
-

  src/sycoca/ksycocautils.cpp 1ba75e8 

Diff: https://git.reviewboard.kde.org/r/127770/diff/


Testing
---

Ran unit tests on KService. All but one of the previously failing tests on 
NixOS is now fixed.


Thanks,

Jos van den Oever

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127757: Don't look-up kbuildsycoca5 on kservice tests

[Jos van den Oever]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127757/#review94905
---



Much more elegant than my approach. Only one comment. I missed on place in 
ksycocatest.cpp:

--- a/autotests/ksycocatest.cpp
+++ b/autotests/ksycocatest.cpp
@@ -223,7 +223,7 @@ void KSycocaTest::dirTimestampShouldBeCheckedRecursively()
 void KSycocaTest::runKBuildSycoca(const QProcessEnvironment , bool 
global)
 {
 QProcess proc;
-const QString kbuildsycoca = 
QStandardPaths::findExecutable(KBUILDSYCOCA_EXENAME);
+const QString kbuildsycoca = QStringLiteral(KBUILDSYCOCAEXE);
 QVERIFY(!kbuildsycoca.isEmpty());
 QStringList args;
 args << "--testmode";

- Jos van den Oever


On apr 27, 2016, 11:38 a.m., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127757/
> ---
> 
> (Updated apr 27, 2016, 11:38 a.m.)
> 
> 
> Review request for KDE Frameworks and Jos van den Oever.
> 
> 
> Repository: kservice
> 
> 
> Description
> ---
> 
> This way we make sure the correct kbuildsycoca is tested.
> 
> 
> Diffs
> -
> 
>   autotests/CMakeLists.txt 52e7298 
>   autotests/ksycoca_xdgdirstest.cpp 1aa7140 
> 
> Diff: https://git.reviewboard.kde.org/r/127757/diff/
> 
> 
> Testing
> ---
> 
> Tests still pass here.
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127754: Use kbuildsycoca5 executable that was just built, not the one from the system

[Jos van den Oever]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127754/
---

(Updated apr 27, 2016, 12:47 p.m.)


Status
--

This change has been discarded.


Review request for KDE Frameworks.


Repository: kservice


Description
---

This test was using QStandardPaths::findExecutable to find buildsycoca5. So the 
test was testing with the wrong buildsycoca5.

I've added a function findFile() that looks for buildsycoca5 in the build 
directory.


Diffs
-

  autotests/ksycoca_xdgdirstest.cpp f879959 

Diff: https://git.reviewboard.kde.org/r/127754/diff/


Testing
---

I ran the unit tests. Some now pass for me, but on my system still some are 
failing.


Thanks,

Jos van den Oever

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Review Request 127754: Use kbuildsycoca5 executable that was just built, not the one from the system

[Jos van den Oever]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127754/
---

Review request for KDE Frameworks.


Repository: kservice


Description
---

This test was using QStandardPaths::findExecutable to find buildsycoca5. So the 
test was testing with the wrong buildsycoca5.

I've added a function findFile() that looks for buildsycoca5 in the build 
directory.


Diffs
-

  autotests/ksycoca_xdgdirstest.cpp f879959 

Diff: https://git.reviewboard.kde.org/r/127754/diff/


Testing
---

I ran the unit tests. Some now pass for me, but on my system still some are 
failing.


Thanks,

Jos van den Oever

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel