Review Request 126413: FindPyKDE4: Make PYKDE4_INSTALL_PYTHON_FILES use PYTHON_INSTALL.
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126413/
---
Review request for Build System, kdelibs, Alex Merry, and Luca Beltrame.
Repository: kdelibs
Description
---
Review Request 126345 ("PythonMacros: specify destination directory in
byte-compiled files") broke Kajongg's build because it uses the
`PYKDE4_INSTALL_PYTHON_FILES()` macro, whose use of `PythonCompile.py` had not
been updated.
Instead of just passing `--destination-dir` in `FindPyKDE4.cmake`, rewrite the
`PYKDE4_INSTALL_PYTHON_FILES()` macro to use PythonMacros's `PYTHON_INSTALL()`.
Not only does this fix Kajongg's build, but it also removes a lot of code
duplication and makes `PYKDE4_INSTALL_PYTHON_FILES()` work with Python 3.2+'s
different .pyc location.
Diffs
-
cmake/modules/FindPyKDE4.cmake 3b87963
Diff: https://git.reviewboard.kde.org/r/126413/diff/
Testing
---
Kajongg builds again. According to LXR `FindPyKDE4.cmake` and `PythonMacros.py`
are the only two places invoking `PythonCompile.py`, so all callers should work
now.
Thanks,
Raphael Kubo da Costa
Re: Review Request 126345: PythonMacros: specify destination directory in byte-compiled files
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126345/ --- (Updated Dec. 16, 2015, 5:30 p.m.) Status -- This change has been marked as submitted. Review request for Build System, kdelibs and Alex Merry. Changes --- Submitted with commit 94f1d2fa9582a2942d5154b85c849cc3c6140e31 by Raphael Kubo da Costa to branch KDE/4.14. Repository: kdelibs Description --- The `PYTHON_INSTALL()` macro is a wrapper around the `py_compile` Python module that also installs the byte-code (.pyc) file it generates. However, when a .py file is passed to `py_compile` without any additional arguments, its full path is recorded in the .pyc file. This is problematic, as most distributions install all files into a build root instead of simply copying files to `/` as part of the packaging process. In this case, the generated .pyc file will have something like /wrkdir/buildroot/usr/lib/python2.7/site-packages/Foo/my_module.py in it. Not only does this show up in exception tracebacks, but if the user later invokes `my_module.py` and has write access to `my_module`'s directory, `my_module.pyc` will be rewritten with the right path to `my_module.py` (without the build root). This can lead to uninstallation errors if the package management system checks each file before removal, for example. Fix it by rewritting the `PythonCompile.py` script so that it takes a `--destination-dir` argument that we use to pass the full path to `my_module.py` instead of letting it be (wrongly) deduced. Diffs - cmake/modules/PythonCompile.py 156fea2 cmake/modules/PythonMacros.cmake 6a82d88 Diff: https://git.reviewboard.kde.org/r/126345/diff/ Testing --- Fedora has packaging macros that will regenerate .pyc and .pyo files with the right paths as part of the build, so it is not affected. Debian disables this macro in pykde4, FreeBSD and openSUSE contain wrong paths in its .pyc files. With this patch, I was able to verify with `hexdump` that the right path is present in the .pyc files installed by pykde4. Thanks, Raphael Kubo da Costa
Review Request 126345: PythonMacros: specify destination directory in byte-compiled files
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126345/ --- Review request for Build System, kdelibs and Alex Merry. Repository: kdelibs Description --- The `PYTHON_INSTALL()` macro is a wrapper around the `py_compile` Python module that also installs the byte-code (.pyc) file it generates. However, when a .py file is passed to `py_compile` without any additional arguments, its full path is recorded in the .pyc file. This is problematic, as most distributions install all files into a build root instead of simply copying files to `/` as part of the packaging process. In this case, the generated .pyc file will have something like /wrkdir/buildroot/usr/lib/python2.7/site-packages/Foo/my_module.py in it. Not only does this show up in exception tracebacks, but if the user later invokes `my_module.py` and has write access to `my_module`'s directory, `my_module.pyc` will be rewritten with the right path to `my_module.py` (without the build root). This can lead to uninstallation errors if the package management system checks each file before removal, for example. Fix it by rewritting the `PythonCompile.py` script so that it takes a `--destination-dir` argument that we use to pass the full path to `my_module.py` instead of letting it be (wrongly) deduced. Diffs - cmake/modules/PythonCompile.py 156fea2 cmake/modules/PythonMacros.cmake 6a82d88 Diff: https://git.reviewboard.kde.org/r/126345/diff/ Testing --- Fedora has packaging macros that will regenerate .pyc and .pyo files with the right paths as part of the build, so it is not affected. Debian disables this macro in pykde4, FreeBSD and openSUSE contain wrong paths in its .pyc files. With this patch, I was able to verify with `hexdump` that the right path is present in the .pyc files installed by pykde4. Thanks, Raphael Kubo da Costa
Re: Review Request 126345: PythonMacros: specify destination directory in byte-compiled files
> On Dec. 14, 2015, 8:24 p.m., Alex Merry wrote: > > Seems sensible to me. Are there any potential Python2 vs Python3 issues? I don't think there are any -- the `.pyc/.pyo` files are installed with a different name into a different location on Python >= 3.2, but that is already taken care of by the macro. This patch only deals with the path where the `.py` file gets installed, which it does not change. The only problem I see is in the use of the `argparse` module: it does not exist on Python < 2.7, Python 3.0 and Python 3.1. If that's an issue, I can rewrite the code to use `optparse`, which is deprecated but exists on all Python versions, including the deprecated ones. - Raphael --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126345/#review89485 --- On Dec. 14, 2015, 6:48 p.m., Raphael Kubo da Costa wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/126345/ > --- > > (Updated Dec. 14, 2015, 6:48 p.m.) > > > Review request for Build System, kdelibs and Alex Merry. > > > Repository: kdelibs > > > Description > --- > > The `PYTHON_INSTALL()` macro is a wrapper around the `py_compile` Python > module that also installs the byte-code (.pyc) file it generates. > > However, when a .py file is passed to `py_compile` without any additional > arguments, its full path is recorded in the .pyc file. This is problematic, > as most distributions install all files into a build root instead of simply > copying files to `/` as part of the packaging process. In this case, the > generated .pyc file will have something like > /wrkdir/buildroot/usr/lib/python2.7/site-packages/Foo/my_module.py > in it. Not only does this show up in exception tracebacks, but if the user > later invokes `my_module.py` and has write access to `my_module`'s directory, > `my_module.pyc` will be rewritten with the right path to `my_module.py` > (without the build root). This can lead to uninstallation errors if the > package management system checks each file before removal, for example. > > Fix it by rewritting the `PythonCompile.py` script so that it takes a > `--destination-dir` argument that we use to pass the full path to > `my_module.py` instead of letting it be (wrongly) deduced. > > > Diffs > - > > cmake/modules/PythonCompile.py 156fea2 > cmake/modules/PythonMacros.cmake 6a82d88 > > Diff: https://git.reviewboard.kde.org/r/126345/diff/ > > > Testing > --- > > Fedora has packaging macros that will regenerate .pyc and .pyo files with the > right paths as part of the build, so it is not affected. Debian disables this > macro in pykde4, FreeBSD and openSUSE contain wrong paths in its .pyc files. > With this patch, I was able to verify with `hexdump` that the right path is > present in the .pyc files installed by pykde4. > > > Thanks, > > Raphael Kubo da Costa > >
Re: Review Request 126345: PythonMacros: specify destination directory in byte-compiled files
> On Dec. 14, 2015, 8:24 p.m., Alex Merry wrote: > > Seems sensible to me. Are there any potential Python2 vs Python3 issues? > > Raphael Kubo da Costa wrote: > I don't think there are any -- the `.pyc/.pyo` files are installed with a > different name into a different location on Python >= 3.2, but that is > already taken care of by the macro. This patch only deals with the path where > the `.py` file gets installed, which it does not change. > > The only problem I see is in the use of the `argparse` module: it does > not exist on Python < 2.7, Python 3.0 and Python 3.1. If that's an issue, I > can rewrite the code to use `optparse`, which is deprecated but exists on all > Python versions, including the deprecated ones. > > Alex Merry wrote: > I guess it depends whether it effectively causes a dependency bump over > what was previously required, and whether that will have any practical impact > on people upgrading kdelibs if it does (ie: does anyone actually use those > Python versions any more?). It has an effect on kdelibs's run-time dependencies in the sense that projects (PyKDE4 etc) using the `PYTHON_INSTALL()` will need either Python >= 2.7 or Python >= 3.2. I doubt anyone is using Python 2.6 with a recent kdelibs these days (even Debian stable and oldstable ship Python 2.7), and if anyone is trying to use Python 3 with kdelibs they are very likely to be using a more recent version than 3.1 or 3.0. My guess is that this is not a big deal, and rewriting the code to use `optparse` is trivial should any bug reports be filed. - Raphael --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126345/#review89485 --------------- On Dec. 14, 2015, 6:48 p.m., Raphael Kubo da Costa wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/126345/ > --- > > (Updated Dec. 14, 2015, 6:48 p.m.) > > > Review request for Build System, kdelibs and Alex Merry. > > > Repository: kdelibs > > > Description > --- > > The `PYTHON_INSTALL()` macro is a wrapper around the `py_compile` Python > module that also installs the byte-code (.pyc) file it generates. > > However, when a .py file is passed to `py_compile` without any additional > arguments, its full path is recorded in the .pyc file. This is problematic, > as most distributions install all files into a build root instead of simply > copying files to `/` as part of the packaging process. In this case, the > generated .pyc file will have something like > /wrkdir/buildroot/usr/lib/python2.7/site-packages/Foo/my_module.py > in it. Not only does this show up in exception tracebacks, but if the user > later invokes `my_module.py` and has write access to `my_module`'s directory, > `my_module.pyc` will be rewritten with the right path to `my_module.py` > (without the build root). This can lead to uninstallation errors if the > package management system checks each file before removal, for example. > > Fix it by rewritting the `PythonCompile.py` script so that it takes a > `--destination-dir` argument that we use to pass the full path to > `my_module.py` instead of letting it be (wrongly) deduced. > > > Diffs > - > > cmake/modules/PythonCompile.py 156fea2 > cmake/modules/PythonMacros.cmake 6a82d88 > > Diff: https://git.reviewboard.kde.org/r/126345/diff/ > > > Testing > --- > > Fedora has packaging macros that will regenerate .pyc and .pyo files with the > right paths as part of the build, so it is not affected. Debian disables this > macro in pykde4, FreeBSD and openSUSE contain wrong paths in its .pyc files. > With this patch, I was able to verify with `hexdump` that the right path is > present in the .pyc files installed by pykde4. > > > Thanks, > > Raphael Kubo da Costa > >
Re: Review Request 125236: FindQt4: Use CHECK_CXX_SYMBOL_EXISTS instead of CHECK_C_SYMBOL_EXISTS.
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125236/
---
(Updated Nov. 29, 2015, 8:15 p.m.)
Status
--
This change has been discarded.
Review request for Build System and kdelibs.
Repository: kdelibs
Description
---
Backport upstream CMake commit de30173d ("Remove C compiler requirement
from FindQt4.cmake") from 2011 so that we check for symbols such as the
Q_WS_* ones with a C++ compiler instead of a C one.
Diffs
-
cmake/modules/FindQt4.cmake e439a72f82c51758830b057b368407e11a6a541e
Diff: https://git.reviewboard.kde.org/r/125236/diff/
Testing
---
Adding something C++-specific to qglobal.h (e.g. `#include `)
works and the right flags are still detected.
Thanks,
Raphael Kubo da Costa
Re: Review Request 125236: FindQt4: Use CHECK_CXX_SYMBOL_EXISTS instead of CHECK_C_SYMBOL_EXISTS.
> On Sept. 17, 2015, 1:01 p.m., Alex Merry wrote:
> > This change is a correct one to make, but I've slightly lost track of the
> > kdelibs release policy.
> >
> > This patch isn't fixing an issue anyone is actually hitting, right? And
> > it's not like qglobal.h in Qt 4 is going to have any significant changes
> > made to it. So it's possible this doesn't pass the threshold of bug fixes
> > for kdelibs, which is permanently frozen to some degree.
> This patch isn't fixing an issue anyone is actually hitting, right?
I was going to say we had to do it downstream for FreeBSD because the oldest
supported release can end up using clang with a very old libstdc++ without
C++11 support, but since then I've managed to put all the required changes in
our Qt package :-)
- Raphael
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125236/#review85550
-------
On Sept. 15, 2015, 11:59 a.m., Raphael Kubo da Costa wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125236/
> ---
>
> (Updated Sept. 15, 2015, 11:59 a.m.)
>
>
> Review request for Build System and kdelibs.
>
>
> Repository: kdelibs
>
>
> Description
> ---
>
> Backport upstream CMake commit de30173d ("Remove C compiler requirement
> from FindQt4.cmake") from 2011 so that we check for symbols such as the
> Q_WS_* ones with a C++ compiler instead of a C one.
>
>
> Diffs
> -
>
> cmake/modules/FindQt4.cmake e439a72f82c51758830b057b368407e11a6a541e
>
> Diff: https://git.reviewboard.kde.org/r/125236/diff/
>
>
> Testing
> ---
>
> Adding something C++-specific to qglobal.h (e.g. `#include `)
> works and the right flags are still detected.
>
>
> Thanks,
>
> Raphael Kubo da Costa
>
>
Review Request 125236: FindQt4: Use CHECK_CXX_SYMBOL_EXISTS instead of CHECK_C_SYMBOL_EXISTS.
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125236/
---
Review request for Build System and kdelibs.
Repository: kdelibs
Description
---
Backport upstream CMake commit de30173d ("Remove C compiler requirement
from FindQt4.cmake") from 2011 so that we check for symbols such as the
Q_WS_* ones with a C++ compiler instead of a C one.
Diffs
-
cmake/modules/FindQt4.cmake e439a72f82c51758830b057b368407e11a6a541e
Diff: https://git.reviewboard.kde.org/r/125236/diff/
Testing
---
Adding something C++-specific to qglobal.h (e.g. `#include `)
works and the right flags are still detected.
Thanks,
Raphael Kubo da Costa
Re: Review Request 120900: kio_sftp: Use the right type for timeout_sec and timeout_usec.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120900/ --- (Updated Oct. 30, 2014, 11:52 a.m.) Status -- This change has been marked as submitted. Review request for KDE Runtime and Andreas Schneider. Bugs: 331674 http://bugs.kde.org/show_bug.cgi?id=331674 Repository: kde-runtime Description --- libssh expects the values passed to the SSH_OPTIONS_TIMEOUT and SSH_OPTIONS_TIMEOUT_USEC to be longs, not plain ints. On 64-bit platforms with sizeof(long) sizeof(int), this mismatch can be problematic and potentially result in invalid memory access that causes the calls to ssh_options_set() to fail. Diffs - kioslave/sftp/kio_sftp.cpp 21cffac0e5892944ca6c5b74537ebc4b7fb4738e Diff: https://git.reviewboard.kde.org/r/120900/diff/ Testing --- This is a bit hard to test, but with some tinkering in `sftpProtocol::sftpOpenConnection()` I can get `ssh_options_set()` to read parts of the argument passed to `sftpOpenConnection()` in addition to the actual value passed to `ssh_options_set()` and make the ioslave always say that it failed to set the timeout value. With this change, everything works as expected. Thanks, Raphael Kubo da Costa
Review Request 120900: kio_sftp: Use the right type for timeout_sec and timeout_usec.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120900/ --- Review request for KDE Runtime and Andreas Schneider. Bugs: 331674 http://bugs.kde.org/show_bug.cgi?id=331674 Repository: kde-runtime Description --- libssh expects the values passed to the SSH_OPTIONS_TIMEOUT and SSH_OPTIONS_TIMEOUT_USEC to be longs, not plain ints. On 64-bit platforms with sizeof(long) sizeof(int), this mismatch can be problematic and potentially result in invalid memory access that causes the calls to ssh_options_set() to fail. Diffs - kioslave/sftp/kio_sftp.cpp 21cffac0e5892944ca6c5b74537ebc4b7fb4738e Diff: https://git.reviewboard.kde.org/r/120900/diff/ Testing --- This is a bit hard to test, but with some tinkering in `sftpProtocol::sftpOpenConnection()` I can get `ssh_options_set()` to read parts of the argument passed to `sftpOpenConnection()` in addition to the actual value passed to `ssh_options_set()` and make the ioslave always say that it failed to set the timeout value. With this change, everything works as expected. Thanks, Raphael Kubo da Costa
Re: Review Request 119025: Actually pass IBUS_DEFINITIONS when compiling ibus-panel
On July 19, 2014, 12:17 a.m., Vadim Zhukov wrote: (As a general note, for build system related stuff like this you can also try including the buildsystem group, which can be more responsive at times) The ibus-panel can't build on OpenBSD because some required definitions obtained from pkgconfig file are not used. 1. IBUS_DEFINITIONS is not used in ibus-panel/CMakeLists.txt Can you post the error you get here? I've tried building kimtoy here on FreeBSD expecting to hit the same issue(s), but it all went along just fine. 2. IBUS_DEFINITIONS is not saved in cache, and thus isn't available at compile time This doesn't make much sense; all values found at configuration time in CMake are then used to generate your make/ninja/whatever files, regardless of whether they are cached or not. Vadim Zhukov wrote: Raphael, thank you for your input! The issue was caused by the fact that X includes are placed in the /usr/X11R6/include directory on OpenBSD. This catalog is mentioned in PC_IBUS_DEFINITIONS variable but isn't propagated to the caller of find_package(IBus). Yes, I was wrong: the CACHE part may and should be omitted. This patch was added by me a long time ago (7 Feb 2012 according to git log; guess the KDE version used then), when I was much less expirienced in CMake... I've started a massive push of OpenBSD local patches upstream recently during OpenBSD hackathon, when I got time for such cleanup. At the present time, the /usr/X11R6/include gets to the include_directories() from another place, so the patch isn't required at all. But, still, the PC_IBUS_DEFINITIONS should be respected, IMHO. What do you think? Please note that FreeBSD and OpenBSD and quiet different. So you can't test on one OS instead of another. The issue was caused by the fact that X includes are placed in the /usr/X11R6/include directory on OpenBSD. This catalog is mentioned in PC_IBUS_DEFINITIONS variable but isn't propagated to the caller of find_package(IBus). [...] At the present time, the /usr/X11R6/include gets to the include_directories() from another place, so the patch isn't required at all. But, still, the PC_IBUS_DEFINITIONS should be respected, IMHO. What do you think? While that is not wrong, the approach we normally take with CMake is that pkg-config's presence is optional, and we don't depend on its output to be able to build a module. In practice, this means one should look for IBus and X11 separately, as well as add their compiler flags/link against them independently. If kimtoy already does that, I just wouldn't make any change. Please note that FreeBSD and OpenBSD and quiet different. So you can't test on one OS instead of another. You don't need to preach to the choir :-) I'm well aware of the differences between them, my point is that in many cases packaging problems in one also impact the other (missing includes because people assume all software is in `/usr`, reliance on non-POSIX features without checking for their availability etc). - Raphael --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119025/#review62669 --- On July 19, 2014, 9:43 p.m., Vadim Zhukov wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119025/ --- (Updated July 19, 2014, 9:43 p.m.) Review request for kde-workspace, Plasma and Hui Ni. Repository: kimtoy Description --- The ibus-panel can't build on OpenBSD because some required definitions obtained from pkgconfig file are not used. This happens due to the following reasons: 1. IBUS_DEFINITIONS is not used in ibus-panel/CMakeLists.txt 2. IBUS_DEFINITIONS is not saved in cache, and thus isn't available at compile time This patch resolves both issues and makes ibus-panel compile on OpenBSD. (I found no suitable review group and therefore used plasma instead, as it was in plasma-addons previously; please, feel free to correct me if I'm wrong and sorry for any possible inconvenience) ((as there was no feedback for more than a week, I've added kde-workspace group to list of reviewers, too)) Diffs - ibus-panel/CMakeLists.txt 3a1ee49 Diff: https://git.reviewboard.kde.org/r/119025/diff/ Testing --- OpenBSD/i386-CURRENT, KDE 4.13 (it doesn't have kimtoy package, of course, but the code is same) Thanks, Vadim Zhukov
Review Request 119454: Make FindPyKDE4 work with PyQt's new build system.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119454/ --- Review request for kdelibs, Scott Kitterman, Luca Beltrame, and Andrea Scarpino. Bugs: 327633 http://bugs.kde.org/show_bug.cgi?id=327633 Repository: kdelibs Description --- Since PyQt 4.10, PyQt.pyqtconfig is deprecated and not available unless PyQt is built using the old configure script. PyKDE4 itself has recently been adjusted to mimic PyQt itself and only install its pykdeconfig module if pyqtconfig is present. Additionally, information such as SIP flags and the directory where PyKDE's SIP files are installed are now also provided in the PyKDE4.kdecore.PYKDE_CONFIGURATION dict. This commit completely rewrites FindPyKDE4.py to make it look like FindPyQt.cmake after commit a7e4743: most of the information used by FindPyKDE4.cmake is fetched from PyKDE4.kdecore, and we first try to obtain the SIP flags and directory from pykdeconfig and, if it fails, we use PYKDE_CONFIGURATION. Furthermore, FindPyKDE4.py now only prints the variables that are actually consumed by FindPyKDE4.cmake -- it is not possible to obtain all the data provided by pykdeconfig in PYKDE_CONFIGURATION. We've also stopped reading and setting PYKDE4_VERSION_TAG, since PyKDE4 stopped setting a KDE tag in 2008 (and its value was 3_92_0 at the time). Diffs - cmake/modules/FindPyKDE4.cmake 98f7c37477393731152d2d8fd51fc50cb9bbd9ef cmake/modules/FindPyKDE4.py e436c2a907efe5e8f764e14ed921d3a87d413f34 Diff: https://git.reviewboard.kde.org/r/119454/diff/ Testing --- Modules such as Kate's Pate plugin detects and builds fine without pyqtconfig and pykdeconfig on the system. Thanks, Raphael Kubo da Costa
Re: Review Request 119025: Actually pass IBUS_DEFINITIONS when compiling ibus-panel
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119025/#review62669 --- cmake/FindIBus.cmake https://git.reviewboard.kde.org/r/119025/#comment43439 Typo: paramaters (As a general note, for build system related stuff like this you can also try including the buildsystem group, which can be more responsive at times) The ibus-panel can't build on OpenBSD because some required definitions obtained from pkgconfig file are not used. 1. IBUS_DEFINITIONS is not used in ibus-panel/CMakeLists.txt Can you post the error you get here? I've tried building kimtoy here on FreeBSD expecting to hit the same issue(s), but it all went along just fine. 2. IBUS_DEFINITIONS is not saved in cache, and thus isn't available at compile time This doesn't make much sense; all values found at configuration time in CMake are then used to generate your make/ninja/whatever files, regardless of whether they are cached or not. - Raphael Kubo da Costa On July 12, 2014, 4:12 p.m., Vadim Zhukov wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119025/ --- (Updated July 12, 2014, 4:12 p.m.) Review request for kde-workspace, Plasma and Hui Ni. Repository: kimtoy Description --- The ibus-panel can't build on OpenBSD because some required definitions obtained from pkgconfig file are not used. This happens due to the following reasons: 1. IBUS_DEFINITIONS is not used in ibus-panel/CMakeLists.txt 2. IBUS_DEFINITIONS is not saved in cache, and thus isn't available at compile time This patch resolves both issues and makes ibus-panel compile on OpenBSD. (I found no suitable review group and therefore used plasma instead, as it was in plasma-addons previously; please, feel free to correct me if I'm wrong and sorry for any possible inconvenience) ((as there was no feedback for more than a week, I've added kde-workspace group to list of reviewers, too)) Diffs - cmake/FindIBus.cmake 8250c49 ibus-panel/CMakeLists.txt 3a1ee49 Diff: https://git.reviewboard.kde.org/r/119025/diff/ Testing --- OpenBSD/i386-CURRENT, KDE 4.13 (it doesn't have kimtoy package, of course, but the code is same) Thanks, Vadim Zhukov
Re: Review Request 119302: Make FindPyQt4 work with PyQt's new build system.
On July 16, 2014, 4:42 a.m., Scott Kitterman wrote: This is the method used in qscintilla2's configure.py (which upstream has generally endorsed): #! /usr/bin/python import sys import os if sys.platform == 'win32': data_dir = sys.prefix else: data_dir = sys.prefix + '/share' py_sip_dir = os.path.join(data_dir, 'sip') # Note: Set this by hand since the logic to figure out if we're using PyQt4 or # PyQt5 isn't relevant to the question (QScintilla does do this, but it's not # germane). pyqt = 'PyQt4' if pyqt is not None: pyqt_sip_dir = os.path.join(py_sip_dir, pyqt) else: pyqt_sip_dir = None print(pyqt_sip_dir) # prints /usr/share/sip/PyQt4 We should use something similar. Scott Kitterman wrote: So markdown and python code comments don't mix. The bolded things all have a leading '#'. Luca Beltrame wrote: Good idea. Can this be done? I did look at QScintilla's build system when writing my patch, but chose not to follow this path: doing this only works for the default values (`sys.prefix/sip` on Windows, `sys.prefix/share/sip` elsewhere), which in the worst case can be a different installation unrelated to the one used by the PyQt version we're using. I didn't see much value in just working out of the box in some specific cases. - Raphael --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119302/#review62457 --- On July 16, 2014, 12:19 a.m., Raphael Kubo da Costa wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119302/ --- (Updated July 16, 2014, 12:19 a.m.) Review request for Build System, KDE Bindings, kdelibs, Alex Merry, Luca Beltrame, and Simon Edwards. Bugs: 337462 http://bugs.kde.org/show_bug.cgi?id=337462 Repository: kdelibs Description --- Since PyQt 4.10, PyQt.pyqtconfig is deprecated and not available unless PyQt is built using the old configure script. There is no direct replacement for it, as PyQt's new build system does not provide as much information as before by design. Luckily, most of the variables we are interested in can be obtained from PyQt's QtCore module itself even if its old build system is used. The only exception is `pyqt_sip_dir`, which cannot be determined at all if pyqtconfig is not available. In this case, there is nothing we can do but ask the user to specify it manually via CMake with something like `-DPYQT4_SIP_DIR=/usr/share/sip`. To this effect, all variables set by FindPyQt4.cmake have been made cache variables, which means their values can be overriden by the user, thus ignoring the contents read via FindPyQt.py. Diffs - cmake/modules/FindPyQt.py 5d2f9514d87553d5a16a95943618572316c92861 cmake/modules/FindPyQt4.cmake b176b4f8cfee471a1b7aecdd2723d165b0496a85 Diff: https://git.reviewboard.kde.org/r/119302/diff/ Testing --- I was able to make Kate find PyQt by passing `-DPYQT4_SIP_DIR=...` with my PyQt installation without `pyqtconfig.py`, and calling `FindPyQt.py` by hand on a Debian system with `pyqtconfig.py` worked as before. Thanks, Raphael Kubo da Costa
Re: Review Request 119302: Make FindPyQt4 work with PyQt's new build system.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119302/ --- (Updated July 16, 2014, 4:21 p.m.) Review request for kdelibs, Alex Merry, Luca Beltrame, and Simon Edwards. Changes --- Use a default value for `pyqt_sip_dir`. Bugs: 337462 http://bugs.kde.org/show_bug.cgi?id=337462 Repository: kdelibs Description (updated) --- Since PyQt 4.10, PyQt.pyqtconfig is deprecated and not available unless PyQt is built using the old configure script. There is no direct replacement for it, as PyQt's new build system does not provide as much information as before by design. Luckily, most of the variables we are interested in can be obtained from PyQt's QtCore module itself even if its old build system is used. The only exception is pyqt_sip_dir, which cannot be determined at all if pyqtconfig is not available. In this case, the most we can do is guess the default path like QScintilla2 does, and fail if it does not exist. The user then needs to specify it manually via CMake with something like -DPYQT4_SIP_DIR=/usr/share/sip/PyQt4. To this effect, all variables set by FindPyQt4.cmake have been made cache variables, which means their values can be overriden by the user, thus ignoring the contents read via FindPyQt.py. Diffs (updated) - cmake/modules/FindPyQt.py 5d2f9514d87553d5a16a95943618572316c92861 cmake/modules/FindPyQt4.cmake b176b4f8cfee471a1b7aecdd2723d165b0496a85 Diff: https://git.reviewboard.kde.org/r/119302/diff/ Testing --- I was able to make Kate find PyQt by passing `-DPYQT4_SIP_DIR=...` with my PyQt installation without `pyqtconfig.py`, and calling `FindPyQt.py` by hand on a Debian system with `pyqtconfig.py` worked as before. Thanks, Raphael Kubo da Costa
Re: Review Request 119302: Make FindPyQt4 work with PyQt's new build system.
On July 16, 2014, 4:42 a.m., Scott Kitterman wrote: This is the method used in qscintilla2's configure.py (which upstream has generally endorsed): #! /usr/bin/python import sys import os if sys.platform == 'win32': data_dir = sys.prefix else: data_dir = sys.prefix + '/share' py_sip_dir = os.path.join(data_dir, 'sip') # Note: Set this by hand since the logic to figure out if we're using PyQt4 or # PyQt5 isn't relevant to the question (QScintilla does do this, but it's not # germane). pyqt = 'PyQt4' if pyqt is not None: pyqt_sip_dir = os.path.join(py_sip_dir, pyqt) else: pyqt_sip_dir = None print(pyqt_sip_dir) # prints /usr/share/sip/PyQt4 We should use something similar. Scott Kitterman wrote: So markdown and python code comments don't mix. The bolded things all have a leading '#'. Luca Beltrame wrote: Good idea. Can this be done? Raphael Kubo da Costa wrote: I did look at QScintilla's build system when writing my patch, but chose not to follow this path: doing this only works for the default values (`sys.prefix/sip` on Windows, `sys.prefix/share/sip` elsewhere), which in the worst case can be a different installation unrelated to the one used by the PyQt version we're using. I didn't see much value in just working out of the box in some specific cases. Scott Kitterman wrote: I think working out of the box in the standard, default case using the upstream recommended method is much better than requiring the value to be set by hand in all cases. This change set is about adjusting to the new upstream approach to things, so using the upstream recommended solution seems only logical. If this doesn't get included upstream, I'll add it as a distro patch for Debian/Kubuntu as I think it's definitely a superior approach. I think working out of the box in the standard, default case using the upstream recommended method is much better than requiring the value to be set by hand in all cases. This change set is about adjusting to the new upstream approach to things, so using the upstream recommended solution seems only logical. *shrug* Done in patch v2. If this doesn't get included upstream, I'll add it as a distro patch for Debian/Kubuntu ... ok? - Raphael --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119302/#review62457 --- On July 16, 2014, 4:21 p.m., Raphael Kubo da Costa wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119302/ --- (Updated July 16, 2014, 4:21 p.m.) Review request for kdelibs, Alex Merry, Luca Beltrame, and Simon Edwards. Bugs: 337462 http://bugs.kde.org/show_bug.cgi?id=337462 Repository: kdelibs Description --- Since PyQt 4.10, PyQt.pyqtconfig is deprecated and not available unless PyQt is built using the old configure script. There is no direct replacement for it, as PyQt's new build system does not provide as much information as before by design. Luckily, most of the variables we are interested in can be obtained from PyQt's QtCore module itself even if its old build system is used. The only exception is pyqt_sip_dir, which cannot be determined at all if pyqtconfig is not available. In this case, the most we can do is guess the default path like QScintilla2 does, and fail if it does not exist. The user then needs to specify it manually via CMake with something like -DPYQT4_SIP_DIR=/usr/share/sip/PyQt4. To this effect, all variables set by FindPyQt4.cmake have been made cache variables, which means their values can be overriden by the user, thus ignoring the contents read via FindPyQt.py. Diffs - cmake/modules/FindPyQt.py 5d2f9514d87553d5a16a95943618572316c92861 cmake/modules/FindPyQt4.cmake b176b4f8cfee471a1b7aecdd2723d165b0496a85 Diff: https://git.reviewboard.kde.org/r/119302/diff/ Testing --- I was able to make Kate find PyQt by passing `-DPYQT4_SIP_DIR=...` with my PyQt installation without `pyqtconfig.py`, and calling `FindPyQt.py` by hand on a Debian system with `pyqtconfig.py` worked as before. Thanks, Raphael Kubo da Costa
Re: Review Request 119302: Make FindPyQt4 work with PyQt's new build system.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119302/#review62509 --- I guess it's important to note that PyKDE is still broken with this change -- it also assumes `pyqtconfig.py` exists. Fixing it is probably going to be more difficult. - Raphael Kubo da Costa On July 16, 2014, 4:21 p.m., Raphael Kubo da Costa wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119302/ --- (Updated July 16, 2014, 4:21 p.m.) Review request for kdelibs, Alex Merry, Luca Beltrame, and Simon Edwards. Bugs: 337462 http://bugs.kde.org/show_bug.cgi?id=337462 Repository: kdelibs Description --- Since PyQt 4.10, PyQt.pyqtconfig is deprecated and not available unless PyQt is built using the old configure script. There is no direct replacement for it, as PyQt's new build system does not provide as much information as before by design. Luckily, most of the variables we are interested in can be obtained from PyQt's QtCore module itself even if its old build system is used. The only exception is pyqt_sip_dir, which cannot be determined at all if pyqtconfig is not available. In this case, the most we can do is guess the default path like QScintilla2 does, and fail if it does not exist. The user then needs to specify it manually via CMake with something like -DPYQT4_SIP_DIR=/usr/share/sip/PyQt4. To this effect, all variables set by FindPyQt4.cmake have been made cache variables, which means their values can be overriden by the user, thus ignoring the contents read via FindPyQt.py. Diffs - cmake/modules/FindPyQt.py 5d2f9514d87553d5a16a95943618572316c92861 cmake/modules/FindPyQt4.cmake b176b4f8cfee471a1b7aecdd2723d165b0496a85 Diff: https://git.reviewboard.kde.org/r/119302/diff/ Testing --- I was able to make Kate find PyQt by passing `-DPYQT4_SIP_DIR=...` with my PyQt installation without `pyqtconfig.py`, and calling `FindPyQt.py` by hand on a Debian system with `pyqtconfig.py` worked as before. Thanks, Raphael Kubo da Costa
Re: Review Request 119302: Make FindPyQt4 work with PyQt's new build system.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119302/ --- (Updated July 16, 2014, 7:49 p.m.) Status -- This change has been marked as submitted. Review request for kdelibs, Alex Merry, Luca Beltrame, and Simon Edwards. Bugs: 337462 http://bugs.kde.org/show_bug.cgi?id=337462 Repository: kdelibs Description --- Since PyQt 4.10, PyQt.pyqtconfig is deprecated and not available unless PyQt is built using the old configure script. There is no direct replacement for it, as PyQt's new build system does not provide as much information as before by design. Luckily, most of the variables we are interested in can be obtained from PyQt's QtCore module itself even if its old build system is used. The only exception is pyqt_sip_dir, which cannot be determined at all if pyqtconfig is not available. In this case, the most we can do is guess the default path like QScintilla2 does, and fail if it does not exist. The user then needs to specify it manually via CMake with something like -DPYQT4_SIP_DIR=/usr/share/sip/PyQt4. To this effect, all variables set by FindPyQt4.cmake have been made cache variables, which means their values can be overriden by the user, thus ignoring the contents read via FindPyQt.py. Diffs - cmake/modules/FindPyQt.py 5d2f9514d87553d5a16a95943618572316c92861 cmake/modules/FindPyQt4.cmake b176b4f8cfee471a1b7aecdd2723d165b0496a85 Diff: https://git.reviewboard.kde.org/r/119302/diff/ Testing --- I was able to make Kate find PyQt by passing `-DPYQT4_SIP_DIR=...` with my PyQt installation without `pyqtconfig.py`, and calling `FindPyQt.py` by hand on a Debian system with `pyqtconfig.py` worked as before. Thanks, Raphael Kubo da Costa
Review Request 119302: Make FindPyQt4 work with PyQt's new build system.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119302/ --- Review request for Build System, KDE Bindings, kdelibs, Alex Merry, Luca Beltrame, and Simon Edwards. Bugs: 337462 http://bugs.kde.org/show_bug.cgi?id=337462 Repository: kdelibs Description --- Since PyQt 4.10, PyQt.pyqtconfig is deprecated and not available unless PyQt is built using the old configure script. There is no direct replacement for it, as PyQt's new build system does not provide as much information as before by design. Luckily, most of the variables we are interested in can be obtained from PyQt's QtCore module itself even if its old build system is used. The only exception is `pyqt_sip_dir`, which cannot be determined at all if pyqtconfig is not available. In this case, there is nothing we can do but ask the user to specify it manually via CMake with something like `-DPYQT4_SIP_DIR=/usr/share/sip`. To this effect, all variables set by FindPyQt4.cmake have been made cache variables, which means their values can be overriden by the user, thus ignoring the contents read via FindPyQt.py. Diffs - cmake/modules/FindPyQt.py 5d2f9514d87553d5a16a95943618572316c92861 cmake/modules/FindPyQt4.cmake b176b4f8cfee471a1b7aecdd2723d165b0496a85 Diff: https://git.reviewboard.kde.org/r/119302/diff/ Testing --- I was able to make Kate find PyQt by passing `-DPYQT4_SIP_DIR=...` with my PyQt installation without `pyqtconfig.py`, and calling `FindPyQt.py` by hand on a Debian system with `pyqtconfig.py` worked as before. Thanks, Raphael Kubo da Costa
Re: FindKDE4Internal.cmake clang c++11 -fdelayed-template-parsing
Milian Wolff m...@milianw.de writes: Also, have you reported the errors that clang outputs to the developers? If it's correct analysis, it should be fixed - even if it's error-prone work. In this case, I don't think there's a bug: I haven't investigated in depth, but clang seems to be doing what -fdelayed-template-parsing is supposed to do, and it's libstdc++ + clang + c++11 that does not work well (the snippet works if I use libc++ instead). Right, but we cannot depend on libc++. If you can detect it, you could disable exceptions for clang when building with libc++ (if that's the case on the BSDs?). Hm? There are no problems with clang and libc++ with or without this change I committed, what I said is that it's libstdc++ and `clang -std=c++11 -fdelayed-template-parsing' that has been causing trouble, and most likely it is just clang doing the right thing (though I haven't investigated).
Re: FindKDE4Internal.cmake clang c++11 -fdelayed-template-parsing
Milian Wolff m...@milianw.de writes:
Hey Raphael!
Thank you for working on clang support in FindKDE4Internal.cmake.
Since recently though I have build issues with clang due to the -fdelayed-
template-parsing flag passed in FindKDE4Internal.cmake. A simple example such
as this:
#include vector
int main() { std::vectorint v; return 0; }
produces with: clang++ -std=c++11 -fdelayed-template-parsing test.cpp
[...]
Apparently, delayed template parsing is not yet mature. So - can we disable it
in FindKDE4Internal.cmake again? Or do you know a workaround for that?
Hi,
Sorry for not answering earlier.
As explained in the comment in FindKDE4Internal.cmake, simply not
passing -fdelayed-template-parsing causes a lot of problems due to us
building most repositories with -fno-exceptions and including template
code that does not get instantiated and throws exceptions: for example,
kdepimlibs' akonadi/itempayloadinternals_p.h throws some exceptions in
code that should not be reached, and while that code is built with
exceptions enabled, kdepim may end up indirectly including that header
without ever knowing it needs to enable exceptions in the first place
(which is what happened when I was testing clang support). The same
thing happened with kdelibs and OpenEXR too.
Contrary to GCC, MSVC and ICC, clang complains and fails to build if
that happens (and is probably doing the right thing).
One option is to fix all places where this might arise (it's manual and
error-prone work), or just remove -fno-exceptions from the default
compiler flags for clang in FindKDE4Internal.cmake. I tend ot favor the
latter, but it will increase the size of the generated binaries (thiago
estimated them to increase by ~5-7%).
Re: Review Request 110090: Clean up kickoff from stale bits
On May 8, 2013, 5:57 p.m., Raphael Kubo da Costa wrote: Thanks for the cleanup. You might also want to remove the references to Strigi in DESIGN-GOALS and STATUS-TODO, but I guess they're very outdated anyway. Please include a descriptive commit message explaining that this patch also fixes build problems for people without Strigi. Albert Astals Cid wrote: Janitorial dude question: Has this been commited and you forgot to mark it as submitted or it does still need submitting? For some reason it looks like Max didn't commit it. I'll do that now. - Raphael --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110090/#review32262 --- On April 20, 2013, 1:13 a.m., Max Brazhnikov wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110090/ --- (Updated April 20, 2013, 1:13 a.m.) Review request for kde-workspace. Description --- kickoff links to strigiqtdbusclient, but Strigi is not used since svn r1018482. Diffs - plasma/desktop/applets/kickoff/CMakeLists.txt e9e2888 plasma/desktop/applets/kickoff/core/config-kickoff-applets.h.cmake cecf380 Diff: http://git.reviewboard.kde.org/r/110090/diff/ Testing --- Thanks, Max Brazhnikov
Re: Review Request 110090: Clean up kickoff from stale bits
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110090/#review32262 --- Ship it! Thanks for the cleanup. You might also want to remove the references to Strigi in DESIGN-GOALS and STATUS-TODO, but I guess they're very outdated anyway. Please include a descriptive commit message explaining that this patch also fixes build problems for people without Strigi. - Raphael Kubo da Costa On April 20, 2013, 1:13 a.m., Max Brazhnikov wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110090/ --- (Updated April 20, 2013, 1:13 a.m.) Review request for kde-workspace. Description --- kickoff links to strigiqtdbusclient, but Strigi is not used since svn r1018482. Diffs - plasma/desktop/applets/kickoff/CMakeLists.txt e9e2888 plasma/desktop/applets/kickoff/core/config-kickoff-applets.h.cmake cecf380 Diff: http://git.reviewboard.kde.org/r/110090/diff/ Testing --- Thanks, Max Brazhnikov
Re: Review Request: Add pkgconfig hints to FindSamba.cmake
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106861/#review20611 --- cmake/modules/FindSamba.cmake http://git.reviewboard.kde.org/r/106861/#comment16254 Please note that support for the QUIET keyword was added in CMake 2.8.2 -- it is probably OK for the 4.10 branch once we start depending on CMake 2.8.8, but not for the other ones. - Raphael Kubo da Costa On Oct. 15, 2012, 12:24 p.m., Rex Dieter wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106861/ --- (Updated Oct. 15, 2012, 12:24 p.m.) Review request for kdelibs. Description --- Add pkgconfig hints to FindSamba.cmake, helps find samba4 libs on non-standard-ish locations. Diffs - cmake/modules/FindSamba.cmake 16522c6 Diff: http://git.reviewboard.kde.org/r/106861/diff/ Testing --- Tested on fedora 18 against samba-4.0-rc2 Thanks, Rex Dieter
Re: Review Request: Fix hang in kcm_useraccount
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105895/#review16989 --- How does this play with https://git.reviewboard.kde.org/r/104439 ? - Raphael Kubo da Costa On Aug. 6, 2012, 5:03 p.m., Michael Palimaka wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105895/ --- (Updated Aug. 6, 2012, 5:03 p.m.) Review request for KDE Base Apps. Description --- When changing the user's full name, chfn may not necessarily produce any output. Since readLine blocks, the kcm may hang. This change checks if chfn exited without output, and if so, use that exit status. This addresses bug 156396. http://bugs.kde.org/show_bug.cgi?id=156396 Diffs - kdepasswd/kcm/chfnprocess.cpp 9f75d4aa75b41acec84e7798c789d4226ca3fab9 Diff: http://git.reviewboard.kde.org/r/105895/diff/ Testing --- On a PAM-enabled system: * Full name changed successfully when permitted by login.defs * Error presented and no change processed with prohibited by login.defs Thanks, Michael Palimaka
Re: Review Request: Add CamelCase wrapper for kcodecaction.h
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105818/#review16789 --- Ship it! Ship It! - Raphael Kubo da Costa On Aug. 1, 2012, 7:27 p.m., Jekyll Wu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105818/ --- (Updated Aug. 1, 2012, 7:27 p.m.) Review request for kdelibs. Description --- There is no CamelCase wrapper for kcodecaction.h. I guess that is an omission. This simple patch adds it. Diffs - includes/CMakeLists.txt 157d321 includes/KCodecAction PRE-CREATION Diff: http://git.reviewboard.kde.org/r/105818/diff/ Testing --- Thanks, Jekyll Wu
Re: Review Request: properly pass the NOGUI flag
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105787/#review16629 --- Ship it! Mek seems to have added it in d37888c6c332ed431f1e08d3cc8b558cf6c98165 for some OS X use case, but it indeed seems to have been broken from the beginning (I thus don't even know if this is really still needed, and that code path in line 873 doesn't seem to have been updated by d502bccc3bcfdc5b1f05b4ee4a93a91722b2a922). As for who uses NOGUI, http://lxr.kde.org/search?filestring=string=kde4_add_unit_test+NOGUI - Raphael Kubo da Costa On July 29, 2012, 11:14 p.m., Aleix Pol Gonzalez wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105787/ --- (Updated July 29, 2012, 11:14 p.m.) Review request for Build System and kdelibs. Description --- While running some KDevelop tests, I've seen some output pointing to this. Looks like an error to me, so we'd better fix it :). Diffs - cmake/modules/KDE4Macros.cmake 2aa948c Diff: http://git.reviewboard.kde.org/r/105787/diff/ Testing --- kdelibs and all keep compiling ok, I'm unsure who uses NOGUI there. Thanks, Aleix Pol Gonzalez
Re: playground/games/picmi moved to KDE Review
Jakob Gruber jakob.gru...@gmail.com writes:
I've replaced all std::shared_pointers with QSharedPointers this morning
and removed the -std=c++11 flag.
I'm now getting a few errors and warnings when trying to build picmi
with g++ 4.2.1, 4.6.3 and clang 3.1: some of them are related to
-pedantic being used + Qt and kdelibs using long long in a few headers,
and some due to QSharedPointer being used with forward-declared
classes. I've attached clang's output since it's more informative than
gcc's.
[ 13%] Building CXX object
src/gui/CMakeFiles/picmi_gui.dir/picmi_gui_automoc.cpp.o
In file included from /tmp/picmi/build/src/gui/picmi_gui_automoc.cpp:4:
In file included from /tmp/picmi/build/src/gui/moc_mainwindow.cpp:10:
In file included from /tmp/picmi/build/src/gui/../../../src/gui/mainwindow.h:24:
In file included from /home/rakuco/kde4/qt4/include/QtCore/QTimer:1:
In file included from /home/rakuco/kde4/qt4/include/QtCore/qtimer.h:47:
In file included from /home/rakuco/kde4/qt4/include/QtCore/qbasictimer.h:45:
/home/rakuco/kde4/qt4/include/QtCore/qglobal.h:926:9: warning: 'long long' is
an extension when C99 mode is not enabled [-pedantic,-Wlong-long]
typedef long long qint64; /* 64 bit signed */
^
/home/rakuco/kde4/qt4/include/QtCore/qglobal.h:927:18: warning: 'long long' is
an extension when C99 mode is not enabled [-pedantic,-Wlong-long]
typedef unsigned long long quint64; /* 64 bit unsigned */
^
In file included from /tmp/picmi/build/src/gui/picmi_gui_automoc.cpp:4:
In file included from /tmp/picmi/build/src/gui/moc_mainwindow.cpp:10:
In file included from /tmp/picmi/build/src/gui/../../../src/gui/mainwindow.h:25:
In file included from
/tmp/opt-kde/lib/cmake/KDEGames/../../../include/highscore/kscoredialog.h:34:
In file included from /home/rakuco/kde4/stow/kdelibs/include/kdialog.h:32:
In file included from /home/rakuco/kde4/stow/kdelibs/include/kconfiggroup.h:718:
/home/rakuco/kde4/stow/kdelibs/include/conversion_check.h:97:15: warning: 'long
long' is an extension when C99 mode is not enabled [-pedantic,-Wlong-long]
QVConversions(long long, supported, supported);
^
/home/rakuco/kde4/stow/kdelibs/include/conversion_check.h:87:34: note: expanded
from macro 'QVConversions'
template struct QVconvertibletype {\
^
/home/rakuco/kde4/stow/kdelibs/include/conversion_check.h:98:24: warning: 'long
long' is an extension when C99 mode is not enabled [-pedantic,-Wlong-long]
QVConversions(unsigned long long, supported, supported);
^
/home/rakuco/kde4/stow/kdelibs/include/conversion_check.h:87:34: note: expanded
from macro 'QVConversions'
template struct QVconvertibletype {\
^
In file included from /tmp/picmi/build/src/gui/picmi_gui_automoc.cpp:4:
In file included from /tmp/picmi/build/src/gui/moc_mainwindow.cpp:10:
In file included from /tmp/picmi/build/src/gui/../../../src/gui/mainwindow.h:25:
In file included from
/tmp/opt-kde/lib/cmake/KDEGames/../../../include/highscore/kscoredialog.h:34:
In file included from /home/rakuco/kde4/stow/kdelibs/include/kdialog.h:32:
In file included from /home/rakuco/kde4/stow/kdelibs/include/kconfiggroup.h:718:
In file included from
/home/rakuco/kde4/stow/kdelibs/include/conversion_check.h:27:
In file included from /home/rakuco/kde4/qt4/include/QtGui/QFont:1:
In file included from /home/rakuco/kde4/qt4/include/QtGui/qfont.h:47:
In file included from /home/rakuco/kde4/qt4/include/QtCore/qsharedpointer.h:50:
/home/rakuco/kde4/qt4/include/QtCore/qsharedpointer_impl.h:342:21: warning:
deleting pointer to incomplete type 'IOHandler' may cause undefined behaviour
[-Wdelete-incomplete]
delete value;
^ ~
/home/rakuco/kde4/qt4/include/QtCore/qsharedpointer_impl.h:336:11: note: in
instantiation of member function
'QtSharedPointer::ExternalRefCountIOHandler::deref' requested here
{ deref(d, this-value); }
^
/home/rakuco/kde4/qt4/include/QtCore/qsharedpointer_impl.h:401:38: note: in
instantiation of member function
'QtSharedPointer::ExternalRefCountIOHandler::deref' requested here
inline ~ExternalRefCount() { deref(); }
^
/home/rakuco/kde4/qt4/include/QtCore/qsharedpointer_impl.h:466:7: note: in
instantiation of member function
'QtSharedPointer::ExternalRefCountIOHandler::~ExternalRefCount' requested here
class QSharedPointer: public QtSharedPointer::ExternalRefCountT
^
/home/rakuco/kde4/qt4/include/QtCore/qsharedpointer_impl.h:336:11: note: in
instantiation of member function
'QtSharedPointer::ExternalRefCountPicmi::deref' requested here
{ deref(d, this-value); }
^
/home/rakuco/kde4/qt4/include/QtCore/qsharedpointer_impl.h:401:38: note: in
instantiation of member function
'QtSharedPointer::ExternalRefCountPicmi::deref' requested here
inline ~ExternalRefCount() { deref(); }
Re: playground/games/picmi moved to KDE Review
Jakob Gruber jakob.gru...@gmail.com writes: Building with KDE trunk will require the patch from http://lists.kde.org/?l=kde-games-develm=134201653803914w=2. BTW, the config.h part of the patch should go in regardless of the rest, as config.h should be the first header included by the source files anyway. I've committed a few CMake fixes, and the remaining remark I have is about the -std=c++11 flag you pass to the compiler due to your usage of std::shared_ptr (I didn't see if there are other C++11 features you are making use of). Have you considered using Qt's pointer types instead? If you decide to stick to shared_ptr, please make sure you describe which compilers and versions are supposed to work correctly -- for example, the base compiler on FreeBSD (and I think on OS X too) is gcc 4.2.1, whose libstdc++ does not have std::shared_ptr.
Re: Compiler version
Martin Gräßlin mgraess...@kde.org writes: What about freebsd? Personally I am not willing to support that platform anymore if it would mean that we have to restrict ourself to an outdated gcc version. Reasons why in general I would find it acceptable to drop support for non-linux in KWin are outlined in a recent discussion on plasma mailing list [1]. @Raphael any idea about that? In the future, the idea is to use clang instead of gcc by default, but I wouldn't count on that for this decision: there's still work to do, and even when a new FreeBSD comes out with clang as the default compiler we'll still have to support a few older releases with gcc 4.2 anyway. That said, it wouldn't really be a problem if a newer gcc was required; we do have many different gcc release series in the ports system (including 4.7.x and 4.8 snapshots) and in the end users will simply have to use them to build KDE instead. As for the Plasma post, there's ongoing work towards having recent X.org releases, as well as improving support for KMS and things like that (AFAICT the NVidia binary blog probably already works fine, and I am using KMS with an Intel graphics card myself with Mesa 7.11.2, with updates on the way). The KMS code might not apply to all supported FreeBSD versions, but this is something that will be automatically solved with time -- other than that, I don't really know what difference it all would make for KWin and workspace in general (I'm not subscribed to plasma-devel, but I can do that or you guys can CC me if you prefer to continue this part of the discussion elsewhere).
Re: Review Request: Added fallback for real username in kcm module to use KUser::FullName
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104965/#review15206 --- Ship it! Makes sense, please commit to master. - Raphael Kubo da Costa On June 27, 2012, 4:29 p.m., Aleksey Yermakov wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104965/ --- (Updated June 27, 2012, 4:29 p.m.) Review request for KDE Base Apps. Description --- Added fallback for real username in kcm module. Really helps in issues like this: http://bugs.rosalinux.ru/show_bug.cgi?id=179 Besides, such method seems natural, considering that change of real username in kcm module calls chfn to change real name in finger db, which will be reflected back in KUser::FullName. Diffs - kdepasswd/kcm/main.cpp 2471444 Diff: http://git.reviewboard.kde.org/r/104965/diff/ Testing --- Tested by QA crew of ROSA Desktop for ROSA Desktop 2012 LTS. Thanks, Aleksey Yermakov
Re: Review Request: Added fallback for real username in kcm module to use KUser::FullName
On June 27, 2012, 4:50 p.m., Raphael Kubo da Costa wrote: Makes sense, please commit to master. Aleksey Yermakov wrote: I'm afraid, I don't have write rights. Could you please point me to some documentation which will help me commit this patch? I will commit this one for you then. The basic documentation is this one: http://techbase.kde.org/Contribute/Get_a_Contributor_Account. In short, if you send a few more patches and show that you're interested in keeping contributing, people will encourage you to apply for an account and second the nomination (it may sound pompous but it's really simple :-) - Raphael --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104965/#review15206 --- On June 27, 2012, 4:29 p.m., Aleksey Yermakov wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104965/ --- (Updated June 27, 2012, 4:29 p.m.) Review request for KDE Base Apps. Description --- Added fallback for real username in kcm module. Really helps in issues like this: http://bugs.rosalinux.ru/show_bug.cgi?id=179 Besides, such method seems natural, considering that change of real username in kcm module calls chfn to change real name in finger db, which will be reflected back in KUser::FullName. Diffs - kdepasswd/kcm/main.cpp 2471444 Diff: http://git.reviewboard.kde.org/r/104965/diff/ Testing --- Tested by QA crew of ROSA Desktop for ROSA Desktop 2012 LTS. Thanks, Aleksey Yermakov
Re: Compiler version
Ivan Čukić ivan.cu...@kde.org writes: Now, my proposal here is to update the required versions for Frameworks 4 to reflect those of KDE Frameworks 5 / Qt 5. Now, I've found different information for this - skelly says [2] the requirement is GCC 4.6 while some other places state it is GCC 4.5, so I'm not sure whether it was a typing error or not. The most up-to-date discussion I can find about defining a lowest common denominator is [1]. In short, if I understand it correctly we (as well as Qt) are still supposed to support Apple with gcc 4.2.1. This also happens to help the situation on FreeBSD, where the default compiler is also gcc 4.2.1 + patches (the latest GPLv2 release), even though it is possible to require a more recent version from its ports system. Apple (as well as FreeBSD) seems to be moving towards clang, however I do not know if they have made it their default compiler in some release of theirs. - FreeBSD 10 - Clang 3.1 (*very* modern) FreeBSD 10 is the development version, and even there clang is not the default compiler yet. For the foreseeable future we're still having gcc 4.2.1 used by default, but, as I said, we could require another version even if a few users complain (I'd prefer if the requirements for KDE 4 were not changed, though). [1] http://article.gmane.org/gmane.comp.kde.devel.buildsystem/7133
Re: Review Request: Added fallback for real username in kcm module to use KUser::FullName
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104965/#review15187
---
kdepasswd/kcm/main.cpp
http://git.reviewboard.kde.org/r/104965/#comment11891
Nit: 'realName' is better, as these are separate words.
kdepasswd/kcm/main.cpp
http://git.reviewboard.kde.org/r/104965/#comment11892
Please follow kdelibs' coding style here (4-space indentation, surrounding
braces even for single-line statements):
if (foo) {
bar();
}
- Raphael Kubo da Costa
On May 16, 2012, 7:09 a.m., Aleksey Yermakov wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104965/
---
(Updated May 16, 2012, 7:09 a.m.)
Review request for KDE Base Apps.
Description
---
Added fallback for real username in kcm module.
Really helps in issues like this: http://bugs.rosalinux.ru/show_bug.cgi?id=179
Besides, such method seems natural, considering that change of real username
in kcm module calls chfn to change real name in finger db, which will be
reflected back in KUser::FullName.
Diffs
-
kdepasswd/kcm/main.cpp 2471444
Diff: http://git.reviewboard.kde.org/r/104965/diff/
Testing
---
Tested by QA crew of ROSA Desktop for ROSA Desktop 2012 LTS.
Thanks,
Aleksey Yermakov
Re: Moving libsolid-hal to unmaintained?
Alex Fiestas afies...@kde.org writes: On Monday, April 23, 2012 11:18:14 AM todd rme wrote: Is there a way to have it so HAL is enabled by default on BSD systems, but on Linux systems you need to manually use a cmake flag to enable it? Well I don't see why we need HAL on Linux, so unless you can give me some reason too keep it I will make it available only on BSD/Solaris but not on Linux. Why not build it depending on the presence of HAL in the system and avoid hardcoding these options depending on the OS?
Re: Pairs going to KDE Edu
Aleix Pol aleix...@kde.org writes: Hi, Last friday Pairs [1] was moved from playground/edu to kdereview because we want it to be moved to kdeedu. We have been working on it for a while already and we would like it to move to kde edu and to be included in the next KDE release. If someone is interested, please take a look into it and tell us what you think. Could you guys add a license file to the root directory? It'd make distro people's lives easier. I also see that kdeclarative is being hardcoded in the target_link_libraries() call in src/CMakeLists.txt, probably due to [1] still being reviewed. This may break the build in the unlikely case that libkdeclarative is not installed in the same directory as the other KDE libraries or the headers are in a different place (dunno if this has any impact on Windows or cross-compilation stuff). [1] https://git.reviewboard.kde.org/r/104140/
Re: Review Request: Port shutdown dialog to QML
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103621/#review10232 --- Isn't it better to move FindKDeclarative.cmake to the top-level cmake/modules directory with the other find-files? ksmserver/FindKDeclarative.cmake http://git.reviewboard.kde.org/r/103621/#comment8421 As picky as it may seem, doesn't this file need a license? ksmserver/FindKDeclarative.cmake http://git.reviewboard.kde.org/r/103621/#comment8422 Why is this needed? ksmserver/FindKDeclarative.cmake http://git.reviewboard.kde.org/r/103621/#comment8424 Why not pass KDECLARATIVE_LIBRARIES directly to find_library() to get rid of this block? ksmserver/FindKDeclarative.cmake http://git.reviewboard.kde.org/r/103621/#comment8423 I don't see these being used anywhere in the patch, does it make sense to keep deprecated declarations in a new file? ksmserver/FindKDeclarative.cmake http://git.reviewboard.kde.org/r/103621/#comment8425 Missing KDECLARATIVE_LIBRARIES? - Raphael Kubo da Costa On Jan. 30, 2012, 2:28 p.m., Lamarque Vieira Souza wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103621/ --- (Updated Jan. 30, 2012, 2:28 p.m.) Review request for KDE Base Apps and KDE Runtime. Description --- Port shutdown dialog to QML. Two QML themes are included: default, which mimics the current shutdown dialog look feel, and contour, which is used in Plasma Active. This addresses bugs 216853 and 216853. http://bugs.kde.org/show_bug.cgi?id=216853 http://bugs.kde.org/show_bug.cgi?id=216853 Diffs - ksmserver/CMakeLists.txt 295b96e ksmserver/FindKDeclarative.cmake PRE-CREATION ksmserver/Messages.sh 0aa8bab ksmserver/shutdown.cpp 7fd1e11 ksmserver/shutdowndlg.h e5f0942 ksmserver/shutdowndlg.cpp a09a1a7 ksmserver/themes/contour/ContourButton.qml PRE-CREATION ksmserver/themes/contour/main.qml PRE-CREATION ksmserver/themes/contour/metadata.desktop PRE-CREATION ksmserver/themes/contour/screenshot.png PRE-CREATION ksmserver/themes/default/ContextMenu.qml PRE-CREATION ksmserver/themes/default/KSMButton.qml PRE-CREATION ksmserver/themes/default/MenuItem.qml PRE-CREATION ksmserver/themes/default/helper.js PRE-CREATION ksmserver/themes/default/main.qml PRE-CREATION ksmserver/themes/default/metadata.desktop PRE-CREATION ksmserver/themes/default/screenshot.png PRE-CREATION Diff: http://git.reviewboard.kde.org/r/103621/diff/diff Testing --- Works in Plasma Active Two using MeeGo image and KDE SC 4.8. It does not work in 4.7.x because the default theme requires kde-runtime 4.8's declarative imports. TODO: . check if there are bugs in bugs.kde.org that could be solved by this patch. . test right to left language support. Screenshots --- http://git.reviewboard.kde.org/r/103621/s/400/ New version with label accelerator working http://git.reviewboard.kde.org/r/103621/s/407/ Thanks, Lamarque Vieira Souza
Re: Review Request: Port shutdown dialog to QML
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103621/#review10238 --- ksmserver/FindKDeclarative.cmake http://git.reviewboard.kde.org/r/103621/#comment8433 Where is Copyright.txt? :) If you move this file to cmake/modules you can share the license header with the other files there and just point to COPYING-CMAKE-SCRIPTS (if you do so, it's probably better not to install it, btw). ksmserver/FindKDeclarative.cmake http://git.reviewboard.kde.org/r/103621/#comment8434 Can you elaborate on why KDECLARATIVE_LIBRARY is being used/set? - Raphael Kubo da Costa On Jan. 30, 2012, 4:35 p.m., Lamarque Vieira Souza wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103621/ --- (Updated Jan. 30, 2012, 4:35 p.m.) Review request for KDE Base Apps and KDE Runtime. Description --- Port shutdown dialog to QML. Two QML themes are included: default, which mimics the current shutdown dialog look feel, and contour, which is used in Plasma Active. This addresses bugs 216853 and 216853. http://bugs.kde.org/show_bug.cgi?id=216853 http://bugs.kde.org/show_bug.cgi?id=216853 Diffs - ksmserver/themes/default/main.qml PRE-CREATION ksmserver/themes/contour/main.qml PRE-CREATION ksmserver/themes/contour/metadata.desktop PRE-CREATION ksmserver/themes/contour/screenshot.png PRE-CREATION ksmserver/themes/default/ContextMenu.qml PRE-CREATION ksmserver/themes/default/KSMButton.qml PRE-CREATION ksmserver/themes/default/MenuItem.qml PRE-CREATION ksmserver/themes/default/helper.js PRE-CREATION ksmserver/themes/contour/ContourButton.qml PRE-CREATION ksmserver/shutdowndlg.cpp a09a1a7 ksmserver/CMakeLists.txt 295b96e ksmserver/FindKDeclarative.cmake PRE-CREATION ksmserver/Messages.sh 0aa8bab ksmserver/shutdown.cpp 7fd1e11 ksmserver/shutdowndlg.h e5f0942 ksmserver/themes/default/metadata.desktop PRE-CREATION ksmserver/themes/default/screenshot.png PRE-CREATION Diff: http://git.reviewboard.kde.org/r/103621/diff/diff Testing --- Works in Plasma Active Two using MeeGo image and KDE SC 4.8. It does not work in 4.7.x because the default theme requires kde-runtime 4.8's declarative imports. TODO: . test right to left language support. Screenshots --- http://git.reviewboard.kde.org/r/103621/s/400/ New version with label accelerator working http://git.reviewboard.kde.org/r/103621/s/407/ Thanks, Lamarque Vieira Souza
Re: Review Request: Report file errors when extracting files using karchive
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/103808/#review10182
---
Not sure if this is 4.x material or frameworks-only (I occasionally see
karchive undergoing some changes there). Only returning errors from that
QFile::write call seems weird IMHO (other parts of the code may fail too).
kdecore/io/karchive.h
http://git.reviewboard.kde.org/r/103808/#comment8364
The documentation is still missing the @since tag.
kdecore/io/karchive.cpp
http://git.reviewboard.kde.org/r/103808/#comment8367
If you're just breaking out of the loop in the next iteration, why not do
something like this:
while (remainingSize 0) {
// yadda, yadda
if (f.write(...) == -1) {
d-fileError = f.error();
break;
}
}
- Raphael Kubo da Costa
On Jan. 28, 2012, 4:46 p.m., Theofilos Intzoglou wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/103808/
---
(Updated Jan. 28, 2012, 4:46 p.m.)
Review request for kdelibs.
Description
---
A simple patch to check if something goes wrong when extracting files from an
archive. You can read the error code using copyToErrorCode()
Diffs
-
kdecore/io/karchive.h 7cd7c0c
kdecore/io/karchive.cpp 86d61d5
Diff: http://git.reviewboard.kde.org/r/103808/diff/diff
Testing
---
Thanks,
Theofilos Intzoglou
Re: a confusion beetwen cmake/ccmake and FindKDE4Internal.cmake
Guy Maurel guy-...@maurel.de writes: The tool ccmake is showing something erroness: The values of CMAKE_C_FLAGS and CMAKE_CXX_FLAGS are shown with -g One may change these values to another value. BUT the file FindKDE4Internal.cmake is stronger. The generated files flags.make contains the values from the file FindKDE4Internal.cmake It might be a good way, but a beginner (such as I) don't see anything, don't anderstand what is working. Could a comment (in the CMakeCache.txt/ at the results list) be generated to explain this? The kde-buildsystem mailing list is probably a better place to discuss this kind of issue. Cheers, rakuco
kdeutils has migrated to git
Hello there, kdeutils has (mostly) finished its migration to git. SVN now only holds MOVED_TO_GIT files which point to the git repositories. Each application (ark, filelight, kcalc, kcharselect, kfloppy, kgpg, kremotecontrol, ktimer, kwallet, printer-applet, superkaramba, sweeper) has its own git repository. It should also be possible to build everything as a single module too. For now, I have created a scratch git repository with the files which used to be in the top-level kdeutils directory in SVN, so you can just use the CMakeLists.txt there to build everything (there are no inter-repository dependencies, that is, there is no libkdeutils). Cheers, rakuco
Re: buildsystem BoF at Desktop Summit
Alexander Neundorf neund...@kde.org writes: On Tuesday 02 August 2011, Alexander Neundorf wrote: On Friday 29 July 2011, Alexander Neundorf wrote: Hi, ... I have set up a doodle poll here: http://www.doodle.com/v53bgft9xkffdnft Please enter when you can attend (the earlier the better), and we'll decide this way. So, if you want to attend, please enter in doodle when it would fit for you: http://www.doodle.com/v53bgft9xkffdnft Ok, the poll is done. We'll meet Monday at 8:00 PM, location is still to be defined. I'll send an email on Sunday and will also blog where we'll meet. Looking forward to see you in Berlin :-) I'm sending this on behalf of Alex, who's sitting next to me and can't send mail ;) Hi, the KDE buildsystem BoF/meeting/whatever you want to call it/ will be Monday, at 8:00 PM (i.e. in the evening) in the restaurant/cafe St.Oberholz. http://www.sanktoberholz.de/?page_id=13 According to Claudia they are used to geeks sitting around and discussing :-) The address is Sankt Oberholz Rosenthaler Straße 72a 10119 Berlin Places are reserved for us on my name (neundorf). St. Oberholz should be 15 to 30 minutes walking from here. I think I'll leave here from the university building at 6:30PM. Then there should be enough time to have dinner before 8:00PM. So either you'll just be there at 8:00PM, or we walk there together. Then let's meet here at the registration at .30PM. P.S.: Kevin, please tell Guillaume. Alex
Re: Review Request: Only include nepomuk directories if nepomuk is available
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101949/#review4751 --- Ship it! Looks OK to me. - Raphael On July 14, 2011, 2:40 p.m., Bjoern Ricks wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101949/ --- (Updated July 14, 2011, 2:40 p.m.) Review request for kdelibs. Summary --- kdelibs build fails if sdo and/or soprano aren't available Diffs - kparts/CMakeLists.txt db76613 Diff: http://git.reviewboard.kde.org/r/101949/diff Testing --- Thanks, Bjoern
Re: Review Request: add default help menu with all standard help actions to Help button in KFind
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101443/#review3869 --- Ship it! Looks OK. Perhaps KFind should actually have a main window with a normal menu bar instead of being a dialog, but that's unrelated to this patch. Please remember to update the 4.6.5 changelog. kfind/kfinddlg.cpp http://git.reviewboard.kde.org/r/101443/#comment3166 Extra whitespace. - Raphael On May 26, 2011, 12:07 p.m., Burkhard Lück wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101443/ --- (Updated May 26, 2011, 12:07 p.m.) Review request for KDE Base Apps. Summary --- The Help button in KFind opens the Handbook, but all usual standard help actions like bug report, switch language and about kde/kfind are missing. Wth this patch the Help in kfind has the default behavior of all other kde applications. ksnaphot has the same button with all standard help actions. This addresses bug 132630. http://bugs.kde.org/show_bug.cgi?id=132630 Diffs - kfind/kfinddlg.cpp 8645c0d Diff: http://git.reviewboard.kde.org/r/101443/diff Testing --- Thanks, Burkhard
Re: Review Request: kcm-grub2
Konstantinos Smanis konstantinos.sma...@gmail.com writes: I have talked with Alberto Mattea (the maintainer of kcmgrub2) and he has agreed to give up his request for inclusion in extragear. Our conversation has been over plain e-mails, so I cannot prove this. Perhaps I should contact him to confirm this? That sounds like getting rid of dandruff by decapitation ;) I thought one of you would just rename your project, as it looks like there are still going to be two projects with almost the exact same name, just not in the same location in KDE.
Re: Review Request: kcm-grub2
Konstantinos Smanis konstantinos.sma...@gmail.com writes: On Thu, May 26, 2011 at 00:03, Konstantinos Smanis konstantinos.sma...@gmail.com wrote: The following line should probably be simplified (see http://websvn.kde.org/?revision=1184860view=revision): src/kcm_grub2.cpp:113: QTreeWidgetItem *item = new QTreeWidgetItem(ui.treeWidget_recover, QStringList(QString()) name partition-filePath() volume-label() volume-fsType() i18n(%1 GiB, QString::number(volume-size() / 1073741824))); Thanks. Fixed: http://commits.kde.org/kcm-grub2/87eac2b1ef0b8f3a1907b86ab7054b99a0ed5ab9 Konstantinos If noone else objects, would it be possible for a sysadmin to make the move? This is not really an objection, as I have not looked at the code. But having two different projects with practically identical names (kcm_grub2 and kcmgrub2) is quite confusing. It'd be nice if this could be worked out, as both projects have actually asked for review and to be moved to the same final destination (extragear/sysadmin).
Re: Review Request: Update KActionCollection member documentation
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101494/#review3646 --- Ship it! Looks OK, the new apidox even looks similar to its counterpart in kstandardaction.h. - Raphael On June 2, 2011, 8:09 p.m., Rolf Eike Beer wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101494/ --- (Updated June 2, 2011, 8:09 p.m.) Review request for kdelibs. Summary --- KActionCollection is missing parameter documention on some members. And one has even a bogus one. Diffs - kdeui/actions/kactioncollection.h 076088b Diff: http://git.reviewboard.kde.org/r/101494/diff Testing --- Thanks, Rolf Eike
Re: KDE/kdeadmin/system-config-printer-kde
Ozan Çağlayan o...@pardus.org.tr writes: 23-02-2011 17:09 tarihinde, Jonathan Riddell yazdı: SVN commit 1222403 by jriddell: Add samba browsing https://bugs.launchpad.net/ubuntu/+source/kdeadmin/+bug/295065 BUG:259283 M +1 -0 CMakeLists.txt A pysmb.py M +363 -40 system-config-printer-kde.py http://websvn.kde.org/?view=revrevision=1222403 Hi, I think this commit garbled the s-c-p-kde code a lot. First of all, You now imported an already available pysmb.py from upstream system-config-printer. Why not to use the one installed on the system as we already depend on debug.py and smburi.py from the very same upstream s-c-p in kdeadmin? The imported pysmb.py brought a python-GTK and thus GTK dependency to KDE. I saw that you imported it in try/except and set PYSMB_AVAILABLE to true/false but you never used it actually. pysmb.smbc is actually a Python binding for smbc. I think you should import smbc directly and depend explicitly on python-smbc project instead of using it through pysmb wrapper. Thank you! Has there been any followup to this?
Re: svn - git transition status ?
Alexander Neundorf neund...@kde.org writes: what's the current status with our svn to git transition ? There are still several modules in svn (kdeaccessibility, kdeadmin, kdeartwork, kdebindings, kdegames, kdegraphics, kdemultimedia, kdenetwork, kdesdk, kdetoys, kdeutils, kdewebdev). kdeutils: The conversion rules are pretty much finished, PovAddict and I just need some free time to double-check everything (we're more cautious than ever after the kdeedu problems). kdeaccessibility: IIRC, kdeaccessibility is in a similar state, but jpwhiting has more details. kdebindings: I think it has migrated, but the 4.6 version is still in svn (I might be wrong here). kdegraphics: It has mostly migrated to git; Okular is the only one left in svn and should migrate soon. The rest of the modules are at an unknown/early/inexistent migration state. Is it planned to move them also to git ? Yes. Is somebody working on this ? Is there a time plan ? Usually, the speed of the migration is proportional to the number of people in the module working on the migration. As people from some modules have not worked on it yet, migrating these modules depends on people who are involved in most migrations (PovAddict, for example) taking some time to write the rules themselves.
Re: Kcmgrub2
Alberto Mattea albe...@mattea.info writes: Hi, kcmgrub2 has been under review for two weeks now. I've done all the suggested changes. Would it be possible to move it to kdemain/sysadmin or (if that's not possible) to extragear/sysadmin? By the way, is there any relationship between kcmgrub2 and kcm-grub2?
Review Request: Fix directory navigation in Dolphin::Terminal.
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101096/ --- Review request for KDE Base Apps. Summary --- Fix directory navigation in Dolphin::Terminal. When navigating in Dolphin it attempts to keep any open Terminal (F4) in sync by changing the directory in the shell. It does this by sending a ^C; cd $DIRECTORY however shells under FreeBSD treat ^C as a literal string and not SIGINT. Fix this by sending SIGINT to the shell instead of ^C. It appears Linux does not exhibit this behaviour. Patch originally written by David Naylor, from the KDE-FreeBSD team. Diffs - dolphin/src/panels/terminal/terminalpanel.cpp 61d80cbfa26fb17d0ba403a52f2ab57fbae7b3cc Diff: http://git.reviewboard.kde.org/r/101096/diff Testing --- Changing directories with a terminal open now changes the terminal's current directory fine on FreeBSD. Thanks, Raphael
Re: Review request: Kcmgrub2
Alberto Mattea albe...@mattea.info writes: In data mercoledì 6 aprile 2011 00:28:21, Raphael Kubo da Costa ha scritto: Buildsystem-wise: * I did not understand why you used include() instead of find_package() in, for example, include(FindPyQt4) Actually I used an article on the KDE wiki as an example, I didn't know it wasn't the standard way of doing it. Now it's fixed. There are some add_subdirectory() calls before the find_package() ones, so you might end up trying to build those directories before checking all the dependencies. You might also want to consider using KDE's macro_optional_find_package() together with macro_log_feature(), so you show a list of all the dependencies which have or have not been found instead of failing at the first one. I remember some discussions before about build-time and runtime dependencies in applications written in Python, but I can't remember the outcome. Someone with a distro hat should have more information about this.
Re: Review request: Kcmgrub2
Alberto Mattea albe...@mattea.info writes:
Hi all,
after 4 releases I think kcmgrub2 has reached an acceptable level of maturity,
so I'd ask for a move to kdereview. It is currently in playground-sysadmin
(git).
I only took a quick look, as my Py{Qt,KDE}-fu is not that good.
Buildsystem-wise:
* I did not understand why you used include() instead of
find_package() in, for example,
include(FindPyQt4)
* It's probably a good idea to add some kind of README for packagers
explaining what the dependencies are.
Licensing-wise:
* Don't you need to add the appropriate license header to your code
files?
As for kcmgrub2.py itself:
* It might be better to set the WhatsThis values in the .ui file
itself.
* `x' is not a good name for a global variable.
Re: git workflow draft
On Wednesday 16 February 2011 10:58:48 John Layt wrote: # ===[ Subject ]===| # ---[ One line only, short meaningful description to show in logs ]---| Personally, I find that starting this short description with the module, library or whatever being touched helps when one is skimming through the log. kzip: Fix typo or kdecore: Fix typo looks better than Fix typo. Is it worth mentioning in the template?
Re: Review Request: Workaround for the hang (freeze) when opening VLC's file dialog under KDE...
Pino Toscano p...@kde.org writes:
kdecore/services/kmimetyperepository.cpp
http://git.reviewboard.kde.org/r/100539/#comment1002
I guess you should also add the /usr/local equivalent, and enclose both
in a
#ifdef Q_OS_UNIX
...
#endif
block
By the way, how welcome would some more default paths be? pkg-config .pc
files are installed in /usr/local/lib/pkgconfig in {Net,Open}BSD, and in
/usr/local/libdata/pkgconfig in FreeBSD. And then there are the
OpenSolaris guys.
Hardcoding a lot paths definitely looks ugly, I'm just not sure whether
it's worse than asking packagers for these OSes to add patches locally
or recommend their users to set PKG_CONFIG_PATH accordingly.
Re: KDE library compile error (on the ARM CORTEX-A8)
On Wed, Jan 19, 2011 at 6:57 PM, Thiago Macieira thi...@kde.org wrote: On Wednesday, 19 de January de 2011 15:32:41 Thiago Macieira wrote: On Wednesday, 19 de January de 2011 09:21:29 Александр Куземко wrote: localhost temp # scanelf -qT /usr/lib/libkhtml.so.5.6.0 scanelf: scanelf_file_textrels(): ELF /usr/lib/libkhtml.so.5.6.0 has TEXTREL markings but doesnt appear to have any real TEXTREL's !? /usr/lib/libkhtml.so.5.6.0 Yep, it's the same as the MeeGo bug I posted. See there if there was a fix. It's a toolchain issue. The URL was wrong: http://bugs.meego.com/show_bug.cgi?idT20 It still looks wrong.
Re: Review Request: Add support for multiple selection in the KEditListBox and KEditListWidget widgets
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/6204/#review9419 --- /trunk/KDE/kdelibs/kdeui/widgets/keditlistbox.h http://svn.reviewboard.kde.org/r/6204/#comment10430 I don't think you're allowed to change this slot's signature, as this is a binary-incommpatible change. Add a new slot instead. /trunk/KDE/kdelibs/kdeui/widgets/keditlistbox.cpp http://svn.reviewboard.kde.org/r/6204/#comment10431 According to kdelibs' coding style, you should #include both module and class name for Qt includes. /trunk/KDE/kdelibs/kdeui/widgets/keditlistbox.cpp http://svn.reviewboard.kde.org/r/6204/#comment10432 Unnecessary change. /trunk/KDE/kdelibs/kdeui/widgets/keditlistwidget.h http://svn.reviewboard.kde.org/r/6204/#comment10433 Missing @since 4.7, and the method can be const. /trunk/KDE/kdelibs/kdeui/widgets/keditlistwidget.h http://svn.reviewboard.kde.org/r/6204/#comment10434 Ditto. /trunk/KDE/kdelibs/kdeui/widgets/keditlistwidget.h http://svn.reviewboard.kde.org/r/6204/#comment10435 Same thing about keeping binary compatibility. - Raphael On 2010-12-25 15:13:50, George Metaxas wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/6204/ --- (Updated 2010-12-25 15:13:50) Review request for kdelibs. Summary --- In order to satisfy Bug 219574, the KEditListBox and KEditListWidget widgets need to have multiple item selection support. This patch adds the familiar type of multiple item selection (select items by pressing CTRL + Left Click, and select consecutive regions with SHIFT + Left click) in both of these widgets. The patch also allows the removal of the multiply selected items from the embedded listview, as well as moving them up or down (while maintaining the same selection pattern). This patch will be required in order to completely solve Bug 219574. This addresses bug 219574. https://bugs.kde.org/show_bug.cgi?id=219574 Diffs - /trunk/KDE/kdelibs/kdeui/widgets/keditlistbox.h 1209179 /trunk/KDE/kdelibs/kdeui/widgets/keditlistbox.cpp 1209179 /trunk/KDE/kdelibs/kdeui/widgets/keditlistwidget.h 1209179 /trunk/KDE/kdelibs/kdeui/widgets/keditlistwidget.cpp 1209179 Diff: http://svn.reviewboard.kde.org/r/6204/diff Testing --- Tested the multiple item selection, moving and removing multiply selected items. Thanks, George
Re: Review Request: Adding net usershare suport for KSambaShare
---
This is an automatically generated e-mail. To reply, visit:
http://svn.reviewboard.kde.org/r/4320/#review9079
---
dfaure is still the best person to comment on the KDirWatch bits. As for the
rest, it looks mostly fine, with these small pieces needing some work.
/trunk/KDE/kdelibs/kio/kio/ksambashare.h
http://svn.reviewboard.kde.org/r/4320/#comment9874
Several @return statements here are followed by a @c, even when the
following word does not need to use a typewriter font.
/trunk/KDE/kdelibs/kio/kio/ksambashare.h
http://svn.reviewboard.kde.org/r/4320/#comment9875
Test - Tests, for consistency. Missing '.' at the end of the sentence.
/trunk/KDE/kdelibs/kio/kio/ksambashare.h
http://svn.reviewboard.kde.org/r/4320/#comment9876
If you want to keep the @c's here, false needs one too.
/trunk/KDE/kdelibs/kio/kio/ksambashare.h
http://svn.reviewboard.kde.org/r/4320/#comment9877
an empty string
/trunk/KDE/kdelibs/kio/kio/ksambashare.h
http://svn.reviewboard.kde.org/r/4320/#comment9884
If this is deprecated, you can add KDE_DEPRECATED here.
/trunk/KDE/kdelibs/kio/kio/ksambashare.cpp
http://svn.reviewboard.kde.org/r/4320/#comment9878
Not sure if this apidox bit will actually be parsed and shown in api.kde.org
/trunk/KDE/kdelibs/kio/kio/ksambashare.cpp
http://svn.reviewboard.kde.org/r/4320/#comment9879
This variable can be removed:
for (...) {
if (condition) {
foo();
return true;
}
}
...
return false;
/trunk/KDE/kdelibs/kio/kio/ksambashare.cpp
http://svn.reviewboard.kde.org/r/4320/#comment9880
if (...) {
return QString::fromLocal8Bit(...);
}
return QString();
and then we can get rid of result.
/trunk/KDE/kdelibs/kio/kio/ksambashare.cpp
http://svn.reviewboard.kde.org/r/4320/#comment9881
const QFileInfo pathInfo(path);
/trunk/KDE/kdelibs/kio/kio/ksambasharedata.h
http://svn.reviewboard.kde.org/r/4320/#comment9885
A brief apidox description + @since 4.6 would be good to have here.
/trunk/KDE/kdelibs/kio/kio/ksambasharedata.h
http://svn.reviewboard.kde.org/r/4320/#comment9882
Nitpick: use all verbs in the 3rd person?
/trunk/KDE/kdelibs/kio/kio/ksambasharedata.cpp
http://svn.reviewboard.kde.org/r/4320/#comment9883
I _think_ we don't.
- Raphael
On 2010-12-01 01:56:04, Rodrigo Belem wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://svn.reviewboard.kde.org/r/4320/
---
(Updated 2010-12-01 01:56:04)
Review request for kdelibs, Raphael Kubo da Costa, Jonathan Thomas, Aurélien
Gâteau, Jonathan Riddell, Adenilson Cavalcanti, loureiro, and Daniel
Nicoletti.
Summary
---
KDE needs to support modern samba tools. With the net usershare command
line tool the users can manage their shares. The attached patch intends to
add support for this tool.
Diffs
-
/trunk/KDE/kdelibs/includes/CMakeLists.txt 1202449
/trunk/KDE/kdelibs/includes/KSambaShareData PRE-CREATION
/trunk/KDE/kdelibs/kio/CMakeLists.txt 1202449
/trunk/KDE/kdelibs/kio/kio/ksambashare.h 1202449
/trunk/KDE/kdelibs/kio/kio/ksambashare.cpp 1202449
/trunk/KDE/kdelibs/kio/kio/ksambashare_p.h PRE-CREATION
/trunk/KDE/kdelibs/kio/kio/ksambasharedata.h PRE-CREATION
/trunk/KDE/kdelibs/kio/kio/ksambasharedata.cpp PRE-CREATION
/trunk/KDE/kdelibs/kio/kio/ksambasharedata_p.h PRE-CREATION
Diff: http://svn.reviewboard.kde.org/r/4320/diff
Testing
---
Thanks,
Rodrigo
Re: Review Request: Adding net usershare suport for KSambaShare
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/4320/#review8836 --- I think we're finally getting there :) dfaure might know better about the KDirWatch stuff; apart from that, the appropriate @since tags need to be added, as well as the missing method apidox. After that it's probably r+ from me. - Raphael On 2010-11-19 01:13:16, Rodrigo Belem wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/4320/ --- (Updated 2010-11-19 01:13:16) Review request for kdelibs, Raphael Kubo da Costa, Jonathan Thomas, Aurélien Gâteau, Jonathan Riddell, Adenilson Cavalcanti, loureiro, and Daniel Nicoletti. Summary --- KDE needs to support modern samba tools. With the net usershare command line tool the users can manage their shares. The attached patch intends to add support for this tool. Diffs - /trunk/KDE/kdelibs/includes/CMakeLists.txt 1180108 /trunk/KDE/kdelibs/kio/CMakeLists.txt 1180108 /trunk/KDE/kdelibs/kio/kio/ksambashare.h 1180108 /trunk/KDE/kdelibs/kio/kio/ksambashare.cpp 1180108 /trunk/KDE/kdelibs/kio/kio/ksambashare_p.h PRE-CREATION /trunk/KDE/kdelibs/kio/kio/ksambasharedata.h PRE-CREATION /trunk/KDE/kdelibs/kio/kio/ksambasharedata.cpp PRE-CREATION /trunk/KDE/kdelibs/kio/kio/ksambasharedata_p.h PRE-CREATION Diff: http://svn.reviewboard.kde.org/r/4320/diff Testing --- Thanks, Rodrigo
Re: Automoc and Cagibi moved to git.kde.org
At Sun, 31 Oct 2010 23:14:09 +, David Jarvie wrote: [1 text/plain; iso-8859-1 (quoted-printable)] On Saturday 30 October 2010 10:49:33 Christophe Giboudeaux wrote: Hi, Automoc and Cagibi moved to git.kde.org. Please update your local checkouts: - Automoc: http://projects.kde.org/projects/kdesupport/automoc - Cagibi: http://projects.kde.org/projects/kdesupport/cagibi If you prefer gitweb.kde.org: - Automoc: http://gitweb.kde.org/automoc.git - Cagibi: http://gitweb.kde.org/cagibi.git They haven't been removed from the SVN repository yet. So, can I just go there replace the contents of those directories in kdesupport with FOO_MOVED_TO_GIT files? Is there anything else left before these two can be removed from svn?
