Re: Information regarding upcoming Gitlab Migration: clarifications
On Fri, May 01, 2020 at 09:25:18AM -0400, Allen Winter wrote: > On Thursday, April 30, 2020 5:15:43 PM EDT Albert Astals Cid wrote: > > El dijous, 30 d’abril de 2020, a les 21:31:02 CEST, Ben Cooksley va > > escriure: > > > On Fri, May 1, 2020 at 6:04 AM Ivan Čukić wrote: > > > > > > > > > We have made a big fuss in the past about having different projects > > > > > that do the same thing and now we'll have that but also we'll have > > > > > several projects with the same name? > > > > > It really feels off to me and I wonder if this is related to the move > > > > > to > > > > > gitlab. > > > > > > > > +1 to both sentiments - that projects should have different names and > > > > that > > > > this is a bit off topic for the gitlab migration. > > > > > > The projects *DO* have very different names. That *HAS NOT* changed. > > > To use the example Bhushan gave above, one is called Plasma Mobile > > > Dialer and the other one is called Maui Dialer. > > > > > > With the current git.kde.org setup, we have a flat namespace, so all > > > repositories if the name appears to be generic (as dialer is) have to > > > be namespaced by prefixing of the repository name. > > > > > > With Gitlab however we will now namespaces that group repositories, > > > making the prefixing unnecessary and as some projects have complained > > > about, duplicative. > > > > > > Otherwise you end up with plasma-mobile/plasma-mobile-dialer as your > > > path, which just looks silly. > > > > Am I the only person that just has all the repos on the same folder? I > > thought it was the common thing to do :? > > > I use kdesrc-build and I see the repos in a hierarchy. > In particular, I like frameworks in frameworks in kdepim in kde/pim > > I don't see that I'm setting any special "layout in a hierarchy" option in my > kdesrc-buildrc So it's been a few months since we had switched the default, but since it's clearly an invasive change, the way we addressed it was to make the flat hierarchy a default for new users (who use either of the 'quick config' schemes like kdesrc-build-setup or kdesrc-build --initial-setup), but to leave the built-in default unchanged. So in essence, existing kdesrc-build users (who had a folder-based layout by default unless they went out of their way to find the right option) saw no change, but new users would have that option pre-set for them in the config. Regards, - Michael Pyne
Re: Notice of upcoming changes to the behaviour of the anongit network
On Sat, Apr 11, 2020 at 10:14:38PM +1200, Ben Cooksley wrote: > Hi all, > > As part of the preparations for the move to Gitlab, and the rewrite of > our anongit tooling, one of the things we have looked into is how the > anongit network in general operates. > > As part of this, it has been observed that the git:// protocol is > unencrypted, and thus vulnerable to intercept and manipulation by > hostile actors. > > We have therefore decided that support for the git:// protocol to > access KDE Git repositories will cease following our migration to > Gitlab. > > Going forward, all anonymous access should take place instead over > https, which is encrypted, and has the added benefit of offering > support for redirects (should those be needed) For kdesrc-build users, as Johan Ouwerkerk noted on the other Gitlab thread, kdesrc-build since January 2020 has already switched over to using https for KDE-based source repositories in anticipation of this change. Thanks to Ben and the sysadmin team for coordinating ahead of time on this, it's allowed us to have it deployed for 3 months now and we haven't heard of any breakage as a result of this change. One important note is that if you have any git repositories that you have manually checked out using KDE's anongit, you may need to either manually adjust it to use a https:// git remote (if it uses git:// now), or adjust it to use a kde: remote (as explained at https://community.kde.org/Sysadmin/GitKdeOrgManual#Let_Git_rewrite_URL_prefixes). kdesrc-build configures git to understand this "kde:" prefix by default. Regards, - Michael Pyne
Re: Update on Status of Gitlab Migration
On Sat, Apr 11, 2020 at 09:25:11PM +0200, Johan Ouwerkerk wrote: > On Sat, Apr 11, 2020 at 8:39 PM Ben Cooksley wrote: > > > > Yes, the hostname git.kde.org will be fully retired as part of this > > transition. > > > > From my understanding kdesrc-build will automatically pick this up > > once we update sysadmin/repo-metadata to show the new repository > > paths. > > This is something we'll confirm with mpyne though to ensure we can > > make the cutover as smooth as possible. > > > > Just to be clear, my understanding based on reading the > `Updated/Git.pm` module is that KDE repo paths are abstracted via > ~/.gitconfig URL remapping using `insteadOf`and `pushInsteadOf`. > Currently the code manipulates the user's ~/.gitconfig to bind the > correct mappings to the `kde:` prefix (this happens even before > cloning sysadmin repos for metadata). > > So if my understanding of the code is correct, the entire switch over > is transparent provided that kdesrc-build is updated beforehand to set > the updated value for `pushInsteadOf`. We already have the same > mechanism in place in kdesrc-build for ensuring that people use > https://anongit.kde.org instead of git://anongit.kde.org when > cloning/fetching. Yeah, when Ben asked me a couple of months back about it, this was the same conclusion I reached after reviewing the existing code. But I need to note that the way this works right now is that a module is cloned via a URL such as 'kde:juk' (not kde:kde/kdemultimedia/juk!). This is transparent to how git operates when setup with ~/.gitconfig, so you won't notice it in `git remote -v`, you need to actually look at the repository's .git/config, or run `git config --local --get remote.origin.url` in the source directory, to see whether it uses a kde: URL or a full URL. Either way, once the switchover happens, then *in theory* it can be as easy as running kdesrc-build once (to update ~/.gitconfig) and from there Git will rewrite to the updated URL automatically. We could add the switchover logic before that and guard it with a date check, that way we can do some testing early. > > Depending on how things look we may also make available a script that > > will update the configuration of a repository to reflect both the > > change in hostname as well as the change in path. > > > > A cleanup script could be handy. I think kdesrc-build will > automatically pick up new repo paths from metadata and that should > work transparently, but the old clones may get left behind as well. > People who use the kdesrc-build option to ignore KDE repo structure > shouldn't be affected at all. I don't know that we'll even necessarily need a cleanup script (though it couldn't hurt). In my case, my entire source repository contains only one repository directly referencing anongit (or git.kde.org), all others are non-KDE or kde: 1 git://anongit.kde.org/scratch/ 1 git://cmake.org/ 16 git://code.qt.io/ 1 git://git.freedesktop.org/ 1 git://git.gnupg.org/ 3 git://github.com/ 23 https://code.qt.io/ 7 https://github.com/ 1 https://gitlab.com/ 344 kde: All of the kde: repositories use the kde:foo syntax, where the 'foo' comes from the 'repopath' parameter of the sysadmin/repo-metadata YAML files. We may need to do on-the-fly conversion of the kde: repo paths if they won't be expressible as 'kde:foo' in the future, but we should have the information needed to do this in kdesrc-build to make this happen on-the-fly. Regards, - Michael Pyne
Re: kdesrc-build messes with environment
On Wed, Apr 24, 2019 at 02:33:32PM +0200, Milian Wolff wrote: > Hey all, > > for some reason, my `kdesrc-build` uses a different environment than my > normal > shells. I have so far not figured out why that is: > > $ env | grep "^PATH=" > PATH=/home/milian/.bin:/home/milian/projects/compiled/other/bin:/home/ > milian/.bin/kf5:/home/milian/projects/compiled/kf5-dbg/bin:/home/milian/ > projects/compiled/other/bin:/home/milian/projects/compiled/kf5/bin:/usr/local/ > sbin:/usr/local/bin:/usr/bin:/usr/lib/jvm/default/bin:/usr/bin/site_perl:/usr/ > bin/vendor_perl:/usr/bin/core_perl > > $ kdesrc-build --run env | grep "^PATH=" > PATH=/bin:/home/milian/.bin:/home/milian/projects/compiled/other/bin:/home/ > milian/.bin/kf5:/home/milian/projects/compiled/kf5-dbg/bin:/home/milian/ > projects/compiled/other/bin:/home/milian/projects/compiled/kf5/bin:/usr/local/ > sbin:/usr/local/bin:/usr/bin:/usr/lib/jvm/default/bin:/usr/bin/site_perl:/usr/ > bin/vendor_perl:/usr/bin/core_perl > > Note how it prepends /bin to PATH, which leads to all kinds of nasty side > effects for me. I want my PATH to be used as-is, most notably such that some > of the tools I've built myself get picked up, rather than the versions I have > available globally in /bin. > > Does anyone know where this could come from? I suspect it's a kdesrc-build bug and that you have one or both of the `kdedir`, `qtdir` kdesrc-build options unset. This is fine (`kdedir` will fallback to `prefix`, `qtdir` is not required since you can use system Qt), but probably results in something like "/bin:" getting prepended to your path instead of "/path/to/custom/qt/bin:". Please see https://invent.kde.org/kde/kdesrc-build/issues/29 for the bug report. I have built a patch that might work and would appreciate if you could test. Regards, - Michael Pyne
Re: kdesrc-build: git pre-commit hooks
On Sat, Jul 29, 2017 at 06:24:54PM -0400, Allen Winter wrote: > Howdy, > > Is there a way to add a git hooks automagically with the git clones done by > kdesrc-build? > > I have a pre-commit hook that I'd like to have for all my kde src repos. > also I have a pre-push hook , but that's less important to me. > > a post clone command, something as simple this would be enough: > ln -s /path/to/my/githooks/pre-commit.py .git/hooks/pre-commit That sounds fine by me. This would probably be a kdesrc-buildrc config knob that the user should set... do you have an idea on what that would look like? Path a user-defined command to run? Path to a directory containing specially-named hooks? Something else? Regards, - Michael Pyne
Re: Review Request 127866: Oxygen: Fix QCache usage
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127866/ --- (Updated Feb. 26, 2017, 12:28 a.m.) Status -- This change has been marked as submitted. Review request for kde-workspace and Hugo Pereira Da Costa. Repository: oxygen Description --- This should mostly complete the QCache fixes I kicked off in a previous RR, 127837. Hugo noted there were many other similar usages, and boy he wasn't kidding! ;) The long story short is that these usages can theoretically cause use-after-free behavior (which can lead to crashes and even undefined behavior if the compiler ever gets smart). *NOTE* It is -much- easier to review if you download the diff to your git repository for oxygen and then run "git diff -b" to ignore whitespace changes, particularly for the QPixmap changes. For QPixmaps we return values instead of pointers, so we simply make a separate copy to be cached when we do insert. For QColor we return references to values so we *must* return pointers, and those have to be owned by a QCache to avoid memleaks. So I added a helper function to loop until the cache accepts the new entry. TileSets are a similar concern, except those have manual loops since I was uncertain about whether TileSet's copy constructor was the best idea or not. This fixes a ton of Coverity issues (59717 - 259733, 259739, 259742 - 259752, 1336154, 1336155) and might be associated with Qt bug 38142 and KDE bug 219055 (which doesn't actually appear to be a dupe of a different bug to me...). Diffs - kdecoration/oxygendecohelper.cpp aa75eca kstyle/oxygenstyle.cpp e428606 kstyle/oxygenstylehelper.h 9510a60 kstyle/oxygenstylehelper.cpp 612ba37 liboxygen/oxygenhelper.h a6453a0 liboxygen/oxygenhelper.cpp 4843604 liboxygen/oxygenshadowcache.cpp 907e586 Diff: https://git.reviewboard.kde.org/r/127866/diff/ Testing --- Compiled without warnings, installed and ran `oxygen-demo5 -style oxygen`. Used the GUI Benchmark feature to automatically cycle through all the listed features -- no crashes or obvious rendering errors. Thanks, Michael Pyne
Re: CI Requirements - Lessons Not Learnt?
On Thu, Jan 12, 2017 at 05:02:40PM +0100, Martin Gräßlin wrote: > Am 2017-01-12 08:32, schrieb Ben Cooksley: > > It would probably be a good idea to announce it for other developers > > to know about as well so they can sort their systems out. > > that's what we have code review for :-) No, code review isn't for every developer to review every single patch that comes across just to see if it introduces a dependency bump. The cyclomatic complexity of that kind of review graph would be quite extreme ;). Regards, - Michael Pyne
Re: Review Request 129806: Update the list of KCModules
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/129806/#review101960 --- Ship it! Ship It! - Michael Pyne On Jan. 11, 2017, 6:31 p.m., Heiko Becker wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/129806/ > --- > > (Updated Jan. 11, 2017, 6:31 p.m.) > > > Review request for KDE Base Apps and David Faure. > > > Repository: konqueror > > > Description > --- > > * The webshortcuts KCM has been renamed from 'ebrowsing' to > 'webshortcuts' when it was moved from konqueror to kio. > * The crypto KCM has been disabled in kdelibs4 since almost 8 years > [1]. > > [1] > https://cgit.kde.org/kdelibs.git/commit/?id=0ba7b04d006d412524ca7af0604f84c04a921a79 > > > Diffs > - > > src/konqmainwindow.cpp e9e2eb7bf > > Diff: https://git.reviewboard.kde.org/r/129806/diff/ > > > Testing > --- > > Webshortcut settings appear under Configure Konqueror | Web Browing > > > Thanks, > > Heiko Becker > >
Re: Discontinuing repository tarballs
On Wed, Nov 23, 2016 at 08:17:29PM +1300, Ben Cooksley wrote: > HI all, > > As of late Sysadmin has been assessing what components of our > infrastructure are in active use and should be maintained going > forward. > > As part of this it has come to our attention that the repository > tarballs offered by our anongit network (tar.gz files of our > repositories essentially) are effectively unused - with only 19 hits > being received by one mirror in the space of a week. Not an objection, but the low usage may be due to the use of tarball repo snapshots being disabled in kdesrc-build for a few months now. Of course, kdesrc-build was using the snapshots by default out of an effort to be gentler to KDE source code repositories, so if it's easier for you all to forgo the snapshots entirely then I'm fully supportive. Regards, - Michael Pyne
Re: Review Request 127866: Oxygen: Fix QCache usage
> On May 30, 2016, 11:34 a.m., Hugo Pereira Da Costa wrote: > > Ship It! > > Hugo Pereira Da Costa wrote: > err. Wait ... > There are rendering issues here once the patch is applied. > See http://wstaw.org/m/2016/05/30/plasma-desktopY12228.png > (left is "before", right is "after"). > So something seems wrong with the background gradient. > I'll investigate a bit ... > > Hugo Pereira Da Costa wrote: > Hi again, > So, thinking more about it, and actually answering the questions raised > in the review: > > - do we track public API for this part of Oxygen? Does anything in a > different library or application link to this? > No we don't this is supposed to be an "internal" (as in private) library, > used only by oxygen style and decoration. No ABI/API guarantee. > > - QColor: can we change the return type. I would say yes, (from reference > to value), and return to using QCache > - Tilesets: I would be inclined to changing the return type here too, > using values, and assuming the copy constructor does not cost much, based on > the implicit-shareness nature of pixmaps, and keep a built-in QCache here > (which should be more efficient than the (otherwise nice) FIFO. > > Now I understand that this is a lot of going back and forth, and a job I > should rather do myself, as the maintainer of the code. > So I would propose to "postpone" this RR for now, and for me to locally > - take the QPixmap change from this patch > - implement something similar for QColor and TileSet > - test. > Then if I manage to do that in a reasonable time (e.g. this week), drop > the review and commit my change instead (with proper credits where due). > Otherwise (because of me being too busy with other stuff), just commit this > review (once the problem mentionned above is fixed, though I could not > investigate yet). > > What do you think ? > > (also: I need to sanitize this run-time changing of the cache use and > max-cost) Hugo, That all sounds fair. And if it's all too much difficulty, then we might be able to lean on the hinted-at-but-not-quite-documented QCache behavior that ::insert() only fails if the item being inserted has a cost higher than maxCost. In that case I could ignore the Coverity issues (since Coverity can't statically prove that items are always < maxCost) and then drop the RR (and perhaps ask Qt to document more stringently that guarantee for QCache::insert). Still though, I think the QColor return type change would make sense even independent of this RR. - Michael --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127866/#review96029 --- On May 22, 2016, 4:20 a.m., Michael Pyne wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/127866/ > --- > > (Updated May 22, 2016, 4:20 a.m.) > > > Review request for kde-workspace and Hugo Pereira Da Costa. > > > Repository: oxygen > > > Description > --- > > This should mostly complete the QCache fixes I kicked off in a previous RR, > 127837. Hugo noted there were many other similar usages, and boy he wasn't > kidding! ;) The long story short is that these usages can theoretically cause > use-after-free behavior (which can lead to crashes and even undefined > behavior if the compiler ever gets smart). > > *NOTE* It is -much- easier to review if you download the diff to your git > repository for oxygen and then run "git diff -b" to ignore whitespace > changes, particularly for the QPixmap changes. > > For QPixmaps we return values instead of pointers, so we simply make a > separate copy to be cached when we do insert. For QColor we return references > to values so we *must* return pointers, and those have to be owned by a > QCache to avoid memleaks. So I added a helper function to loop until the > cache accepts the new entry. TileSets are a similar concern, except those > have manual loops since I was uncertain about whether TileSet's copy > constructor was the best idea or not. > > This fixes a ton of Coverity issues (59717 - 259733, 259739, 259742 - 259752, > 1336154, 1336155) and might be associated with Qt bug 38142 and KDE bug > 219055 (which doesn't actually appear to be a dupe of a different bug to > me...). > > > Diffs > - > > kdecoration/o
Re: Review Request 127866: Oxygen: Fix QCache usage
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127866/ --- (Updated May 22, 2016, 4:20 a.m.) Review request for kde-workspace and Hugo Pereira Da Costa. Changes --- Updates patch based on much of the feedback, with major reverts to the way I approached TileSets and QColor in particular. For the TileSets I went ahead and implemented what I was talking about regarding a simple FIFO-based cache that holds shared pointers. I used QSharedPointer<> for this since it doesn't require subclassing from QSharedData -- but if this is the only part of the code that uses TileSet it might make sense to subclass from QSharedData instead. Although it builds, installs, makes it through oxygen-demo5 benchmark and all the rest, I'm not sure if the return value changes for TileSet are ABI-safe (do we track public API for this part of Oxygen? Does anything in a different library or application link to this?). On the other hand, if we can change return value, we should also be able to do that for QColor which will significantly improve that portion of the code, as right now the code doesn't 'cache' QColor at all anymore, we just dump them into a QMap that stays alive throughout the process lifetime. After reviewing the QCache sources I'm pretty sure this is all actually only a problem if you try to ::insert() into QCache with a cost > maxCost -- but we have codepaths in Oxygen that appear to lead to either reducing maxCost or disabling the cache entirely so I can understand why Coverity would be wary. But as long as we've convinced that we're not ever inserting entries into a cache with an invalid cost, I can also just flag those Coverity entries to be ignored if that would be easier, and then drop this RR. Repository: oxygen Description --- This should mostly complete the QCache fixes I kicked off in a previous RR, 127837. Hugo noted there were many other similar usages, and boy he wasn't kidding! ;) The long story short is that these usages can theoretically cause use-after-free behavior (which can lead to crashes and even undefined behavior if the compiler ever gets smart). *NOTE* It is -much- easier to review if you download the diff to your git repository for oxygen and then run "git diff -b" to ignore whitespace changes, particularly for the QPixmap changes. For QPixmaps we return values instead of pointers, so we simply make a separate copy to be cached when we do insert. For QColor we return references to values so we *must* return pointers, and those have to be owned by a QCache to avoid memleaks. So I added a helper function to loop until the cache accepts the new entry. TileSets are a similar concern, except those have manual loops since I was uncertain about whether TileSet's copy constructor was the best idea or not. This fixes a ton of Coverity issues (59717 - 259733, 259739, 259742 - 259752, 1336154, 1336155) and might be associated with Qt bug 38142 and KDE bug 219055 (which doesn't actually appear to be a dupe of a different bug to me...). Diffs (updated) - kdecoration/oxygendecohelper.cpp aa75eca kstyle/oxygenstyle.cpp e428606 kstyle/oxygenstylehelper.h 9510a60 kstyle/oxygenstylehelper.cpp 612ba37 liboxygen/oxygenhelper.h a6453a0 liboxygen/oxygenhelper.cpp 4843604 liboxygen/oxygenshadowcache.cpp 907e586 Diff: https://git.reviewboard.kde.org/r/127866/diff/ Testing --- Compiled without warnings, installed and ran `oxygen-demo5 -style oxygen`. Used the GUI Benchmark feature to automatically cycle through all the listed features -- no crashes or obvious rendering errors. Thanks, Michael Pyne
Re: Review Request 127866: Oxygen: Fix QCache usage
> On May 15, 2016, 8:51 a.m., Hugo Pereira Da Costa wrote: > > To be honest, I am quite puzzle by this whole thing. > > Now, every insertion in the cache requires at least two searches in there > > and (in many case) at least one copy constructor being called. This is > > quite expansive ... (even though this happens only if the object is not > > found in the cache). > > > > Also: not sure I understand what issue we are trying to fix and how: why is > > it that if the object inserted in the cache is immediately deleted, just > > retrying an indefinite amount of time will "fix" the issue. Are we not just > > transforming a crash into a freeze (infinite loop) ? > > > > The Qt documentation is very vague about cases where the object is deleted > > immediately, and the only case it mentions is: " In particular, if cost is > > greater than maxCost(), the object will be deleted immediately." > > Well, in such cases (that should not appear here), the infinite loop will > > not help. Right ? > > Since we have no idea on how "predictible" the other deletion cases are, I > > don't think the fix is a good fix. > > > > Does that mean that we should change the code in order to use references > > rather than pointer everywhere ? (as you did in the first patch on this > > topic) ? > > > > Or get rid of using QCache (because this absence of guarantee at the > > insertion stage is too much of a pain to handle) ? > > > > Or just commit and wait for bug reports about freezes ? (but with a happy > > coverty) ? > > Michael Pyne wrote: > For the most part the requirements are determined by the current return > type within the code. If we return a pointer then currently it has to have > come directly out of the QCache. Since QCache is assumed to be the owner, the > calling code won't delete the pointer ; but if the caller won't delete the > pointer then we'll have a memory leak if we return a pointer to something > that had been new'd instead. > > References (i.e. QColor&) are a similar issue; it's safe enough to return > a reference to something held within the QCache, but we can't return a > reference to a local variable since that reference will invalidate as soon as > we return from the function. Of course a reference to a cached QColor may > *also* invalidate with the next call to an insert method of that cache, but > that's a separate story. > > It is unfortunate that the Qt docs are vague about this, since if the > **only** thing we had to worry about was cost being >maxCost(), we could > pretty much just mark 'ignore' for all the Coverity issues associated with > this (and I'd be fine doing that). The docs do kind of hint at that but don't > make it clear if is the only way that an entry would be deleted immediately. > > I think you're right that a loop is not a good idea... I was figuring > that eventually QCache would remove enough other items to make it work but > then I suppose QCache::insert() would have done that with the very first > attempt. > > As far as other options, I would definitely recommend against QCache for > the QColors: I'd say just hold onto specific QColors directly (perhaps in a > QHash) and, if possible, return them as values instead of references. > > I'm not sure if we could get away with the same for TileSets, but if so > it would again make things easier. If we can't we could look into making > TileSet an implicitly shared class so that we can return it by value cheaply. > > I wouldn't recommend a commit only to make Coverity happy. I've marked > other reports as "False Positive" and even "That's a bug, but we're ignoring > it" before. But it does seem to me that if a crash *is* possible (especially > in underlying library code) we should do something to avoid it. > > Either way I'll see if I can work up a revised patch but I'd still > appreciate advice on what's workable or not within Oxygen. > > Mark Gaiser wrote: > Disclaimer: i'm not an oxygen dev! I just read the patch and want to > share my opinion :) > > I'm also quite puzzeled with this change.. > > A cache is "usually" being used to store the result of a "heavy" > operation and use that result the next time to speed things up. > That principle sounds great to me and should be used in places if needed. > A better approach would be to speed up the slow operation to make caching > simply not needed, but that's a different story. > > The changes you've made now make Q
Re: Review Request 127866: Oxygen: Fix QCache usage
> On May 15, 2016, 8:51 a.m., Hugo Pereira Da Costa wrote: > > To be honest, I am quite puzzle by this whole thing. > > Now, every insertion in the cache requires at least two searches in there > > and (in many case) at least one copy constructor being called. This is > > quite expansive ... (even though this happens only if the object is not > > found in the cache). > > > > Also: not sure I understand what issue we are trying to fix and how: why is > > it that if the object inserted in the cache is immediately deleted, just > > retrying an indefinite amount of time will "fix" the issue. Are we not just > > transforming a crash into a freeze (infinite loop) ? > > > > The Qt documentation is very vague about cases where the object is deleted > > immediately, and the only case it mentions is: " In particular, if cost is > > greater than maxCost(), the object will be deleted immediately." > > Well, in such cases (that should not appear here), the infinite loop will > > not help. Right ? > > Since we have no idea on how "predictible" the other deletion cases are, I > > don't think the fix is a good fix. > > > > Does that mean that we should change the code in order to use references > > rather than pointer everywhere ? (as you did in the first patch on this > > topic) ? > > > > Or get rid of using QCache (because this absence of guarantee at the > > insertion stage is too much of a pain to handle) ? > > > > Or just commit and wait for bug reports about freezes ? (but with a happy > > coverty) ? For the most part the requirements are determined by the current return type within the code. If we return a pointer then currently it has to have come directly out of the QCache. Since QCache is assumed to be the owner, the calling code won't delete the pointer ; but if the caller won't delete the pointer then we'll have a memory leak if we return a pointer to something that had been new'd instead. References (i.e. QColor&) are a similar issue; it's safe enough to return a reference to something held within the QCache, but we can't return a reference to a local variable since that reference will invalidate as soon as we return from the function. Of course a reference to a cached QColor may *also* invalidate with the next call to an insert method of that cache, but that's a separate story. It is unfortunate that the Qt docs are vague about this, since if the **only** thing we had to worry about was cost being >maxCost(), we could pretty much just mark 'ignore' for all the Coverity issues associated with this (and I'd be fine doing that). The docs do kind of hint at that but don't make it clear if is the only way that an entry would be deleted immediately. I think you're right that a loop is not a good idea... I was figuring that eventually QCache would remove enough other items to make it work but then I suppose QCache::insert() would have done that with the very first attempt. As far as other options, I would definitely recommend against QCache for the QColors: I'd say just hold onto specific QColors directly (perhaps in a QHash) and, if possible, return them as values instead of references. I'm not sure if we could get away with the same for TileSets, but if so it would again make things easier. If we can't we could look into making TileSet an implicitly shared class so that we can return it by value cheaply. I wouldn't recommend a commit only to make Coverity happy. I've marked other reports as "False Positive" and even "That's a bug, but we're ignoring it" before. But it does seem to me that if a crash *is* possible (especially in underlying library code) we should do something to avoid it. Either way I'll see if I can work up a revised patch but I'd still appreciate advice on what's workable or not within Oxygen. - Michael ------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127866/#review95480 --- On May 8, 2016, 5:03 a.m., Michael Pyne wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/127866/ > --- > > (Updated May 8, 2016, 5:03 a.m.) > > > Review request for kde-workspace and Hugo Pereira Da Costa. > > > Repository: oxygen > > > Description > --- > > This should mostly complete the QCache fixes I kicked off in a previous RR, > 127837. Hugo noted there were many other similar usages, and boy he wasn't > kidding!
Review Request 127866: Oxygen: Fix QCache usage
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127866/ --- Review request for kde-workspace and Hugo Pereira Da Costa. Repository: oxygen Description --- This should mostly complete the QCache fixes I kicked off in a previous RR, 127837. Hugo noted there were many other similar usages, and boy he wasn't kidding! ;) The long story short is that these usages can theoretically cause use-after-free behavior (which can lead to crashes and even undefined behavior if the compiler ever gets smart). *NOTE* It is -much- easier to review if you download the diff to your git repository for oxygen and then run "git diff -b" to ignore whitespace changes, particularly for the QPixmap changes. For QPixmaps we return values instead of pointers, so we simply make a separate copy to be cached when we do insert. For QColor we return references to values so we *must* return pointers, and those have to be owned by a QCache to avoid memleaks. So I added a helper function to loop until the cache accepts the new entry. TileSets are a similar concern, except those have manual loops since I was uncertain about whether TileSet's copy constructor was the best idea or not. This fixes a ton of Coverity issues (59717 - 259733, 259739, 259742 - 259752, 1336154, 1336155) and might be associated with Qt bug 38142 and KDE bug 219055 (which doesn't actually appear to be a dupe of a different bug to me...). Diffs - kstyle/oxygenstylehelper.cpp 612ba37 liboxygen/oxygenhelper.h a6453a0 liboxygen/oxygenhelper.cpp 4843604 liboxygen/oxygenshadowcache.cpp 907e586 Diff: https://git.reviewboard.kde.org/r/127866/diff/ Testing --- Compiled without warnings, installed and ran `oxygen-demo5 -style oxygen`. Used the GUI Benchmark feature to automatically cycle through all the listed features -- no crashes or obvious rendering errors. Thanks, Michael Pyne
Re: Review Request 127629: Fix KDateTime::isValid() for ClockTime values
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127629/#review95194 --- Ship it! Ship It! - Michael Pyne On April 10, 2016, 6:43 p.m., David Jarvie wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/127629/ > --- > > (Updated April 10, 2016, 6:43 p.m.) > > > Review request for kdelibs. > > > Bugs: 336738 > https://bugs.kde.org/show_bug.cgi?id=336738 > > > Repository: kdelibs4support > > > Description > --- > > KDateTime::isValid() wrongly returns invalid for an instance which is > specified in ClockTime, if the date/time is invalid in the local time zone. > This is due to the internal QDateTime value being set to Qt::LocalTime, and > unlike in Qt4, QDateTime in Qt5 uses the local time zone to validate the > date/time. For ClockTime, there is no associated time zone (and choosing the > local time zone to perform validation is purely arbitrary), so it should > ignore the local time zone when validating. > > > Diffs > - > > autotests/kdatetimetest.cpp a8e7749 > src/kdecore/kdatetime.cpp c530db1 > > Diff: https://git.reviewboard.kde.org/r/127629/diff/ > > > Testing > --- > > Test added to kdatetimetest, all tests pass. > > > Thanks, > > David Jarvie > >
Re: Why is C90 enforced in KDE?
On Sun, December 6, 2015 16:08:04 Antonio Rojas wrote: > Hi, > Kipi-plugins fails to build with flex 2.6. This is due to the autogenerated > code in libpanorama containing //-style comments, which are disallowed in > C90. Adding -std=c99 to the CFLAGS at compile time doesn't have any effect, > since it is overriden by kdelibs (and by extra-cmake-modules in KF5). What > is the reason for this? And is there any way to force using C99? I'm not sure offhand... it may be due to trying to support builds using MSVC on Windows (which has poor C99 or later support). On the other hand even MSVC supports //comments in C code. I think there's some way to get CMake to add compiler flags for .c files in particular; another option is to have flex generate a .cpp file, assuming that the code being generated is safely in the common subset of C and C++. Regards, - Michael Pyne
Re: kde-workspace repository locked
On Sun, September 27, 2015 09:57:30 Ben Cooksley wrote: > Hi all, > > Per the request of the Plasma team, the kde-workspace repository on > git.kde.org has now been locked to prevent any further changes. > > If there are any problems with this, please let us know. I'd just ask that a notice be left within the files of repository itself (e.g. a README.git) with the same information, that way one won't have to web search later to figure out why they can't push to kde-workspace. Though even there I suppose that would leave open the question of which git branches to leave the readme in... but 'master' should suffice, I would think. Regards, - Michael Pyne
Re: Proposal to improving KDE Software Repository Organization
On Sun, August 16, 2015 17:48:59 John Layt wrote: On 16 August 2015 at 11:14, David Faure fa...@kde.org wrote: (*) I keep finding the division term a bit obscure, and I wonder if this shouldn't be called product instead. I.e. matching how we release things. Nowadays we basically have 4 products (frameworks, plasma, applications, extragear?), previously we had 2 (KDE SC 4, extragear). If this is what you had in mind with divisions, then I suggest to call it product for clarity. The reason why I think it's the same concept is that the one reason to have different tracks is exactly because of different release schedules (e.g. plasma could be tested against KF5/Stable (last release) and KF5/Devel (more recent code)) [Rambling naming bikeshed alert !] Just to be clear I agree with David that product would better match the intent of my proposal than division; I've spent the past year or so not being very satisfied with division. TLDR: We have a Marketing View and we have a Technical Build View and I think they are different enough to have different names and structures. How about we call 'division' something like 'build-group' or 'build-unit' instead? And while we're busy regrouping and renaming things, lets get rid of the application Modules... snip So, a proposal: we drop modules and extragear and playground and unmaintained altogether for organising our repos and paths and build process and instead just have Applications with a 'release-status' tag marking where they are in our official KDE Application Lifecycle [3]. A second 'release-cycle' tag could identify those that are to be included in the regular KDE Applications release cycle. I think this is probably a good conversation to have as well but I do want to re-emphasize that the proposal David is referring to relates to how we build KDE software, and how we organize to build that software easily. This involves grappling with at least a couple of things: - What underlying source repositories make up a given major product that we ship? (e.g. what comprised Plasma 5? what makes up KF5?). We have to care at least about this level of granularity since each of these may have their own release cycles. - For each of those repositories, what is the appropriate branch to build from, for a given development branch (e.g. stable or dev)? One of the weaknesses of the current branch-group scheme is that we list all KDE repositories *first* and only then try to map to a branch... but not every repository necessarily belongs with every major product. But beyond that we already really don't care about extragear/, playground/, etc., at least as far as build infrastructure goes. The reason we'd split playground or Extragear in the new scheme would be because they operate on different release schedules from Plasma, Applications or KF5 (and therefore probably have different ideas of 'Release', 'Dev', 'Stable', etc.). For playground in particular it would also give us a way to lower the priority for those from a CI perspective. But we don't have to make that part of our marketing efforts and I'm not aware that we do even today. We can still sort-of keep module, but now it's more a category tag, so all edu apps live under the path apps/edu/*. This could also remove some hassle when apps move around, their place in the namespace hierarchy and path doesn't change, just their release status tag. Well, having a project hierarchy is also a slightly different question. There's no reason even with our current build metadata that we'd *have* to have project hierarchies, as long as each underlying git repository name remains unique. It might even make things easier since there would be no way for a sub-path in our project hierarchy (such as kde/kdelibs/kactivities) to mask a git repository name (kdelibs in this case). If we got rid of this it would take away the ability to easily refer collectively to related modules though, and I don't think we'd want to reimplement that with the high-level Project scheme being talked about. Regards, - Michael Pyne
Re: Using nullptr instead of Q_NULLPTR
On Fri, August 14, 2015 22:49:44 Thomas Lübking wrote: On Freitag, 14. August 2015 22:34:28 CEST, Thiago Macieira wrote: Don't do that. If you use nullptr, there's no going back to zero because it's not the same. You've irrevocably locked yourself into requiring a compiler that supports it. Even if it's only been used in a 0x0 compliant way? Some ABI trap? A source compat trap perhaps. nullptr is unequivocally a *pointer*, and names the null pointer for whatever pointer type it's cast to. 0 can be a pointer or an integer, so code that previously 'worked' using nullptr might become ambiguous (or worse, change behavior) if switched to 0. In fact there was a JuK bug with gstreamer many years ago where 64-bit JuK would crash because passing NULL as a sentinel to a gstreamer varargs function was turned into passing 0, which C++ treated as an (32-bit) int instead of a pointer due to its use as a varargs argument. This wouldn't have happened if nullptr had been used. Regards, - Michael Pyne
Re: Updating our tools
On Sun, August 9, 2015 09:58:26 Allen Winter wrote: On Sunday, August 09, 2015 09:35:06 PM Ben Cooksley wrote: On Sun, Aug 9, 2015 at 3:15 AM, Allen Winter win...@kde.org wrote: On Saturday, August 08, 2015 04:59:49 PM Elvis Angelaccio wrote: Sorry to bump this old thread, but it looks like Krazy still complains about kdelibs4 errors even if an application is now KF5 based. For instance consider again Kanagram: http://ebn.kde.org/krazy/reports/kde-4.x/kdeedu/kanagram/index.html Am I missing something? Is there another page showing KF5-related issues for KF5-ready apps? If Krazy is running in the kde-4.x component, then it does use the KDE4 checkers. For now, Krazy only knows its looking at KF5 code if it's running in the frameworks 5 component in http://ebn.kde.org/krazy/index.php?component=frameworksmodule=framewor ks5 I don't see kanagram listed in the frameworks5 component. The KDE projects also lists kanagram in kde-4.x only, as far as I can tell. So first step is to get kanagram listed as a frameworks project. To my knowledge there isn't anything on projects.kde.org which states a project is kde-4.x only. Where is this information coming from? the projects db I see that kanagram master is kf5 based but is not part of frameworks. therefore I will need to invent a way to detect that a project is kf5 based. do we have an easy way to detect kde4 vs. kf5 projects? The only way I'm aware of is to use the kde-build-metadata/logical-module- structure metadata (this is used by kdesrc-build and the CI). This is much more annoying than a simple lookup but there are at least a couple of tools in kde-build-metadata that can be used to help. I believe i18n tracks the 'unstable' and 'stable' branches for each module and 'unstable' might mean KF5 by now. This information *is* available with the project data directly. But maybe Albert or Luigi could better explain how the i18n infra is setup and what we can conclude from these variables. Regards, - Michael Pyne
Re: Strigi usage in KF5 world
On Sun, April 5, 2015 20:47:03 šumski wrote: Now i got suggestion by Vishesh and Albert to ask for a general decision regarding strigi-related code. IMO the code should be killed, as the classes are deprecated even in kdelibs4 code, so there isn't much point in bringing it all back in kdelibs4support, even though lxr.kde.org still shows some users... What do others think? I was honestly surprised we still used it when I first built KF5. I suspect it's because it was easier to port strigi (which is after all not strongly tied to a toolkit) than to port its users, not because of a positive decision to keep using strigi. But I don't know, I haven't done a lot of KF5 porting... Regards, - Michael Pyne
Re: Build dependency issues with kdesrc-build
On Fri, February 6, 2015 15:11:22 Kevin Funk wrote: On Thursday 05 February 2015 22:16:54 Michael Pyne wrote: However as of now it only reorders modules you pull into the build list, so you still need to specify dependencies somehow. E.g. if you only asked to build plasmate, kdesrc-build wouldn't pull kdevplatform for you, but if you asked to build both kdesrc-build would do it in the right order. Question: Can't we add an option that does exactly that? Anything that'd prevent us to do so? I've been meaning to forever now. :-/ The only real catch is that you'd probably want to have a way to specify dependencies not to include, and what to do with soft deps. Please note that the kf5-*-build-include files with kdesrc-build are not authoritative information about what depends on what, they are very coarse groupings intended to help with high-level organization. Hm, but in fact it gives you the information about the actual package dependencies pretty precisely, no? (Otherwise the whole CI infrastructure wouldn't work -- Our CI scripts can figure out the exact dependency set needed for a build) We're talking about two different things. The CI scripts (and kdesrc-build) use data from the kde-build-metadata git module, which *is* authoritative regarding which modules depend on which. The kf5-*-build-include files come with kdesrc-build itself, and are generally used with a sample kdesrc-buildrc to tell kdesrc-build which KF5 modules to build. These files don't include dependencies per se (though the order that modules are listed is used as a hint in the absence of any other dependency information). Rather they are used to get kdesrc-build to bother with building those specific KF5 modules in the first place. Regards, - Michael Pyne
Re: Build dependency issues with kdesrc-build
On Tue, February 3, 2015 00:54:25 Albert Astals Cid wrote: El Dissabte, 31 de gener de 2015, a les 19:20:06, Mathieu Tarral va escriure: Hi, I noticed an issue with the plasmate component, which is build when you include kf5-workspace-build-include. It has a build dependency on KDevPlatform, but this component is only selected in kf5-applications-build-include. So should we move plasmate into Applications ? I have no idea how kdesrc-build but i'd hope/guess/expect it maps the virtual hierarchy of the repos, so probably plasmate should go to something extragear- ish that can depend on kdevlplatform just fine? But as said i know not much of kdesrc-build so i could be talking crap :D kdesrc-build will reorder modules into the right build order (assuming the needed metadata in kde-build-metadata is present). However as of now it only reorders modules you pull into the build list, so you still need to specify dependencies somehow. E.g. if you only asked to build plasmate, kdesrc-build wouldn't pull kdevplatform for you, but if you asked to build both kdesrc-build would do it in the right order. Please note that the kf5-*-build-include files with kdesrc-build are not authoritative information about what depends on what, they are very coarse groupings intended to help with high-level organization. You don't even need to use kf5-*-build-include, you can make your own kdesrc-buildrc that does the same thing; you can consider them as maintained sample files. Regards, - Michael Pyne
Re: Proposal to improving KDE Software Repository Organization
On Tue, August 19, 2014 10:18:17 David Faure wrote: On Tuesday 19 August 2014 19:10:14 Ben Cooksley wrote: The old kf5-qt5 / latest-qt4 names are being mapped to division / track combinations. They are otherwise not used. Ah! Just a clarification though: there would only be two divisions for the above scenario: Plasma5 and KF5. Plasma5 would then have two tracks: stable and devel. KF5 would have it's single track. Ah! OK it's a lot clearer to me now. I thought a division was an overall selection of everything (like kf5-qt5 was). Now I see, it's a group of modules, e.g. just KF5, or just Plasma5. This makes a lot more sense, and leads to understandable naming, I like it :) I assume a future kdesrc-buildrc will look like a selection of *one or many* divisions, with a track for each one, then. Yes, and branch groups simply become a pre-tested set of divisions (I would imagine it simply defines the various combinations CI would test in the future, though that concept is still up to Ben). In the kdesrc-build world you'd still be able to pick divisions (and tracks), individual modules, etc. There's two feasible ways to go with the branch-group option though: It is used to pick the right branch or track if not specified, or it is used as CI would use it, to pull in an entire set of divisions in one fell swoop. I'm open to other ideas, but that's a topic for a separate thread in any event. Regards, - Michael Pyne
Re: Proposal to improving KDE Software Repository Organization
On Wed, August 20, 2014 00:48:36 Àlex Fiestas wrote: On Monday 18 August 2014 21:54:40 Michael Pyne wrote: We await your comments, suggestions, clarification requests, and other feedback. The proposed solution will clearly help to improve the situation, so +1! Something I would like to explore is the possibility of putting on each repository the information relative to it, and then maybe via jenkins generate the unified file. The pros of this is that we have the information about each project contain on itself making it easier for maintainers to keep the information up to date and also making it easier to discover this information (not many people know about kde-build-metadata and surely not third party Actually this was my initial idea. I had even tried to sell the sysadmins on the idea of expanding the information recorded about a project in the projects.kde.org interface to allow for this. However we ended up noting some issues. Eventually, the idea at the creation of kde-build-metadata was that this information would be centralized there (not at the repo) so that maintainer action would not be required to make things work. Not to say that maintainer action is not desired; just that it wouldn't be mandated to make things work right. Although it's true that moving data to the repo doesn't mean errors couldn't be fixed by KDE developers who happen to notice it, that would require cloning the whole repo (since it's fairly likely that it's not already be checked out), making the fix, pushing it, you know the drill. With kde-build-metadata you already have it checked out by definition, and the ones most likely to notice a test failure know where to make the fix (or at least where to point the repo's maintainer). Additionally there's actually a couple of scripts in kde-build-metadata's tools/ subdirectory that would be much less useful if they had to wait for Jenkins to re-generate the centralized file, and these tools are useful for validating the JSON format used and proving that the dependencies are as expected, so I think that would increase the odds of introducing errors by accident. As it stands I don't think discoverability is a big concern here. If a maintainer is making a repo from a template, we could add a pointer to the right repo in the template itself. If a maintainer is already maintaining a repository, they'd have to hear the news to know to add a new metadata file, but if they're able to receive that news it's easy enough to point them to kde-build-metadata instead. The one problem is developers making a new repository by simply copying the structure of an old one, but then they'd hear about dependency data when their new repo doesn't work on Jenkins. ;) With all that said there's no reason we can't make the data in kde-build- metadata more easily available to developers in a more-preferred location (maybe at projects.kde.org?), but I don't think I'll be the one implementing it. Regards, - Michael Pyne signature.asc Description: This is a digitally signed message part.
Proposal to improving KDE Software Repository Organization
Hi all, Ben Cooksley and I would like to get some feedback on further evolutions to the organization structure we employ for the repositories at git.kde.org, to allow our current usage of CI even as we move farther into the KF5-based world. TL;DR: More indirection in our JSON in kde-build-metadata, not a lot of end- user visible change, new org. terms: Division and Track for multi-repo organization, tracking inter-repo dependencies would change too (sayonara dependency-data-$branch_group), less CI servers turned into melting piles of slag. +1? The proposal follows for those who like reading excessively wordy text. Regards, - Michael Pyne Improving KDE Project Organization: A Proposal == 18 Aug 2014 Michael Pyne mp...@kde.org and Ben Cooksley bcooks...@kde.org This is a proposal to evolve the current method of organizing our mass of KDE source code repositories, and their dependencies, as contained in the `kde-build-metadata` repository and used by kdesrc-build and build.kde.org (referred to as CI). This is needed in order to correct some deficiencies in the [current specification](https://community.kde.org/Infrastructure/Project_Metadata), and to help better support changing trends in developer workflow. Current Situation = If you're familiar with the current organization of KDE build metadata you should skip to the next section. Currently, the git-based source code repositories that make up KDE.org's software releases are each given a project path that fully specifies the name of the module in a virtual hierarchy. For instance, kdesrc-build itself is really extragear/utils/kdesrc-build, and KDE 4's kdelibs is kde/kdelibs. Since many modules support KDE4 and Qt5/KF5 (or may in the future), some developers associated with KDE source code repositories introduced the branch group construct, that maps the git repository branch for the majority of repositories into a few broad groupings, such as stable-qt4, latest-qt4 and kf5-qt5. Developers and users using kdesrc-build could then use these groups to easily build the appropriate git branch of the many repositories needed for current releases of KDE.org software. This also allowed the CI infrastructure to support testing the development branches of both software using both KDE4 and KF5, in addition to the libraries/Frameworks themselves. Current Issues == Things have gone fairly well with branch groups, but there have been minor issues with the construct: 1. The existing metadata listing dependencies between git repositories could not support multiple branch groups, as the dependencies were not necessarily identical for a given repository, for every possible branch group it belonged to. We worked around this by forking the metadata such that each different branch group used a separate dependency file. 2. Compounding that issue, different branch groups would have different sets of repositories. For instance some repositories will never have a KF5-based release due to ongoing reorganization, and many repositories were born for KF5. By common agreement, software using `kde-build-metadata` now recognize empty git branch names to mean that a repository doesn't actually belong to the given branch group. This is still a workaround, however; if we forget to manually specify an empty branch, then CI and kdesrc-build will both try to build that repository as part of that branch group (using a default branch). Upcoming Problems = A larger concern (and what instigated this effort) is that the KF5 era will introduce multiple development models that are difficult for the CI infrastructure to efficiently support. For example, testing the KF5-based Plasma 5 Workspace will eventually need to test both the stable and development tracks for Plasma 5. Under the branch group concept, this would lead to branch groups kf5-qt5 and kf5-qt5-stable (or similar names). However the KF5 repositories that Plasma 5 requires do not have a split between stable and devel: They use a review-required process by which there's only one development track. In other words, Plasma 5's two development tracks will only depend on 1 KF5 track. At this time, that means CI will have to build 56 KF5 modules to test Plasme 5-stable, and then re-build, re-install, etc. the exact same 56 modules to then test Plasma 5-devel. This re-build is required because experience has shown that built repositories cannot be assumed to be compatible between different branch groups (in fact many repositories are significantly different on-disk between branch groups). There's simply no data recorded at this point that delimits the ways in which repositories would remain compatible (or not) between different branch group combinations. Solving this (so that the right 56 modules are retained and re-used) would require quite some manual hackery, and it's uncertain how easy
Re: Review Request 119497: Report crashes of KDE apps in Apple OS X (1) (fix kcrash, kinit)
On July 27, 2014, 11:32 a.m., Thomas Lübking wrote: kdeui/util/kcrash.cpp, line 316 https://git.reviewboard.kde.org/r/119497/diff/1/?file=293441#file293441line316 is libdispatch OSX only or is it also used on FreeBSD? The question (more to Michael ;-) is whether unconditionally closing all FDs is acceptable in general (there's hell lot of Q_OS_* items, incl. Q_OS_HURD ;-) or the check should be inverted to #ifdef Q_OS_LINUX Michael Pyne wrote: Unfortunately closing all FDs is probably not acceptable *in general*, but in the context of a launching a crash handler in a crashed KDE GUI application, I don't see any reason why it would hurt, it's not as if the application itself is going to need the FD anymore as it's about to crash and has already run its emergency save function (and if the app does need it, they can either use their own crash function or use the documented KeepFD flag). Closing all FDs is a security and rlimit precaution in case Dr. Konqi gets started directly from the crashing proc instead of via kdeinit. The check should probably remain on any POSIX-alike, in my opinion, as we wouldn't want Dr. Konqi on Mac OS X accidentally having access to a crashing application FDs if launched directly either. Ian Wadham wrote: On Apple OS X it is Apple's COCOA library internals that have the problem FDs, not the app, and COCOA crashes internally if you close its FDs in the crash handler. We do not know what numbers those FDs have in any arbitrary app or run of an app, and I think there is intrinsically no way to know. We also do not know what FDs may have been created by internals of the KDE library, but they do not seem to mind being closed peremptorily. I could put a setFlags call (conditional on Q_OS_MAC) a few lines earlier in the default crash-handler, but there does not seem to be much point in that. It is quite safe to leave FDs open, because KCrash closes them all unconditionally on line 623 of kcrash.cpp (if it starts Dr K by forking), with the comment We are in the child now. Close FDs unconditionally. Presumably, by that time, the COCOA library has had a chance to terminate its internals gracefully, because this time there is no secondary crash. Presumably also, if Dr K is started via kinit and klauncher, its FDs are all its own, but kinit is a very grey area for me. Yes, you're right. It seems in the only problematic case (a direct fork/exec to start drkonqi) that we already close FDs unconditionally. In that case I think don't closing the FDs from the crash handler adds any value (and I suppose, might even interfere in debugging once drkonqi can get a debugger attached), so it might be best to remove the feature from the default crash handler entirely. David, thoughts? - Michael --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119497/#review63257 --- On July 27, 2014, 9:15 a.m., Ian Wadham wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119497/ --- (Updated July 27, 2014, 9:15 a.m.) Review request for KDE Software on Mac OS X, KDE Runtime, kdelibs, and Michael Pyne. Repository: kdelibs Description --- When a KDE app crashes in Apple OS X, it just disappears from the screen. At the most, the user is invited to report the crash to Apple. AFAIK this has been a problem in KDE on Apple OS X for years, leading to frustration with KDE among Apple users and MacPorts developers and an attitude among KDE developers of Why does nobody report the problem(s) on bugs.kde.org? It is my strong belief that the failure to report crashes of KDE apps in Apple OS X also exists in Frameworks. So far I have identified a number of portability bugs in KDE on Apple OS X: 1 in KCrash, 1 in kdeinit4 and 5 in Dr Konqi. Patches for the first two are submitted in this review. Patches for three more are submitted in part 2 of this review, against kde-runtime. I am still investigating the other two problems in Dr Konqi - and there could be more than two... In this review we have two portability problems: 1. KCrash itself crashes (SIGILL) when it tries to close all file descriptors and so Dr Konqi is not started. Some of the FDs belong to the Apple OS X library (COCOA) and it crashes if they are closed prematurely. 2. The preferred way to start Dr K is via a socket message to kdeinit4, but that fails in Apple OS X because kdeinit4 is listening with the wrong socket name. The DISPLAY ID is missing from the end of the name. The fallback is for KCrash to use fork() and exec
Re: Review Request 119497: Report crashes of KDE apps in Apple OS X (1) (fix kcrash, kinit)
On July 27, 2014, 11:32 a.m., Thomas Lübking wrote: kdeui/util/kcrash.cpp, line 316 https://git.reviewboard.kde.org/r/119497/diff/1/?file=293441#file293441line316 is libdispatch OSX only or is it also used on FreeBSD? The question (more to Michael ;-) is whether unconditionally closing all FDs is acceptable in general (there's hell lot of Q_OS_* items, incl. Q_OS_HURD ;-) or the check should be inverted to #ifdef Q_OS_LINUX Unfortunately closing all FDs is probably not acceptable *in general*, but in the context of a launching a crash handler in a crashed KDE GUI application, I don't see any reason why it would hurt, it's not as if the application itself is going to need the FD anymore as it's about to crash and has already run its emergency save function (and if the app does need it, they can either use their own crash function or use the documented KeepFD flag). Closing all FDs is a security and rlimit precaution in case Dr. Konqi gets started directly from the crashing proc instead of via kdeinit. The check should probably remain on any POSIX-alike, in my opinion, as we wouldn't want Dr. Konqi on Mac OS X accidentally having access to a crashing application FDs if launched directly either. - Michael --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119497/#review63257 --- On July 27, 2014, 9:15 a.m., Ian Wadham wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119497/ --- (Updated July 27, 2014, 9:15 a.m.) Review request for KDE Software on Mac OS X, KDE Runtime, kdelibs, and Michael Pyne. Repository: kdelibs Description --- When a KDE app crashes in Apple OS X, it just disappears from the screen. At the most, the user is invited to report the crash to Apple. AFAIK this has been a problem in KDE on Apple OS X for years, leading to frustration with KDE among Apple users and MacPorts developers and an attitude among KDE developers of Why does nobody report the problem(s) on bugs.kde.org? It is my strong belief that the failure to report crashes of KDE apps in Apple OS X also exists in Frameworks. So far I have identified a number of portability bugs in KDE on Apple OS X: 1 in KCrash, 1 in kdeinit4 and 5 in Dr Konqi. Patches for the first two are submitted in this review. Patches for three more are submitted in part 2 of this review, against kde-runtime. I am still investigating the other two problems in Dr Konqi - and there could be more than two... In this review we have two portability problems: 1. KCrash itself crashes (SIGILL) when it tries to close all file descriptors and so Dr Konqi is not started. Some of the FDs belong to the Apple OS X library (COCOA) and it crashes if they are closed prematurely. 2. The preferred way to start Dr K is via a socket message to kdeinit4, but that fails in Apple OS X because kdeinit4 is listening with the wrong socket name. The DISPLAY ID is missing from the end of the name. The fallback is for KCrash to use fork() and exec(), which works, but could cause Dr K to be polluted, depending on the nature of the crash. This deafness of kdeinit4 (and klauncher) could be causing other failures in KDE software in the Apple OS X environment. Diffs - kdeui/util/kcrash.cpp 45eb46b kinit/kinit.cpp e41845a Diff: https://git.reviewboard.kde.org/r/119497/diff/ Testing --- Using Apple OS X 10.7.5 (Lion) on a MacBook Pro, I have installed KDE libs via MacPorts (at version 4.12.5) and I have adapted kdesrc-build to run in an Apple OS X environment and used it to test against the KDE 4.13 branch. I have been testing with a KDE app that I can crash at will and using stderr and Apple OS X Console log output to determine the outcome. Please note that I am the -only- KDE developer who has this kind of setup, but I am NOT a KDE core developer. My experience before now has been in KDE Games. However I used to be a UNIX and database guru before I retired in 1998. I NEED HELP from KDE -core- developers to proceed further. These problems also exist in FRAMEWORKS, but I am as yet unable to build or test Frameworks on Apple OS X. And I am sure there are many more Apple OS X portability problems in KDE software. Without my patches, Dr Konqi is not started and, if it does get past its own crash, KCrash reports: KCrash: Attempting to start /kdedev/kde4.13/kde4/lib/kde4/libexec/drkonqi.app/Contents/MacOS/drkonqi from kdeinit sock_file=/kdedev/kde4.13/home/.kde4.13/socket-Tara.local/kdeinit4__tmp_launch-svPd0L_org.x_0 Warning: connect() failed: : No such file
Re: Update needed to binary compatibility guide for Windows?
On Mon, April 14, 2014 18:28:14 Ian Monroe wrote: On Sun, Apr 13, 2014 at 6:36 PM, Michael Pyne mp...@kde.org wrote: If it's true, do we want to adopt a constraint on our handling of virtual functions in leaf classes based on this? IMO we shouldn't worry about ABI on Windows. And not because meh Windows, but since Microsoft breaks C++ ABI with every compiler release, which is quite frequently these days. In general C++ ABI stability just isn't a thing on Windows. I've looked it up and you're right, they don't even pretend to try to maintain ABI compatibility (instead they recommend using an extern C wrapper or a COM interface). But at the same time we went through a time where GCC seemed to have to fix their C++ ABI every release and we tried to maintain the same binary compatibility standards throughout. I think what I'll do is note the issue. But in fact it may be worse: Do we require that applications never derive from our exported classes? I.e. do we export interfaces (which should not be derived from) or classes (which can be subclassed)? Because if we export classes with virtual methods, and then an application subclasses our class with their own virtual methods, then adding another virtual method to our most derived exported class would break the application even with the GCC ABI. Regards, - Michael Pyne
Update needed to binary compatibility guide for Windows?
Hi all,
In the past couple of days our Binary Compatibility in C++ TechBase page [1]
was posted to Reddit [2].
That post received a response [3] which indicated that we're actually missed a
potential source of binary incompatibility with virtual functions on Windows
with MSVC.
Specifically, that adding an override of an existing virtual function in a
class may cause other vtable function entries to be re-ordered. E.g. in
something like:
class blah
{
public:
virtual void func1(int arg);
virtual void func2(int arg);
virtual void func3(int arg);
};
Adding a virtual override func2(char *arg) to the *end* might cause the
vftable to line up as if declared in this order:
class blah
{
public:
virtual void func1(int arg);
virtual void func2(int arg);
virtual void func2(char *arg);
virtual void func3(int arg); // moved
};
Is anyone able to confirm this behavior on Windows? If it's true, do we want
to adopt a constraint on our handling of virtual functions in leaf classes
based on this? (Adding virtual methods is already not permitted for non-leaf
classes)
Regards,
- Michael Pyne
[1] http://techbase.kde.org/Policies/Binary_Compatibility_Issues_With_C%2B%2B
[2]
http://www.reddit.com/r/cpp/comments/22rpjd/binary_compatibility_issues_with_c/
[3]
http://www.reddit.com/r/cpp/comments/22rpjd/binary_compatibility_issues_with_c/cgq9iyo
Re: Is kpovmodeler still valid or should it be removed
On Wed, April 9, 2014 23:50:15 Burkhard Lück wrote: From my pov I'd like to file a sysadmin request to move these apps to unmaintained4: kpovmodeler kuickshow kgrab I'll +1 the request if you want to go ahead and file it. Regards, - Michael Pyne
Re: Review Request 117157: Unlock session via DBus
On Fri, April 4, 2014 02:20:28 Valentin Rusu wrote: On Sunday, March 30, 2014 05:25:58 PM Michael Pyne wrote: In fact the list of folders and keys present in KWallet (though not their values) can be queried without unlocking KWallet, or even causing it to prompt to unlock. Could you please elaborate more on the possibility to enumerate the keys without opening the wallet? From the KWallet::Wallet API docs: bool Wallet::keyDoesNotExist(...): Determine if an entry in a folder does not exist in a wallet. This does not require decryption of the wallet. This is a handy optimization to avoid prompting the user if your data is certainly not in the wallet. Wallet::folderDoesNotExist() has similar verbiage. enumerating is overstating the case here since there's no direct support for enumerating folders or keys. But all the same, it's not hard at all to brute- force potential folder or key names using the same method used to guess valid Coinbase user identities that just hit the news. Of course if an attacker is running code they'd probably just find it easier to open the .kwl directly and read the folder and key names, since apparently those are stored unencrypted, if the API docs are to be believed. Note that there is a valid use case for this feature: It would be tremendously annoying for a user to have to open their wallet just so an application can verify if it does or does not have an entry stored in the wallet. Instead the application can defer opening the wallet (and forcing the password prompt0 until the value is actually needed. Regards, - Michael Pyne signature.asc Description: This is a digitally signed message part.
Re: Review Request 117157: Unlock session via DBus
On Sat, March 29, 2014 15:25:59 Thiago Macieira wrote: Em sáb 29 mar 2014, às 12:25:48, Martin Gräßlin escreveu: no, the lockscreen is secure. If you are logged in at a tty there is no way to unlock the screen - the only way to bypass the lock is to kill ksmserver which results in the session being killed. You can attach gdb to ksmserver and make it think that the authentication was successful for whichever password was typed. I'll note I've actually done this before, during the development process for the new QML-based screenlocker. The screenlocker at that time would often simply not show the UI for entering the password for whatever reason and leave me completely locked out of the desktop... talk about lame! ;) With that in mind I'd love to have a more official way to tear down the screenlocker from a separate same-user login. If you think being able to unlock a screenlocker is bad security, wait until you figure out that a same- user login can also copy your KWallet passwords out of your running kwalletd if you have it unlocked (something which can be queried over DBus as well). In fact the list of folders and keys present in KWallet (though not their values) can be queried without unlocking KWallet, or even causing it to prompt to unlock. And remember the threat model: Who are we locking the screen again? A physically-present adversary. If they can gain access to a TTY login we are already screwed, especially if they have enough skill to manually hack the qdbus command line invocations needed. Regards, - Michael Pyne
Re: kdesrc-build complains about a dependency cycle in kdelibs..
On Sat, March 15, 2014 21:47:39 Mark Gaiser wrote: Can you try setting up separate builds? Hi Ben, At first i was indeed trying to build both under the same user (but different prefixes and configs). I was expecting that to be likely an issue so i moved to a different user (nothing of frameworks is there) and still the same error.. The mentioned error is from a clean build, clean user and no frameworks and just the default. I can reproduce. Without better debugging on my part to print out what the cycle actually *is* it's hard to be sure. But uncommenting the branch-group latest-qt4 in the sample-rc file does work to break the cycle(s) so I've gone that route for now. Regards, - Michael Pyne
Re: Review Request 115497: Replace SHA with PBKDF2-SHA512+Salt
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115497/#review49315 --- I've had a chance to download the patch and restart the desktop with the patched code. It didn't eat my wallet, which is the best news. ;) I've stared at the code for about an hour now and only saw one other issue (and that's only in the edge case that we can't write a salt file). I think it looks OK so I'd leave the decision to commit up to you, or you can see if valir has other comments. kwalletd/backend/kwalletbackend.cc https://git.reviewboard.kde.org/r/115497/#comment34813 I think this should only be set to KWALLET_VERSION_MINOR if _useNewHash is true; otherwise when we read in this file later we'll try to use the PBKDF2 hash to decrypt it instead of the old hash which will fail. kwalletd/backend/kwalletbackend.cc https://git.reviewboard.kde.org/r/115497/#comment34814 I think that this should be PBKDF2_SHA512_KEYSIZE, strictly speaking. Of course, in this case it turns out to be the exact same number (448 / 8 == 56). Or maybe it's better to warn the future developer when the KEYSIZE macro is defined not to change the PBKDF2 key size without also looking at the Blowfish persist handler. - Michael Pyne On Feb. 7, 2014, 5:39 p.m., Àlex Fiestas wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115497/ --- (Updated Feb. 7, 2014, 5:39 p.m.) Review request for KDE Runtime, Teo Mrnjavac and Valentin Rusu. Repository: kde-runtime Description --- Uses the MINOR_VERSION (which until now it was 0) to upgrade the hash from SHA to PBKDF2-SHA512+salt. I would have loved to completely replace it once the wallet is ported to the new hashing but because of kwalletd code that is not possible without a bigger rewrite. There are 2 reasons for this patch: 1-We avoid using our own implementation of SHA 2-We use a modern hashing technique I'm cooking more patches to use the system user password to open the wallet, we want that password to be hashed using PBKDF2_SHA512 for security reasons. Diffs - CMakeLists.txt 275a6c7 cmake/modules/FindLibGcrypt.cmake PRE-CREATION kwalletd/backend/kwalletbackend.cc e4d461c kwalletd/backend/kwalletbackend.h 83ebf7f kwalletd/backend/backendpersisthandler.cpp bdef6ca kwalletd/backend/CMakeLists.txt 5a5837c Diff: https://git.reviewboard.kde.org/r/115497/diff/ Testing --- Thanks, Àlex Fiestas
Re: Review Request 115497: Replace SHA with PBKDF2-SHA512+Salt
On Feb. 7, 2014, 5:14 a.m., Michael Pyne wrote: kwalletd/backend/kwalletbackend.cc, line 387 https://git.reviewboard.kde.org/r/115497/diff/1-2/?file=242022#file242022line387 Again, might want to add error-checking here. If the salt can't be saved for whatever reason then we don't want to destroy an existing old-style wallet by mistake. It looks like it would be as simple as returning an empty QByteArray if an error is detected. Àlex Fiestas wrote: The check is done in Backend::setPassword, if the resulting salt is empty then we do not use the new hash. In Backend::createAndSaveSalt I'm not sure I can add any extra check besides the return of QIODevice::write, but if that files you have bigger problems... Also from what I see in gcrypt code gcry_random_bytes can't return null, it doesn't seem to have any error reporting. QIODevice::write was the thing I was thinking of. QIODevice::close() too, although it turns out it has no way of returning an error if one occurs. :( You're right that this is a pretty minor edge case, I only brought it up because of how important the data in question is. - Michael --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115497/#review49163 --- On Feb. 7, 2014, 5:39 p.m., Àlex Fiestas wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115497/ --- (Updated Feb. 7, 2014, 5:39 p.m.) Review request for KDE Runtime, Teo Mrnjavac and Valentin Rusu. Repository: kde-runtime Description --- Uses the MINOR_VERSION (which until now it was 0) to upgrade the hash from SHA to PBKDF2-SHA512+salt. I would have loved to completely replace it once the wallet is ported to the new hashing but because of kwalletd code that is not possible without a bigger rewrite. There are 2 reasons for this patch: 1-We avoid using our own implementation of SHA 2-We use a modern hashing technique I'm cooking more patches to use the system user password to open the wallet, we want that password to be hashed using PBKDF2_SHA512 for security reasons. Diffs - CMakeLists.txt 275a6c7 cmake/modules/FindLibGcrypt.cmake PRE-CREATION kwalletd/backend/kwalletbackend.cc e4d461c kwalletd/backend/kwalletbackend.h 83ebf7f kwalletd/backend/backendpersisthandler.cpp bdef6ca kwalletd/backend/CMakeLists.txt 5a5837c Diff: https://git.reviewboard.kde.org/r/115497/diff/ Testing --- Thanks, Àlex Fiestas
Re: Repository rename and consequences
On Sat, February 8, 2014 10:58:23 Ben Cooksley wrote: Once the procedure has been completed, automated build scripts such as kdesrc-build should automatically clone the repositories under their new names - however custom scripts and procedures will need to be adjusted by hand. I would just note that you should still double-check your kdesrc-buildrc to see if you have manually specified kwallet or kwallet-framework (the old names). If that's the case you'd want to remove the manual mention of kwallet. If you need to set options for kwallet (after the move) or kwalletmanager then you should be able to use something like options kwalletmanager # your-options-here end options somewhere in your kdesrc-buildrc (as of kdesrc-build git from last month), and kdesrc-build will use those options once the module shows up. Regards, - Michael Pyne
Re: Review Request 113779: KInfocenter/OpenGL: fix ReadPipe() in the case that the command cannot be run
On Sun, November 10, 2013 11:18:19 Wolfgang Bauer wrote: -Original Message- From: Rolf Eike Beer [mailto:k...@opensource.sf-tec.de] Sent: Sunday, November 10, 2013 9:11 AM To: kde-core-devel@kde.org; Wolfgang Bauer Cc: David Stephen Hubner Subject: Re: Review Request 113779: KInfocenter/OpenGL: fix ReadPipe() in the case that the command cannot be run Can't this be ported to simply use QProcess instead? Eike Well, I only wanted to fix this bug... ;-) But I can have a look at reimplementing this with QProcess as well. How should I proceed? Commit this for now to fix the bug (maybe to the 4.11 branch only)? Or discard this patch and just rewrite ReadPipe() using QProcess? I would recommend applying the patch to 4.11 and master and then investigating whether QProcess would be suitable for master (this might also help with KF5 and Windows porting, not that I expect KInfoCenter to do great things on Windows). Regards, - Michael Pyne signature.asc Description: This is a digitally signed message part.
Re: Review Request 113479: fix kshareddatacache on windows
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113479/#review42533 --- Ship it! Looks good from here, the test you added even has a matching counterpart in the Linux version, so I hope I didn't write the Windows code. ;) - Michael Pyne On Oct. 28, 2013, 10:07 a.m., Sune Vuorela wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113479/ --- (Updated Oct. 28, 2013, 10:07 a.m.) Review request for KDE Frameworks, kdelibs and Michael Pyne. Repository: kdelibs Description --- fix kshareddatacache on windows to at least not be a way to have a bytearray roundtrip. Also, the windows implementation is currently only a in-memory one, so don't test on windows if there is a file written. Diffs - tier1/kcoreaddons/autotests/kshareddatacachetest.cpp a4484560735f9096ecdac26b3c539394602e0f31 tier1/kcoreaddons/src/lib/caching/kshareddatacache_win.cpp cdc6536b56888a615e74960bf1b55fb12cc3e70d Diff: http://git.reviewboard.kde.org/r/113479/diff/ Testing --- Test suite passes Thanks, Sune Vuorela
Re: Review Request 108845: add support for SSSE3 and SSE4.2 in cpufeatures and for msvc
On Sept. 23, 2013, 9:12 a.m., Kevin Ottens wrote: Any reason why this patch isn't committed yet? Please do so ASAP. Kevin Ottens wrote: Any news? I'll close it as discarded if I don't see any activity by next monday. I've integrated it, you don't have to worry about closing this out. Pity we haven't heard from Patrick though, I hope he's OK. - Michael --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108845/#review40505 --- On Oct. 21, 2013, 3:23 p.m., Patrick Spendrin wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108845/ --- (Updated Oct. 21, 2013, 3:23 p.m.) Review request for KDE Frameworks, kdelibs and Patrick von Reth. Repository: kdelibs Description --- This change implements cpu feature checks for MSVC. While at it, I added support SSSE3 and SSE4.2 to the InstructionSets. I took the new values from http://en.wikipedia.org/wiki/CPUID#EAX.3D1:_Processor_Info_and_Feature_Bits . Diffs - tier1/solid/src/solid/processor.h ce4f0e1 tier1/solid/src/solid/backends/shared/cpufeatures.cpp baa1af2 Diff: http://git.reviewboard.kde.org/r/108845/diff/ Testing --- Windows Thanks, Patrick Spendrin
Re: Review Request 113189: lower (a lot) warning noise caused by undefined macros
On Oct. 10, 2013, 11:59 a.m., Bernd Buschinski wrote: This is a bit confussing... lets sum up the background story: - kjs(/wtf) loves gcc features - kjs-cmake filters out (gcc) pedantic now comes the problem... whats with other compilers? if you look at the commit message that caused the warnings: According to CPP standards (defined(X) X) should be the same of just (X). On the other hand it is undefined behavior if 'defined' appears in a macro expansion. gcc -pedantic and icl evaluates them always to false. ( http://quickgit.kde.org/?p=kdelibs.gita=commith=75fa90c16232186bbd287a2ac79839ea34da32c4 ) - it is undefined behavior if 'defined' appears in a macro expansion - So? Whats the correct solution here? And yes, I agree the warnings ARE annoying. But I would like to hear a second opinion here :) IIRC when I was trying to filter out these warnings to debug some Clang issues, the define flags in question are always non-zero if they are defined at all. So it might be possible to replace #if with #ifdef instead. - Michael --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113189/#review41489 --- On Oct. 10, 2013, 10:52 a.m., Jiří Pinkava wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113189/ --- (Updated Oct. 10, 2013, 10:52 a.m.) Review request for kdelibs. Repository: kdelibs Description --- There is a lot of warning noise in kdelibs build, this decreases number of warnings a lot. I have 'git grep'ped the code and checked all usages, and found no problem. Tested by compile/usage. Diffs - kjs/wtf/Platform.h 843cfd2 Diff: http://git.reviewboard.kde.org/r/113189/diff/ Testing --- Thanks, Jiří Pinkava
Re: kde-workspace master becomes Qt5-based
On Wed, September 25, 2013 13:55:57 Michael Pyne wrote: On Thursday, September 19, 2013 19:24:51 Michael Pyne wrote: On Thu, September 19, 2013 19:30:52 Sebastian Kügler wrote: We're planning to merge the frameworks-scratch branch of kde-workspace into master next Monday. That means if you're building your Plasma yourself from Git (and you're not yet ready for Plasma2 ;)), you should switch to the KDE/4.11 branch. The build will fail otherwise, so you will notice. This was a public service announcement. (And you can ask questions here.) Please don't forget to update kde-build-metadata! I suppose I should go and update the sample kdesrc-buildrc. That would be nice. I was asked how to have kdesrc-build build and install kde-workspace now, and didn't know the answer. As far as I can see, the metadata is correct. I suppose people's kdesrc-buildrc needs updating, but how? It should be as easy as using the branch-group option (instead of branch) for the modules to be built, in a kde-projects module set. Something like: module-set repository kde-projects use-modules kdelibs kde-workspace kde-runtime branch-group kf5-qt5 # or latest-qt4 or stable-qt4 end module-set I've just checked and kdesrc-build seems to pick up your change in kde-build- metadata just fine for kde-workspace. Regards, - Michael Pyne signature.asc Description: This is a digitally signed message part.
Re: kde-workspace master becomes Qt5-based
On Thu, September 19, 2013 19:30:52 Sebastian Kügler wrote: Hi all, We're planning to merge the frameworks-scratch branch of kde-workspace into master next Monday. That means if you're building your Plasma yourself from Git (and you're not yet ready for Plasma2 ;)), you should switch to the KDE/4.11 branch. The build will fail otherwise, so you will notice. This was a public service announcement. (And you can ask questions here.) Please don't forget to update kde-build-metadata! ;) I suppose I should go and update the sample kdesrc-buildrc. Regards, - Michael Pyne signature.asc Description: This is a digitally signed message part.
Re: Review Request 112458: Fix build with jpeg-9
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/112458/#review39389
---
Ship it!
Change makes sense, tested here and works, please commit.
- Michael Pyne
On Sept. 4, 2013, 5:49 a.m., Michael Palimaka wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/112458/
---
(Updated Sept. 4, 2013, 5:49 a.m.)
Review request for kdelibs.
Description
---
Since jpeg-9, build fails: khtml/imload/decoders/jpegloader.cpp:145:20:
error: cannot convert ‘bool’ to ‘boolean’ in return
This is because in jpeg-9, the boolean typedef changes from int to typedef
enum { FALSE = 0, TRUE = 1 }.
Diffs
-
khtml/imload/decoders/jpegloader.cpp
ea255726fdf1c310826c0037583aca3c5c741245
Diff: http://git.reviewboard.kde.org/r/112458/diff/
Testing
---
Also tested against jpeg-8d (previous stable) and libjpeg-turbo.
Thanks,
Michael Palimaka
Re: Review Request 112367: Include the icon's theme in the cache key
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112367/#review38968 --- Ship it! Well, why not commit it now, and if it ends up slow we can revert it later? We can at least be correct in KF5 while we wait for the rest of the frameworks to catch up. :) - Michael Pyne On Aug. 29, 2013, 8 p.m., Àlex Fiestas wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112367/ --- (Updated Aug. 29, 2013, 8 p.m.) Review request for KDE Frameworks, kdelibs and Michael Pyne. Description --- If we make no difference between an icon from oxygen and an icon from gnome, when we change themes for example from oxygen to gnome, the icon will be found in the cache and won't be upgraded. Diffs - kdeui/icons/kiconloader.cpp ce6aeea Diff: http://git.reviewboard.kde.org/r/112367/diff/ Testing --- Thanks, Àlex Fiestas
Re: Review Request 112367: Include the icon's theme in the cache key
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112367/#review38906 --- The change seems reasonable (but keep in mind it makes one of the hottest methods in KIconLoader that much slower). However I thought the KCM for the Desktop Theme Appearance already reset the cache as suggested by Aleix. Is this for situations where the theme is somehow changed via other means (such as direct edits to the rc files)? - Michael Pyne On Aug. 29, 2013, 8 p.m., Àlex Fiestas wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112367/ --- (Updated Aug. 29, 2013, 8 p.m.) Review request for KDE Frameworks, kdelibs and Michael Pyne. Description --- If we make no difference between an icon from oxygen and an icon from gnome, when we change themes for example from oxygen to gnome, the icon will be found in the cache and won't be upgraded. Diffs - kdeui/icons/kiconloader.cpp ce6aeea Diff: http://git.reviewboard.kde.org/r/112367/diff/ Testing --- Thanks, Àlex Fiestas
Update on git repository logical branch module groups
Hi all, (please keep discussion on core-devel) A couple of weeks ago at Akademy a decision was reached to allow for the changing of the kde-workspace development branch for KF5 efforts to be 'master' (instead of 'frameworks' as in kdelibs). In exchange we would implement a means for users to give a high-level description of what branch they want in general. That is, a way to say that in general, you just want a stable KDE 4, development KDE 4, or bleeding-edge Qt5/KF5-based desktop. I'd like to announce that the groundwork for that system is now in place. There is a file in the kde-build-metadata repository, logical-module- structure, which contains a high-level overview of which branch to use for the KDE Project git repositories, for each of the various overarching groups. Right now there are 3 such groups, also listed in that file. kde-build-metadata also has a sample Python script (in the tools/ subdir) which can be used to verify the file remains valid JSON, and to see what git- branch would be used for a given module and group based on the rules saved into the file. Please use this before committing changes to the file to ensure that things are as you were expecting. ;) kdesrc-build can now also support this data (I still need to add the docs though). The short story is to use the branch-group option to name one of the groups listed in the JSON file, instead of the branch option (if both are set then branch will take precedence, though I'll have to double-check I actually implemented it that way). branch-group obeys normal option precedence semantics within kdesrc-build, so you can set it globally and then override it within a module or module-set. Just remember that if the JSON file doesn't have any rules for a module that the branch 'master' will be assumed. So you could do something like: global branch-group stable-qt4 ... end global module-set reqs repository kde-projects use-modules qt soprano attica cagibi # etc... end module-set # List all desired modules common to KDE 4/KF5 in one file include common-kde-modules.ksbrc and then have a KF5 kdesrc-buildrc with something similar but just change out the branch-group. I intend on having kdesrc-build (and possibly the Python script too) warn about branch-groups that are not actually listed within the layers list of the JSON file as a backup against speling errors. It doesn't do this yet, however. But please make sure that if you decide to add a fourth group or rename the existing ones, that you've updated the layers list in the JSON as well. I'd like for people (especially those working on frameworks or just trying to huddle to KDE 4) to go ahead and test this. On or around the __19th__ I'd like to bake in whatever we consider the names of logical groups to be, so that they are not modified further without discussion on kde-core-devel. (Think of that as a backward-compat concern). I hope this all makes sense, if the feature seems to be evolving in a way other than desired during the discussion at Akademy please let me know. You can find some further documentation on the Community Wiki: http://community.kde.org/Infrastructure/Project_Metadata. Regards, - Michael Pyne signature.asc Description: This is a digitally signed message part.
Re: Proposal for branching policy towards KF5
On Mon, July 29, 2013 22:09:38 Aurélien Gâteau wrote: Le mercredi 24 juillet 2013 23:05:55 Michael Pyne a écrit : I didn't want to write another parser, but JSON has no native comment support, so the documentation [1] is on community.kde.org (though perhaps that's for the best). Slightly off-topic: I probably arrive a bit late, but have you considered using YAML instead of JSON? YAML is as light to write (if not lighter) than JSON, widespread enough to have parsers in many languages and does support comments. I won't claim to have seriously considered it, but it seems we're using JSON elsewhere already, and Python at least appears to support it by default on all of the hosts where JSON may be necessary (e.g. l10n servers, build.kde.org, etc.). The logic of the existing scripts is not very tied to the data interchange format so it's not like migrating to YAML would be traumatic at this point, but I don't think comments alone would warrant switching either now that we have some small amount of code supporting it. And also, who wants to have to worry about code execution when you're just trying to parse a data interchange format? ;) Regards, - Michael Pyne signature.asc Description: This is a digitally signed message part.
Re: Proposal for branching policy towards KF5
On Fri, July 26, 2013 21:11:21 Torgny Nyblom wrote: On Thursday 25 July 2013 18.24.50 Michael Pyne wrote: The 'logical module groups' might play a role in the release process after a release is done, but shouldn't have any further role for tagging that I can see. i18n is covered above. Wouldn't this be nice to have as the source of branch info for the releases? One place to have this information instead of duplicate it for each tool/user? I think I see what you're talking about, but the release team essentially just make their own branch already, make their tags, that's it. Things are generally not tagged directly from master or any other development branch. It's true that our proposed 'stable-qt4' branch would generally be hooked up to a release branch, so there's no theoretical reason why we couldn't later add released branches for the SC/Platform/*Desktop. But I think it would be easier to use sane and consistent branch naming policy for release purposes, with any exceptions clearly known. Even with kde-workspace, there's no technical reason I can see why we can't have both KDE/4.11 and KDE/4.12 (when we get to that point) pointed to the same commit once we're in LTS mode on that, if we're worried about people just trying to checkout KDE/4.12 for everything. Are there tools that have to maintain this data for themselves though? I do agree with the idea of centralizing it so if that's an issue I say let's see what the impl would look like and see if that works out. Regards, - Michael Pyne signature.asc Description: This is a digitally signed message part.
Re: Proposal for branching policy towards KF5
On Thu, July 25, 2013 22:44:47 Albert Astals Cid wrote: El Dimecres, 24 de juliol de 2013, a les 23:05:55, Michael Pyne va escriure: On Fri, July 19, 2013 00:21:21 you wrote: After more live discussion with Sebas and Marco plus Aaron over a video chat, we came up with the following setup for the workspace repos (*) : - the development branch for their next feature release (based on Qt5/KF5) will be master. - *before* this happens, however, kdesrc-build / kde-build-metadata / projects.kde.org will need to be improved so that tools (kdesrc-build and possibly build.kde.org) can automatically select the latest Qt4-based branch (i.e. master everywhere and 4.11 for the workspace repos), on demand. This would also be the opportunity to implement latest *stable* branch which is 4.11 for most modules right now, but could be at some point 4.12 for most and 4.11 for workspace repos. Adding a similar generic selection for qt5/kf5, we would end up giving 3 options to people who compile from sources: stable, latest-qt4, or qt5/kf5- based. Back on topic, I have made an initial draft specification [1] for what this logical module group layer would look like. In addition, there is a sample JSON file in the kde-build-metadata git repository, called logical-module-structure that one can view to get a feel for the proposed syntax/semantics. snip A point of concern is that currently we already have a concept similar to this, for i18n. It's possible to specify in the projects.kde.org management interface a stable or trunk branch for translation purposes. I don't know the translation infrastructure well enough to see how this proposal would impact that feature; I assume we'd want to move scripty ( friends) over to using this at some point if we go through with it, but it's probably easy enough to keep both techniques on whatever release checklist we're using now. [I18N_HAT] I'd appreciate if you guys decide on something :D Not so long ago we had to write support for the projects.kde.org branches thing when we already had our nice set of scripts and now you say we'll have to build support for something different? It's good that we never removed our internal scripts and they are the authoritative source, that way removing the projects.kde.org support is trivial :D [/I18N_HAT] Well it would certainly be possible to *keep* support for whatever you're doing now with projects.kde.org, that isn't going away at this point. But since the concept is complementary, it would make sense to try to end up on one solution. At least this way it should be easier to fixup the branches for groups of modules at a time! ;) I'm not familiar with the i18n scripts at this point but I would volunteer to help with any needed porting. At this point I think this still needs a green light from the release team, though. They are now CC'ed for review. One clarification I should make is that I also received a recommendation to investigate migrating our current dependency data into this new JSON file if possible. You mean something like kde-build-metadata? Neither i18n nor releasing uses that file. Kind of (dependency data is one part of kde-build-metadata). It is true that neither requires dependency info. In the event, some prototyping of the result of making *all* of our deps go in the file is rather unsatisfactory so I'll be dropping that for now (the hard work is already done in case we decide to investigate later though). Basically from release I don't see how that affects us, we use the data from the release-tools module that is de-coupled from what you mention, no? dependency-data would not affect you at all. The 'logical module groups' might play a role in the release process after a release is done, but shouldn't have any further role for tagging that I can see. i18n is covered above. Regards, - Michael Pyne signature.asc Description: This is a digitally signed message part.
Re: Proposal for branching policy towards KF5
On Fri, July 19, 2013 00:21:21 you wrote: After more live discussion with Sebas and Marco plus Aaron over a video chat, we came up with the following setup for the workspace repos (*) : - the development branch for their next feature release (based on Qt5/KF5) will be master. - *before* this happens, however, kdesrc-build / kde-build-metadata / projects.kde.org will need to be improved so that tools (kdesrc-build and possibly build.kde.org) can automatically select the latest Qt4-based branch (i.e. master everywhere and 4.11 for the workspace repos), on demand. This would also be the opportunity to implement latest *stable* branch which is 4.11 for most modules right now, but could be at some point 4.12 for most and 4.11 for workspace repos. Adding a similar generic selection for qt5/kf5, we would end up giving 3 options to people who compile from sources: stable, latest-qt4, or qt5/kf5- based. First note: There's a lot of different mailing lists with at least some interest in this discussion, so I've mailed them all for informational purposes... but let's keep the discussion limited to the kde-core-devel mailing list! Back on topic, I have made an initial draft specification [1] for what this logical module group layer would look like. In addition, there is a sample JSON file in the kde-build-metadata git repository, called logical-module-structure that one can view to get a feel for the proposed syntax/semantics. I didn't want to write another parser, but JSON has no native comment support, so the documentation [1] is on community.kde.org (though perhaps that's for the best). For those with no clue what I'm talking about, the original thread from kde- core-devel is available from [2]. A point of concern is that currently we already have a concept similar to this, for i18n. It's possible to specify in the projects.kde.org management interface a stable or trunk branch for translation purposes. I don't know the translation infrastructure well enough to see how this proposal would impact that feature; I assume we'd want to move scripty ( friends) over to using this at some point if we go through with it, but it's probably easy enough to keep both techniques on whatever release checklist we're using now. At this point I think this still needs a green light from the release team, though. They are now CC'ed for review. One clarification I should make is that I also received a recommendation to investigate migrating our current dependency data into this new JSON file if possible. I put the effort into doing this as it would also help make the implementation of some kdesrc-build misfeatures relating to dependency-data additions a bit easier, as there's no need to construct an AST and a parser. Additionally it would permit 'soft' dependencies, which are useful for modules that can utilize optional features but don't have required dependencies on other git modules. However that can, and probably should, be considered separately (though I'll take comments now, if you have them). [1]: http://community.kde.org/Infrastructure/Project_Metadata [2]: http://markmail.org/message/4l3yqerga7mfiit5 Anyways, thanks for your interest and please let me know if this will work to solve the problem at hand. If so I will start on integrating within kdesrc- build, and working with the sysadmins to support within the continuous integration infrastructure. Regards, - Michael Pyne signature.asc Description: This is a digitally signed message part.
Re: Supported Compilers / C++11 Support in KF5
On Mon, July 22, 2013 08:38:12 Johannes Sixt wrote: Am 7/21/2013 13:52, schrieb Rolf Eike Beer: Explicit virtual overrides require g++ 4.7: http://gcc.gnu.org/projects/cxx0x.html This is trivially to work around by a CMake time check and then just define override to empty. Not so. 'override' is a legal identifier, not a pure keyword. By defining it away, you break existing code that uses the name for something else. That's a great point (something I just came across in Stroustroup's 4th Edition of TC++PL the other day). 'override' and 'final' are both what are described as contextual keywords due to backward compatibility concerns. This means it's not safe to use pre-processor macros to expand either of those to be empty as they can be used in normal code sections (even though that's probably a bad idea at this point). Regards, - Michael Pyne signature.asc Description: This is a digitally signed message part.
Re: Proposal for branching policy towards KF5
On Fri, July 19, 2013 00:21:21 David Faure wrote: After more live discussion with Sebas and Marco plus Aaron over a video chat, we came up with the following setup for the workspace repos (*) : Adding a similar generic selection for qt5/kf5, we would end up giving 3 options to people who compile from sources: stable, latest-qt4, or qt5/kf5- based. I see this as kind of layer on top of the actual branch names (an indirection, in other words). This gives us consistency for everyone: * for contributors to a specific module, master is where new features should go * for people who compile many modules that are supposed to be working together, a logical selection layer can be selected, stable, latest-qt4, or qt5-kf5. implementation Michael: I see two ways this could be done in kdesrc-build. Either with the selection layers being defined by the projects XML and some additional magic in branch selection to allow for these new names, or with a much more low-tech solution: 3 available files to include from kdesrc-buildrc, like we did with kf5-qt5-build-include, except that these files would only contain module- options (yet another reason for that feature to exist), not actual module definitions. The user's kdesrc-buildrc would still define which module he/she wants to build, but the included file would define which branch should be used for each module (unless overriden, of course). /implementation Well there's a 3rd method as well, which is to add a separate metadata file to the kde-build-metadata repository which maps each git repository to its appropriate branch for each of the 3 categories. This is a labor-intensive task, but it's easy to centrally-manage without having to rely on many different module maintainers or core developers for each git repository to update their own metadata in projects.kde.org (assuming we can get the sysadmins to add that). In addition this would probably be easier to support for build.kde.org and the other non-kdesrc-build tools that use the current information in kde-build- metadata. We could probably significantly reduce the labor involved in maintenance by setting up defaults and then only specifying exceptions from the rule. The question will be how to group modules together. This is definitely something that would need discussion with the sysadmins and Release Team to hash out the details and any process changes that might need to happen too (e.g. adding a tool to verify all modules in kde/* are actually described for each of the 3 categories and running before a release to make sure new modules are not inadvertently left out of the metadata). I think we're getting to the point where it would be a good idea to document the metadata on our TechBase as well instead of just a README embedded within the directory. By a strange coincidence I happen to be on vacation leave so I will try to be online to discuss on IRC, but failing that we can setup a separate thread on the relevant mailing lists to hash out details. Regards, - Michael Pyne signature.asc Description: This is a digitally signed message part.
Re: KDE/4.11 branched what to do with kde-workspace?
On Mon, July 15, 2013 12:27:53 Aaron J. Seigo wrote: On Saturday, July 13, 2013 23:36:24 David Faure wrote: * let master be the branch that will become 4.12, even if you (= the workspace developers) don't intend to do more than bugfixing on 4.x, there will always be a need to separate fixes that can go into the next bugfix release and fixes that need a new i18n, or smallish features contributed externally. We want to do a long term release of 4.11. Something that doesn’t have new features or new i18n, but which just gets boring old bug fixes and translations. Doing a 4.12 / 4.13 makes that infeasible: it would mean continuing to track the 4.11 branch *and* master *and* the Qt5 branch *and* the eventual 4.12/4.13 branches. That is a recipe for disastrous neglect. So it sounds like the issue in this case is that kde-workspace's KDE 4 bugfixing will be concentrated in the 4.11 branch lineage? While that is fine, I don't see how that is different in practice from doing 4.12/13 development in a bugfix-only state, except for the version numbers. Perhaps this is more semantically clear though (the reason it's still called 4.11.y is that there are still no new features)? Please, let us focus on 4.11 and Qt5. We can manage that. I resent people telling us we can do more than we know we can. I resent people interpreting a given comment in the most-negative possible light. :) Unless *I'm* the one misinterpreting things it appears you both are suggesting to maintain 1 KDE 4 branch at a time and do development in Qt5/KF5, and that the only point of disagreement here is what to call the KDE 4 development. The *actual* point of disagreement is later in your email. It’s the merge from our Qt5 devel back into master and the branch management between 4.11, master and frameworks. If you want us to learn from the kdelibs experience, there it is! It seems this is the point of concern then, no? By branching off the eventual Plasma Workspace 2 from KDE/4.11 we can 'gracefully' change over to Qt5/KF5, and since that development will happen in master there will be no icky problem merges *back* to master when it comes time to do a cut-over. Is that the idea? An alternative would be to still do development in 'frameworks' but have frequent curated merges back to master (keeping with the idea of always- summer), but you would still need to reserve master for the use of development and so it would still not be suitable for LTS KDE/4.x. I don't see the latter as being useful now (assuming we are pushing frameworks development into master) since those worried about Qt5/KF5 will be having to track the bleeding-edge anyways. But it might be useful when we get closer to thinking about preview and alpha releases for more people to be able to start banging on Qt5/KF5 without it breaking every other day. version, but still usable. That's what everyone building KDE from sources expects, including all current scripts (kdesrc-build, build-tool, jenkins, custom tools, people's heads, etc. etc.). Don’t I can’t support custom tools or people’s heads, obviously, other than to communicate. For tools that use projects.kde.org, however, perhaps we can define the “current default branch” there? That should be possible in the XML. A variant of that idea is already used for i18n, and kdesrc-build supports that already via the use-stable-kde option (which repeats the scheme kdesrc-build used during the KDE 3-4 transition). The definition is of the current *stable* branch though, if we want *default* branch to mean something different then we might need to add that to the XML metadata so the tools can use it where appropriate. I will support whatever. Would it perhaps be useful to have in kdesrc-build a 'news to the users' feature that developers could tie into? Most of the time when we made a breaking change on the KDE side I try to fix it up for the users within kdesrc-build, but I don't see a way to assume that a user building kde- workspace master isn't actually doing it on purpose. Even if I check the kdelibs version they build, they could be using a self-built KF5. Regards, - Michael Pyne signature.asc Description: This is a digitally signed message part.
Re: KDE/4.11 branched what to do with kde-workspace?
On Mon, July 15, 2013 10:46:49 Aaron J. Seigo wrote: On Friday, July 12, 2013 17:48:50 David Jarvie wrote: best of intentions) for old habits to take over and to accidentally use master again. This happened to me more than once. So how can we make that clearer for people and hard for this sort of mistake to be made? Here’s a possible idea: we could make the build fail (along with a relevant message on console) unless a specific env var or a cmake option is passed. If it doesn’t build, you probably won’t end up using it, and hopefullya build failure gets noticed. That (or something like it) would be wonderful. Even where the tool *wants* to support Doing the Right Thing I have taken a dim view at rewriting config files for the users. However I can have the build failure cause kdesrc-build to display a specific warning to the users that they have to adjust their config, if you want to coordinate something like that I'd be happy to put it in. Regards, - Michael Pyne signature.asc Description: This is a digitally signed message part.
Re: KDE/4.11 branched what to do with kde-workspace?
On Sat, July 13, 2013 23:36:24 David Faure wrote: No changes to anyone's workflow, including translators, testers, powerusers, distros, kde-wide bugfixers, etc. The only ones who have to switch to another branch is the people switching to a qt5/frameworks based development, and these people have a lot more setup to do anyway, the git checkout frameworks is 1% of it all. I can confirm that the time when kdelibs was blocked to a certain branch was more problematic for kdesrc-build users (and by extension, myself). In any event the branch 'master' should mean something semantically. At this point that means either 4.x or the Qt5/KF5 code that's coming up. It seems a bit early to declare that Qt5/KF5 will be the answer during the 4.12 timeframe, so that would tend to argue against making master target Qt5/KF5. Bugfixes and other minor fixes during the 4.12 (13, 14...) timeframe will still need to be committed somewhere (presumably somewhere else besides KDE/4.11) and 'master' would need to mean something during that time. So why not leave it the way it is now (master makes sense, 4.x bugfixes have a working dev branch), until we're ready to cut over Qt5/KF5 to be the new 'master'? Regards, - Michael Pyne signature.asc Description: This is a digitally signed message part.
Re: Releases in 3 months
On Mon, July 8, 2013 17:45:10 Philip Muskovac wrote: What would at least make my life easier here would be a way to easily get a list of all patches that were applied to a stable release (esp. when someone bothers to backport a fix after the last point release is out). The only way to do that, that I found so far, is filtering out mails from kde- commits, which is neither very easy nor reliable. I'm assuming a variant of 'git log --oneline v4.x.y..KDE/4.x' is not adequate for some reason? Admittedly sometimes we need to be reminded to push the release tags but that seems like it should work for all modules in the kde.org-released software. Regards, - Michael Pyne signature.asc Description: This is a digitally signed message part.
Re: Review Request 111390: kshorturifilter: inverted condition in home directory handling
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/111390/#review35587
---
Ship it!
Change looks good to me, please commit.
- Michael Pyne
On July 4, 2013, 1:45 p.m., Jonathan Marten wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/111390/
---
(Updated July 4, 2013, 1:45 p.m.)
Review request for KDE Runtime.
Description
---
While investigating URI filtering/validation elsewhere I spotted some odd
logic here. The original line in question seems to be saying:
if (user is valid user's home directory is empty)
{
// replace from '~' onwards with home directory
}
else
{
// generate an error
}
The second condition, though, is surely wrong. It should say:
if (user is valid user's home directory is NOT empty)
...
Diffs
-
kurifilter-plugins/shorturi/kshorturifilter.cpp d27b018
Diff: http://git.reviewboard.kde.org/r/111390/diff/
Testing
---
Build kderumtime with this change. Checked correct results for '~' and
'~user', where the user both exists and does not, in konqueror and krunner.
Ran the kurifiltertest with all passes.
Thanks,
Jonathan Marten
Re: Review Request 111261: [KDE-Workspace]: Possible NULL ptr. deref. in KDM and KCheckPass
On June 29, 2013, 8:31 p.m., Commit Hook wrote: This review has been submitted with commit 45b7f137fbc0b942fd2c9b4e8d8c1f0293e64ba7 by Michael Pyne to branch KDE/4.10. I have also pushed to master to fix for KDE 4.11. Commit 194da6154375fc8103b8c4e29e385cd7ae2e. Thanks for the notification and the patch Mancha! - Michael --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111261/#review35291 --- On June 29, 2013, 8:31 p.m., mr. mancha wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111261/ --- (Updated June 29, 2013, 8:31 p.m.) Review request for kde-workspace. Description --- Background: Beginning with glibc 2.17 (eglibc 2.17), crypt() fails with EINVAL (w/ NULL return) if the salt violates specifications. Additionally, on FIPS-140 enabled Linux systems, DES or MD5 encrypted passwords passed to crypt() fail with EPERM (w/ NULL return). Description: If KDM uses raw crypt() authentication (or pw_encrypt() on a patched Shadow system; see: https://alioth.debian.org/tracker/index.php?func=detailaid=314234 ), instead of higher-level authentication such as PAM, and that crypt() can return a NULL pointer (as glibc 2.17+ does when passed a DES/MD5 encrypted passwords on Linux systems in FIPS-140 mode), then attempting to login to such an account via KDM crashes the daemon. - kdm[1879]: segfault at 0 ip b74a1909 sp bfd209d4 error 4 in libc-2.17.so[b7421000+186000] kdm[1841]: Unknown session exit code 0 (sig 11) from manager process - Likewise, KCheckPass, when called in a similar scenario as KDM above, or when attempting to pass invalid input to crypt()/pw_encrypt() such as a locked account that has a ! prepended in the password field, will crash. - kcheckpass[1927]: segfault at 0 ip b762b910 sp bffb0494 error 4 in libc-2.17.so[b75ab000+186000] - Note: an earlier (and buggy) patch was emailed directly to ML (not via RR). Please disregard that submission entirely. Diffs - kcheckpass/checkpass_etcpasswd.c 1dbe06f kcheckpass/checkpass_osfc2passwd.c 9a074f9 kcheckpass/checkpass_shadow.c ec3a4e0 kdm/backend/client.c bdff6da Diff: http://git.reviewboard.kde.org/r/111261/diff/ Testing --- Tests conducted on KDE-Workspace 4.10.4 confirm attached patch corrects the issues described above. Before applying the patch, KDM and KCheckPass segfault as shown in log snippets above. After applying the patch, both properly handle NULL returns from crypt() and pw_encrypt(). Thanks, mr. mancha
Re: Review Request 110367: Add JoinTheGame menu entry
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110367/#review32763 --- This caused an unhandled switch warning in kdoctools/genshortcutents.cpp, which can be trivially fixed with: diff --git a/kdoctools/genshortcutents.cpp b/kdoctools/genshortcutents.cpp index b35b5a4..92bdea5 100644 --- a/kdoctools/genshortcutents.cpp +++ b/kdoctools/genshortcutents.cpp @@ -300,6 +300,9 @@ static QString entityForAccel( KStandardShortcut::StandardShortcut accel ) case KStandardShortcut::AboutKDE: markup += AboutKDE; break; + case KStandardShortcut::JoinTheGame: + markup += JoinTheGame; + break; case KStandardShortcut::StandardShortcutCount: break; } however I'm not sure if there is anything else needed for the doc tools. This additional patch allowed kdelibs docs to build just fine so I don't think there are additional XML schemas to modify, however it might be prudent to check. --- As a separate point though, I must admit to being nervous about the idea of integrating this into *every* KDE-using app's UI, including applications that have no affiliation with KDE whatsoever other than using kdelibs. How would we as devs/GNU-users react if the default bash prompt contained a notice on how to fund the FSF? Additionally even if we assume that users of KDE software wish to fund contributor meetings, necessary infrastructure, and the future security of KDE software, it doesn't necessarily follow that they wish to e.g. raise awareness for Free Culture. In any event I would recommend avoiding baking in a donation amount or method in the U/I, it may change in the future. I don't want to splash cold water on the idea because I think this kind of promo activity can be very useful, but I don't think this is the right format. I would recommend a separate app or Plasmoid that can be run with a way to link it from the Plasma desktop (which *does* belong to KDE), preferably not quite as obtrusively. We're currently riding a relative wave of positive karma with FOSS users for the first time in years and I don't want to see KDE exposed to the same reaction that we've seen with Ubuntu's Amazon gem or earlier with KDE 4.0. I've grown fond of not having flamewars about cashews or JRTs and I wish to see that continue if possible. :) kdeui/dialogs/kjointhegamedialog_p.cpp http://git.reviewboard.kde.org/r/110367/#comment24335 activities do so is wrong, I'm not sure what exactly is meant though. - Michael Pyne On May 9, 2013, 11:05 p.m., Albert Astals Cid wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110367/ --- (Updated May 9, 2013, 11:05 p.m.) Review request for kdelibs. Description --- Patch by Pau ages ago, Lydia wants to push for it again. Note that i still find it ugly, but i'm not the kdeui maintainer, so i'll leave it to someone else to discuss. Not sure if the StandardAction enum addition should be at the end in case someone is storing those enums in a file or something. Diffs - kdecore/doc/README.kiosk b95002d kdeui/CMakeLists.txt b439e04 kdeui/actions/kstandardaction.h 3064301 kdeui/actions/kstandardaction.cpp 7de0c6f kdeui/actions/kstandardaction_p.h b8f8df1 kdeui/dialogs/jointhegame.png PRE-CREATION kdeui/dialogs/kaboutkdedialog_p.cpp b9728bf kdeui/dialogs/kjointhegamedialog_p.h PRE-CREATION kdeui/dialogs/kjointhegamedialog_p.cpp PRE-CREATION kdeui/shortcuts/kstandardshortcut.h b31e45c kdeui/shortcuts/kstandardshortcut.cpp 669f750 kdeui/widgets/khelpmenu.h 3389068 kdeui/widgets/khelpmenu.cpp f547c46 kdeui/xmlgui/ui_standards.rc a0f5bed Diff: http://git.reviewboard.kde.org/r/110367/diff/ Testing --- Ran okular, saw the new menu entry, it did stuff. Ran ksnapshot (that doesn't use xmlgui), saw the new menu entry, it did the same stuff. Thanks, Albert Astals Cid
Re: filter unwated cflags? (-ffast-math)
On Tue, April 16, 2013 20:29:25 Albert Astals Cid wrote: El Dimarts, 16 d'abril de 2013, a les 16:39:32, Bernd Buschinski va escriure: hi, Hi I am not sure how do deal with unwanted cflags(cxxflags), so I ask here :) For kjs for example -ffast-math is known to cause problems, as we stricly rely on IEEE compatible floating point, NaN, Inf, signed Zero and all the stuff. So the question should I filter them? or leave it to the user to optimize(break) his system? Well, if you know it'll break, you should shout. If that shouting means just a huge message or stopping the compilation i am not sure, but if it'll give you extra work for no reason in forme of bugs, etc. i'd go for stopping the compilation. Having run into this before, I can only +1 this about 1000 times or so. KJS with -ffast-math and friends is unsupportable so there's no reason to let the compile go on if we can detect it. That code should be considered completely miscompiled, if a user wants to use the flag anyways they can take it on themselves to figure out CMake in addition. Regards, - Michael Pyne signature.asc Description: This is a digitally signed message part.
Re: Review Request 109998: kjs: Fix CanvasImageData eating the color value by doing wrong alpha premultiply
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109998/#review31004 --- At this point you're probably better suited to review KHTML and KJS than many of the rest of the KDE devs, but I would say in general that it is more important that the code be correct than to be optimized, so I would say to Ship It based on the problem description, if no one else with KHTML/KJS experience is able to review. I agree that it's very difficult to un-premultiply the colors once you have an alpha of 0 introduced. Even without an alpha of 0, there is probably precision errors introduced with repeated changes to the color attributes the way we have it now, which would be a separate bug. - Michael Pyne On April 13, 2013, 3:34 p.m., Bernd Buschinski wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109998/ --- (Updated April 13, 2013, 3:34 p.m.) Review request for kdelibs. Description --- previous knowledge: - the CanvasImageData allows setting the single color red/green/blue/alpha-values for each pixel - the default for all pixel is Rgba 0,0,0,0 and must not be changed - you can set the color-rgba values in any order, there is no fixed order the bug: We use QImage::Format_ARGB32_Premultiplied beacuse it faster for many operations. As we directly set the red/green/blue/alpha color value via QImage::scanLine, we must premultiply it, to keep it in the QImage::Format_ARGB32_Premultiplied format. But as CanvasImageData does not specify any order of setting the values or does not know what values are set, we might end up doing a wrong premultiply. For premultiply we MUST have the correct alpha value, if it is not set it is assumed 0 ( the default), but if we premultiply a given (for example) red value with 0, its get zero, the red color value is gone. Gone and no way to recover it. So for example if we set for each pixel (in this order) red=255,green=0,blue=0,alpha=255, we get a black image instead of a red one. my solution: As I don't want to introduce a is set check for the alpha value, I suggest we use ARG32 instead of ARGB32_Premultiplied. This allows us to set the color values in any order we want. possible problems: ARG32 is slower than ARGB32_Premultiplied. Yes, but as far as I can see we do not display the image directly, it is only used as in QPainter::setCompositionMode(QPainter::CompositionMode_Source); QPainter::drawImage(x,y,ourImage); to get painted on another image. I hope QPainter uses some sse magic, so the paint is faster than our custom premultiply. btw: While (afaik) it is possible to re-use that not-displayed ImageData, I only found javascript code creating new ImageData for each animation-frame. In this case ARGB32_Premultiplied would be more performant. Diffs - khtml/html/html_canvasimpl.cpp c7fce27 Diff: http://git.reviewboard.kde.org/r/109998/diff/ Testing --- http://www.w3schools.com/tags/canvas_createimagedata.asp shows the problem, as it shows a black image but it should be red. Thanks, Bernd Buschinski
Re: Password strengh meter in KNewPasswordDialog
On Wednesday, April 03, 2013 18:47:17 Cristian Tibirna wrote: On Wednesday 03 April 2013 22:39:47 Rolf Eike Beer wrote: Hi all, http://xkcd.com/936/ In fairness, common dictionary words (no matter how long) have less entropy than you would get just from adding the letters. Each word can simply be considered a letter in a larger alphabet. E.g. a 4-word long password from within the 500 most common words is one of only 6.25e10 possibilities. So I'd use dictionary words as a supplement to other means, not by itself. The authors of JohnTheRipper surely read XKCD just as we do. :) so a password containing only lowercase characters and numbers needs to be much longer than one also containing specials and uppercase characters. Really, this whole can be short because has mixed types of characters nonsense has to die. There is a math theory behind password strength. There might even be libraries capable of measuring this properly. Completely agreed. If anything it seems that even the idea of password entropy might not apply to any passwords that a human generates [1]. In such a scenario it may be best to simply correlate password strength loosely with password length. [1] http://reusablesec.blogspot.com/2010/10/new-paper-on-password-security-metrics.html Regards, - Michael Pyne signature.asc Description: This is a digitally signed message part.
Re: Review Request 109814: bump workspace to require Qt 4.8.0
On April 1, 2013, 10:18 p.m., Albert Astals Cid wrote: Qt 4.8 seems like it was released 1.5 years ago, if we need it for a feature I don't see why we should not increase the requirement I agree. Even RHEL 6.2 seems to be able to build Qt 4.8, so it doesn't seem like a severe requirement for KDE 4.11. - Michael --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109814/#review30235 --- On April 1, 2013, 6:41 p.m., Thomas Lübking wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109814/ --- (Updated April 1, 2013, 6:41 p.m.) Review request for kde-workspace. Description --- http://git.reviewboard.kde.org/r/109784/ requires a Qt 4.8 feature Though kdelibs/CMakeLists.txt in 4.10 already has set (QT_MIN_VERSION 4.8.0) kde-workspace atm. requires 4.9.4 which does have that requirement. So this boils down to the question whether a) kde-workspace 4.11 (or just git master) on kdelibs 4.9.x is actually a concern. b) this would expose any downstream version gap (ie. shipping Qt 4.7 and KDE 4.9 but you want to compile workspace master) eg. debian stable has Qt 4.6 (not supported, we already require 4.7) while testing has 4.8 - so there's no issue. Ubuntu ships KDE 4.8 on Precise and 4.9 on Quantal (latter alongside Qt 4.8) Diffs - CMakeLists.txt 06b779b Diff: http://git.reviewboard.kde.org/r/109814/diff/ Testing --- Thanks, Thomas Lübking
Re: KDEReview: Nepomuk-Controller QML
On Tuesday, March 26, 2013 11:42:46 Jörg Ehrichs wrote: One detail though is missing, the old systray was able give a number of indexer file resources. This number is missing from my QML approach. Not sure how important this number is, as it isn't really a good measure or even a measure at all how big the Nepomuk database is at all. Well we rearrange U/I even across point releases (if it fixes bugs) so merely removing a useless stat is not a big concern, as long as the overall functionality is still comparable to what was present before. Removing the number might even be judged as a U/I improvement (why show something neither the user nor developer will need except possibly for bug triage?). Likewise it would be a good idea to ensure our Release Notes for 4.11 (if any are started) reflect the migration so that our packagers can ensure they change package dependencies if deemed appropriate. I would write a blogpost about this, as soon as this goes into SC. IS there any other action I need to do to ensure packagers are aware of the change? The release notes is the big thing, I would expect packagers are tracking that Wiki page or web page as they are put together during the release series so that they can make preparations ahead-of-time. The only other thing you'd want to do is to send an email to the kde-packager mailing list informing them of the change and any action that might be needed or desirable on their part to ensure a smooth transition for their users. The mailing list is moderated so expect that; your message will make it through regardless for this topic. Regards, - Michael Pyne signature.asc Description: This is a digitally signed message part.
Re: Downtime Notification: Git and Subversion
On Tuesday, March 26, 2013 22:33:24 Stephane MANKOWSKI wrote: Hi, I don't know if this is linked, but now, when I use releaseme to build a tar file, I have an error during the git clone. I just tried, both your example from git.kde.org, and a full clone from anongit.kde.org (not sure which one was selected), and there were no issues. I ran a git fsck --full on both as well to verify. I'm not sure what (if anything) changed in the meantime, but are you still having this error? Regards, - Michael Pyne signature.asc Description: This is a digitally signed message part.
Re: KDEReview: Nepomuk-Controller QML
On Monday, March 25, 2013 16:46:10 Vishesh Handa wrote: On Sat, Mar 23, 2013 at 8:12 PM, Jörg Ehrichs joerg.ehri...@gmx.de wrote: The dataengine as well as the applet is only installed if Nepomuk-Core/Soprano is found as build dependency. This would deprecate: kde-runtime/nepomuk/controller/ Deprecate or remove? Cause I would like to remove it. What will the provision be to automatically replace the old systray program with the applet (or at least let the user know where to add the new applet from)? The minor version upgrade process all throughout the KDE/Plasma 4 releases started off as a fairly steady source of minor nits and irritants for users. The Nepomuk controls themselves are already fairly well-known so it's even more important IMO that if they are deprecated that they don't simply disappear completely the first time a new KDE 4.11 user logs in. Likewise it would be a good idea to ensure our Release Notes for 4.11 (if any are started) reflect the migration so that our packagers can ensure they change package dependencies if deemed appropriate. Not trying to discourage, but just some things to think about. Regards, - Michael Pyne signature.asc Description: This is a digitally signed message part.
Re: Interesting issue for kde-baseapps, Jenkins kdesrc-build, proposed solution
On Tuesday, March 12, 2013 20:38:30 Ben Cooksley wrote: Does anyone have objections to the sysadmins realigning the 3 git modules in question? (And if so, I'd appreciate ideas for better ways to fix). Hi, I would have no problem with moving kate.git around, splitting is IMHO a pain for later developing on it, as all parts are tightly coupled anyway. Please note that the actual location of kate.git (on git.kde.org) will not be affected in any way by this. It only affects the module structure. Given the lack of opposition I have submitted a KDE Sysadmin bug [1] to request the move. I will update dependency-data and kdesrc-build after the move is complete. Thanks! [1] https://bugs.kde.org/show_bug.cgi?id=316629 Regards, - Michael Pyne
Interesting issue for kde-baseapps, Jenkins kdesrc-build, proposed solution
Hi all, I would like to propose moving the kde-baseapps, kate, and konsole git modules to an alternative layout within the kde-projects repository, due to circular dependency issues. For some background, the aforementioned modules have the following names and virtual layouts in kde-projects (the database): kde-baseapps = kde/kde-baseapps kate = kde/kde-baseapps/kate konsole = kde/kde-baseapps/konsole Logically speaking, you can see that it might seem that both kate and konsole depend on the kde-baseapps module. As it turns out, the opposite is true, at least for kate: kate provides the text/plain KPart required to exercise the test suite for konqueror in kde- baseapps, so recently Albert Astals Cid made the proper update to the dependency-data information used for Jenkins on build.kde.org, to make kde- baseapps depend on kate. The change is actually correct, but kdesrc-build has an implicit dependency in the opposite direction (kate depending on kde-baseapps) since kdesrc-build uses the same on-disk layout as it presented in the kde-projects database [1] (otherwise there would likely be 100+ directories in the source directory). Without this implicit dependency kdesrc-build would try to run git-clone of kde-baseapps into a non-empty source directory (since it would already contain kate), which git-clone treats as a fatal error. Even without kdesrc-build adding this dependency, it is very strange for a parent module to depend on its child in general. The easiest fix, as proposed by the sysadmins is to move the affected modules to a different virtual layout within kde-projects. Something like: kde-baseapps = kde/$foo/kde-baseapps kate = kde/$foo/kate konsole = kde/$foo/konsole where $foo == applications (as my proposal). This would be a new grouping and make it possible to fix the dependency issue cleanly, since there would only be sibling dependencies (which are far better IMHO than modules that are simultaneously git modules and logical parents of other git modules, like kdelibs is). Other alternatives include splitting kate up in the various library/runtime- support/application components but that's a lot of extra work for what is really just a kde-projects problem. Does anyone have objections to the sysadmins realigning the 3 git modules in question? (And if so, I'd appreciate ideas for better ways to fix). Regards, - Michael Pyne [1] See kdesrc-build commit b48b7ed0db219f1d9e2b9cbb80b1be10fa238410; http://quickgit.kde.org/?p=kdesrc-build.gita=commith=b48b7ed0db219f1d9e2b9cbb80b1be10fa238410 signature.asc Description: This is a digitally signed message part.
Re: kwalletmanager ui refactor
On Sunday, February 03, 2013 14:50:33 Valentin Rusu wrote: Hello, Lots of us are frequently using kwalletmanager to get/store the secrets for the ever extending sensitive information. We click it's icon to pop the main window, then double click the main wallet icon to pop another window, that will eventually go to the second display (that's my case). Perhaps you put aside a quick thought that this should be changed, but return to the task under hand. And then forget about this until you'll next need to use kwalletmanager. :-) During last days I finally sat down and did a ui-refactor and now kwalletmanager handles all the wallets inside a single, KPageWidget based design. A screen capture is far better than a hundred words so here it is: http://imgur.com/MD3GDxO It looks great! One thing I'd add is that it seemed to inherit the old kwalletmanager size, which is too small for the new layout; perhaps ignore the old saved size and prefer sizeHint if the old size is sizeHint (or just have a kconf_update script which removes the old saved size?) P.S. For those wondering how best to review in the face of the source code being moved around, try this git command line: $ git diff -M -C origin/master..origin/ui-refactor -M tries to find renames, -C tries to find copies (not that there should be any here). If you just want to see a summary you can put add --stat (but you might want to try --stat=120 to use more width, other you'll only see partial renames) Regards, - Michael Pyne signature.asc Description: This is a digitally signed message part.
Re: Any chance to fix Bug 312320 in KDE 4.10?
On Tuesday, January 01, 2013 11:32:38 Peter Klotz wrote: Hello Recently I stumbled over Bug #312320 (https://bugs.kde.org/show_bug.cgi?id=312320) and created a patch that according to my tests fixes the problem. Is there a chance that this one-line fix can be incorporated into KDE 4.10? Since it is a data corruption issue I would really appreciate having a fixed version very soon. Peter, I've taken a look at the bug and though I still do not understand why exactly the kio_sftp code does not properly subtract out the extra buffer length in this case, I can confirm the bug. Your fix is correct in that it properly implements the intent of the listed code comment, so it should be safe to go in. At this point I'm not sure if it's the actual fix or just a happy accident but I'm too tired to troubleshoot farther tonight. If no one else gets a chance to look I can commit and backport tomorrow night (~19 hours from now unfortunately). Regards, - Michael Pyne signature.asc Description: This is a digitally signed message part.
Re: Moving Trojitá to extragear
On Thursday, November 22, 2012 12:02:17 Jan Kundrát wrote: On Wednesday, 21 November 2012 20:53:14 CEST, Albert Astals Cid wrote: I.e. if you don't use i18n() or tr() + kdecore (and actually this one has a missing feature because someone in Qt decided to make a method not virtual) you can't get our .po/.mo system work-flow to work. Not having translations seems a big issue to me. Agreed; my goal never was screw transaltions, but let's make it possible to build without kdelibs. I don't care whether the source code uses calls to tr() or i18n() or some other wrapper. If you're willing to support translations only in the event that kdelibs is available then you can use i18n all the time, and use a macro to define it as a no-effect wrapper when kdelibs isn't available. With other (non-KDE) translations systems it might also be possible to flag i18n() as the translatable string marker with a bit of extra work to make i18n() integrate with the alternative translation system. Regards, - Michael Pyne
Re: Review Request: Fix memory leak in KJSEmbed when connecting signals and slots through the QObject binding
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107324/#review22068 --- Ship it! Looks good (even a bit better than my initial try). Assuming it still passes your Kross and script tests then please commit (since this is kdelibs, it should go to the KDE/4.9 branch only, it will be forward-ported for you). - Michael Pyne On Nov. 15, 2012, 10:42 a.m., Daniel Calviño Sánchez wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107324/ --- (Updated Nov. 15, 2012, 10:42 a.m.) Review request for kdelibs. Description --- A char* is used to store the signal name, and another char* is used to store the slot name. Each name is a deep copy of the data of a QByteArray, made with qstrdup. Thus, the copies must be deleted using delete[]. Before, the function callConnect returned without deleting the signal and slot names. Now, the return value is got, the strings are deleted, and then the return value is returned. The strings are needed to get the return value, so they can not be deleted before getting it. Diffs - kjsembed/kjsembed/qobject_binding.cpp afc1989 Diff: http://git.reviewboard.kde.org/r/107324/diff/ Testing --- Memory test for class ScriptingTest in KTutorial project (that is where I discovered the leak, as it uses KJSEmbed through Kross), and unit test for ScriptingTest (to ensure that everything still works as expected after the changes). Thanks, Daniel Calviño Sánchez
Re: Review Request: Fix memory leak in KJSEmbed when connecting signals and slots through the QObject binding
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107324/#review22016 --- First off, good catch on the memory leak, I agree the current code is not handling it correctly. I would propose a different fix instead though. Usually we try to avoid performing manual memory management if we can avoid it, and there's no reason we can't have the compiler do the deallocation for us here. In fact, if you look at the return value of the createSignal() and createSlot() functions (which are being passed to qstrdup()) you will see that they already have a proper type: QByteArray. So instead I would recommend changing the types of the signal and slot variables to QByteArray. You would have to use signal.constData() or slot.constData() in order to use the QMetaObject::indexOfSignal and QMetaObject::indexOfSlot methods, and also for QObject::connect, but no other changes should be needed, and QByteArray will make sure the memory is properly deallocated when the function returns. - Michael Pyne On Nov. 14, 2012, 11:39 a.m., Daniel Calviño Sánchez wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107324/ --- (Updated Nov. 14, 2012, 11:39 a.m.) Review request for kdelibs. Description --- A char* is used to store the signal name, and another char* is used to store the slot name. Each name is a deep copy of the data of a QByteArray, made with qstrdup. Thus, the copies must be deleted using delete[]. Before, the function callConnect returned without deleting the signal and slot names. Now, the return value is got, the strings are deleted, and then the return value is returned. The strings are needed to get the return value, so they can not be deleted before getting it. Diffs - kjsembed/kjsembed/qobject_binding.cpp afc1989 Diff: http://git.reviewboard.kde.org/r/107324/diff/ Testing --- Memory test for class ScriptingTest in KTutorial project (that is where I discovered the leak, as it uses KJSEmbed through Kross), and unit test for ScriptingTest (to ensure that everything still works as expected after the changes). Thanks, Daniel Calviño Sánchez
Re: Comparing KFileItems
On Friday, October 26, 2012 10:16:50 Frank Reininghaus wrote: I was thinking the first one, but indeed Qt often does the second one, I'm not sure what the difference really is. Either one is fine with me. in the end, I did not inline the function - before I pushed the commit, I wondered whether making a non-inline function inline is guaranteed to be binary compatible. According to Techbase, it's not: http://techbase.kde.org/Policies/Binary_Compatibility_Issues_With_C++#The_Do .27s_and_Don.27ts More specifically, the non-inlined version needs to exist. The compiler is still within its rights to actually inline the code if possible (unless you mark it as no_inline, which takes extra effort on your part). Or in other words, the compiler might do the right thing for you regardless. Regards, - Michael Pyne
Re: CMake 2.8.8 will be required for kdelibs 4.10 starting October 30th
On Thursday, October 18, 2012 20:37:04 Alexander Neundorf wrote:
Hi,
in kdelibs we require since more than 2 years cmake .2.6.4, since then many
improvements and fixes have gone into cmake, and we cannot make use of them.
These are e.g.
* builtin automoc
* support for creating proper Config.cmake files
* ${CMAKE_CURRENT_LIST_DIR}
and many many more.
So, after the discussion we had here on this list,
starting October 30th CMake 2.8.8 or newer will be required for building
kdelibs 4.10.
If it is not yet available in your distribution, simply wget and untar it:
In a pinch, git versions of kdesrc-build can also build CMake (from git).
Regards,
- Michael Pyne
Re: Comparing KFileItems
On Thursday, October 18, 2012 13:54:09 Frank Reininghaus wrote: There are ways to fix or work around this problem inside Dolphin, but I'm wondering if it makes sense to change the way KFileItems are compared. If two items do not have the same d pointers, one could check if their URLs are equal and consider them equal in that case. Or, if that is not considered suffient for equality, do what KFileItem::cmp(const KFileItem) does. I see the following advantages and disadvantages: Pro: a) The implicit sharing is an implementation detail of KFileItem. At least I would not have expected that operator==() depends on this and only compares the d pointers. b) It could possibly prevent other bugs in the future and save debugging time. Con: Changing the way KFileItems are compared could considerably slow down comparisons of items which are not equal. Any opinions about this? Well, your pros are both correct (and you didn't even list that the current behavior is inconsistent between KFileItem::cmp() and KFileItem::operator==() ;). But also, you list high CPU usage as a possible con, even though it's what got you into troubleshooting this in the first place! I would recommend comparing the d-ptrs as done now as an early optimization and then falling back to KFileItem::cmp() (or the inlined equivalent) if the d-ptrs are different. Regards, - Michael Pyne signature.asc Description: This is a digitally signed message part.
Re: [PATCH] kdecore: Fix a bug in KDateTime utc offset string parsing.
On Sunday, October 07, 2012 22:23:56 Jon Severinsson wrote: The sign of the UTC offset was ignored, and an offset of -0500 (New York) would be treated at +0500 (Pakistan). This commit also adds a unit test for UTC offset parsing and comparasion. --- Hi When mucking around in the frameworks branch of kdelibs I found a bug in the KDateTime string parsing, which appears to be present in master as well as every branch from 4.0 to 4.10. I have, however, only run the updated unittest using the frameworks branch and Qt5, so someone else should probably test on 4.9, 4.10 and/or master before committing it. I've adapted the patch slightly to the 4.9 testsuite and I have verified that the new test does fail without your patch, and passes with it. I can commit the test and fix to 4.9 and 4.10, but I would like it if someone with experience in the date/time code could review first. The fix makes sense to me, I just don't know if there are other affected areas or unintended breakage that would be experienced. I've CC'ed David Jarvie but anyone else with experience can chime in too. :P Regards, - Michael Pyne signature.asc Description: This is a digitally signed message part.
Re: Requiring cmake 2.8.9 for kdelibs 4.10 ?
On Saturday, September 29, 2012 12:12:43 André Wöbbeking wrote: On Saturday 29 September 2012 11:59:04 Rolf Eike Beer wrote: Am Samstag 29 September 2012, 11:48:16 schrieb André Wöbbeking: Hi Alex, On Saturday 29 September 2012 10:36:55 Alexander Neundorf wrote: I know this will cause some effort, because I guess only few distributions already come with CMake 2.8.9, but doing this once again after 2 1/2 years seems acceptable for me. Do you really need 2.8.9 or would 2.8.8 from April also be sufficient? The older version would probably cause less trouble?!? What trouble would it cause? Well it's one more dependency to fulfill before you can build KDE. Probably no problem for most people here but maybe for distributions or users. I've recently had to add support for building CMake to kdesrc-build for this reason. This isn't a complaint per se as the work is already done, but the price of closely tracking the latest stable release of a dependency is pretty much always that it hampers development of KDE itself. 2.8.8 at least has a shot of having packages available in most distros, that would obviously not be as true for 2.8.10 (and 2.8.9 would be a question as well). I guess the point is that if we're going to bump the dependency to something that isn't broadly available from distro packages then we might as well bump the requirement to the latest release. But hopefully we only make these bumps when there are clear advantages. Regards, - Michael Pyne signature.asc Description: This is a digitally signed message part.
Re: How and what does (plasma theme) caching operate? / KSharedDataCache issue?
and the cache is mmapped and shared in RAM w/o any overhead for everybody else. Well, that's certainly the goal (or rather, the necessary I/O should be getting scheduled by the OS in a way that is satisfactory to the OS). In fact the only two msync() calls are made when the KSharedDataCache is finally destructed. Everything in between mmap and msync/munmap should be getting handled by the OS. Let me know how best I can support our desktop performance by improving KSharedDataCache. Regards, - Michael Pyne [2] what is much, MUCH, ***MUCH*** faster when done by hand and makes using plasma frames take like no time instead of stressing the HDD for seconds and quite reduces the startup time of the desktop shell - even in a running session. Do you have more details on this (benchmark code and the file system used, how it's attached, etc.)? I'm all for safe optimizations. :) signature.asc Description: This is a digitally signed message part.
Re: Review Request: Use a qml based screen locker in place of the screensaver
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106124/#review18947 --- Is there any documentation on how to port screensavers over to the QML-based framework? I'm also confused about something else, is there really no way to XEmbed a typical X screensaver into a managed viewport or something, that way old screensavers can still be used with a Plasma or new-screenlocker shell? I know you've done a KDE forum poll but let me be clear: Removing a major feature like ALL (X, KDE, *all*) SCREENSAVERS as part of 4.10 is not going to go over well with our users or the various roving Internet hiveminds. Just witness the abuse that continues to be heaped on the GNOME folks (e.g. the power button thing, the maximize/restore button complaints, etc.) I will port over my screensaver (if you point me to how it can be done) but I'm convinced this will be highly contentious if this goes in as described. I'm sorry I'm pointing it out this late (I was kind of hoping I wouldn't have to since I'm biased for obvious reasons ;), but I can't let this go without making my big public warning. Am I missing something? Is there an out so that most screensavers are still working that I'm not seeing? - Michael Pyne On Sept. 13, 2012, 12:17 p.m., Marco Martin wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106124/ --- (Updated Sept. 13, 2012, 12:17 p.m.) Review request for KDE Runtime and Martin Gräßlin. Description --- this is the finalization of the old screenlocker branch in workspace: the screen saver goes away (discussed at the time, about one year ago) and the screen locker gets managed by ksmserver, with a greeter that has the ui dine in qml. The same qml ui gets loaded by the plasma based greeter when the allow widgets on screen locker is enabled. the screensaver kcm is now called Screen locker and is way simpler, the screen saver chooser is gone from it. Diffs - kcontrol/screensaver/CMakeLists.txt e4dcc3a kcontrol/screensaver/screensaver.ui 0ad5cd8 kcontrol/screensaver/scrnsave.h 7c8deba kcontrol/screensaver/scrnsave.cpp c0507d4 krunner/CMakeLists.txt 21eac6f krunner/dbus/org.freedesktop.ScreenSaver.xml 5efd943 krunner/dbus/org.kde.screensaver.xml e700b88 krunner/kcfg/kscreensaversettings.kcfg c8f76f3 krunner/kcfg/kscreensaversettings.kcfgc af9133d krunner/krunnerapp.h 040198d krunner/krunnerapp.cpp eea6220 krunner/lock/CMakeLists.txt cf9a67e krunner/lock/autologout.h 0c444050 krunner/lock/autologout.cc c86e29a krunner/lock/config-krunner-lock.h.cmake 7bfdfd6 krunner/lock/kscreenlocker.notifyrc cc5c3ea krunner/lock/lockdlg.h f25e55f krunner/lock/lockdlg.cc 14a9b34 krunner/lock/lockprocess.h 8b6d9a8 krunner/lock/lockprocess.cc 65c7f1d krunner/lock/main.h 8a60353 krunner/lock/main.cc 7b41024 krunner/main.cpp 84a547b krunner/screensaver/saverengine.h 3384d4a krunner/screensaver/saverengine.cpp 4d90faa krunner/screensaver/xautolock.h 3db3233 krunner/screensaver/xautolock.cpp 7124215 krunner/screensaver/xautolock_c.h 3b82f5c krunner/screensaver/xautolock_diy.c b9df2f8 krunner/screensaver/xautolock_engine.c d6d0cf5 ksmserver/CMakeLists.txt 5f0fd34 ksmserver/config-ksmserver.h.cmake 933da35 ksmserver/main.cpp 430a61a ksmserver/screenlocker/CMakeLists.txt PRE-CREATION ksmserver/screenlocker/DESIGN PRE-CREATION ksmserver/screenlocker/Messages.sh PRE-CREATION ksmserver/screenlocker/autologout.h PRE-CREATION ksmserver/screenlocker/autologout.cpp PRE-CREATION ksmserver/screenlocker/data/CMakeLists.txt PRE-CREATION ksmserver/screenlocker/data/force_krunner_lock_shortcut_unreg.cpp PRE-CREATION ksmserver/screenlocker/data/kscreenlocker_locksession-shortcut.upd PRE-CREATION ksmserver/screenlocker/dbus/org.freedesktop.ScreenSaver.xml PRE-CREATION ksmserver/screenlocker/dbus/org.kde.screensaver.xml PRE-CREATION ksmserver/screenlocker/greeter/CMakeLists.txt PRE-CREATION ksmserver/screenlocker/greeter/Messages.sh PRE-CREATION ksmserver/screenlocker/greeter/greeter.h PRE-CREATION ksmserver/screenlocker/greeter/greeter.cpp PRE-CREATION ksmserver/screenlocker/greeter/greeterapp.h PRE-CREATION ksmserver/screenlocker/greeter/greeterapp.cpp PRE-CREATION ksmserver/screenlocker/greeter/main.cpp PRE-CREATION ksmserver/screenlocker/greeter/screensaverwindow.h PRE-CREATION ksmserver/screenlocker/greeter/screensaverwindow.cpp PRE-CREATION ksmserver/screenlocker/greeter/sessions.h PRE-CREATION ksmserver/screenlocker/greeter/sessions.cpp PRE-CREATION
Re: Review Request: Use a qml based screen locker in place of the screensaver
On Sept. 13, 2012, 11:22 p.m., Michael Pyne wrote: Is there any documentation on how to port screensavers over to the QML-based framework? I'm also confused about something else, is there really no way to XEmbed a typical X screensaver into a managed viewport or something, that way old screensavers can still be used with a Plasma or new-screenlocker shell? I know you've done a KDE forum poll but let me be clear: Removing a major feature like ALL (X, KDE, *all*) SCREENSAVERS as part of 4.10 is not going to go over well with our users or the various roving Internet hiveminds. Just witness the abuse that continues to be heaped on the GNOME folks (e.g. the power button thing, the maximize/restore button complaints, etc.) I will port over my screensaver (if you point me to how it can be done) but I'm convinced this will be highly contentious if this goes in as described. I'm sorry I'm pointing it out this late (I was kind of hoping I wouldn't have to since I'm biased for obvious reasons ;), but I can't let this go without making my big public warning. Am I missing something? Is there an out so that most screensavers are still working that I'm not seeing? ahiemstra on IRC pointed me out to Diff 2, which re-adds support for xscreensaver but removes default support. This sounds just fine (I will have to compile and see if this applies to KDE screensavers too). I'd still like to port to whatever the new framework will be to get ahead of the curve on that. ahiemstra has mentioned it's possible by making a Plasmoid. Will there be any other method available with this initially? (If I seem overeager, it's because my autistic son loves the screensaver and now expects to see it when I leave the desk...). Once my current kdesrc-build run completes I'll apply the patch and see what I can figure out regarding the KDE screensavers. - Michael --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106124/#review18947 --- On Sept. 13, 2012, 12:17 p.m., Marco Martin wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106124/ --- (Updated Sept. 13, 2012, 12:17 p.m.) Review request for KDE Runtime and Martin Gräßlin. Description --- this is the finalization of the old screenlocker branch in workspace: the screen saver goes away (discussed at the time, about one year ago) and the screen locker gets managed by ksmserver, with a greeter that has the ui dine in qml. The same qml ui gets loaded by the plasma based greeter when the allow widgets on screen locker is enabled. the screensaver kcm is now called Screen locker and is way simpler, the screen saver chooser is gone from it. Diffs - kcontrol/screensaver/CMakeLists.txt e4dcc3a kcontrol/screensaver/screensaver.ui 0ad5cd8 kcontrol/screensaver/scrnsave.h 7c8deba kcontrol/screensaver/scrnsave.cpp c0507d4 krunner/CMakeLists.txt 21eac6f krunner/dbus/org.freedesktop.ScreenSaver.xml 5efd943 krunner/dbus/org.kde.screensaver.xml e700b88 krunner/kcfg/kscreensaversettings.kcfg c8f76f3 krunner/kcfg/kscreensaversettings.kcfgc af9133d krunner/krunnerapp.h 040198d krunner/krunnerapp.cpp eea6220 krunner/lock/CMakeLists.txt cf9a67e krunner/lock/autologout.h 0c444050 krunner/lock/autologout.cc c86e29a krunner/lock/config-krunner-lock.h.cmake 7bfdfd6 krunner/lock/kscreenlocker.notifyrc cc5c3ea krunner/lock/lockdlg.h f25e55f krunner/lock/lockdlg.cc 14a9b34 krunner/lock/lockprocess.h 8b6d9a8 krunner/lock/lockprocess.cc 65c7f1d krunner/lock/main.h 8a60353 krunner/lock/main.cc 7b41024 krunner/main.cpp 84a547b krunner/screensaver/saverengine.h 3384d4a krunner/screensaver/saverengine.cpp 4d90faa krunner/screensaver/xautolock.h 3db3233 krunner/screensaver/xautolock.cpp 7124215 krunner/screensaver/xautolock_c.h 3b82f5c krunner/screensaver/xautolock_diy.c b9df2f8 krunner/screensaver/xautolock_engine.c d6d0cf5 ksmserver/CMakeLists.txt 5f0fd34 ksmserver/config-ksmserver.h.cmake 933da35 ksmserver/main.cpp 430a61a ksmserver/screenlocker/CMakeLists.txt PRE-CREATION ksmserver/screenlocker/DESIGN PRE-CREATION ksmserver/screenlocker/Messages.sh PRE-CREATION ksmserver/screenlocker/autologout.h PRE-CREATION ksmserver/screenlocker/autologout.cpp PRE-CREATION ksmserver/screenlocker/data/CMakeLists.txt PRE-CREATION ksmserver/screenlocker/data/force_krunner_lock_shortcut_unreg.cpp PRE-CREATION ksmserver/screenlocker/data/kscreenlocker_locksession-shortcut.upd PRE-CREATION ksmserver/screenlocker/dbus
Re: When do we need to update GENERIC_LIB_VERSION / KDE_NON_GENERIC_LIB_VERSION ?
On Thursday, September 06, 2012 14:34:32 David Faure wrote: On Thursday 30 August 2012 20:35:02 Andras Mantia wrote: On Thursday, August 30, 2012 11:51:07 AM Allen Winter wrote: We discussed this on IRC last night. The only downside we could come up with was extra disk space needed by the developers if we weren't vigilant about cleaning-up older library versions. Volker just created a script to clean up the build/install dir when this happens (old .so files lying around). :) Cool. And he uploaded it to... where? In the meantime the script I have for a similar purpose is attached. 4 hours ago it didn't so much as have comments or command-line options so be wary I guess. ;) Regards, - Michael Pyne cleanup-lib.pl Description: Perl program signature.asc Description: This is a digitally signed message part.
Re: Fwd: KActivities library optimizations
On Monday, September 03, 2012 13:31:47 Ivan Čukić wrote: Hi, The latest master of libkactivities caches and pre-fetches some stuff like the currentActivity, list of activities, list of running activities, activity names and icons, to minimise the amount of d-bus related locks*. What do you think of the idea to go one step further, and instead of accessing the data via d-bus, to only use d-bus for signalling the changes, but to use QSharedMemory for actual data access (read-only from the library). This would remove any blocking while accessing the data, unless I'm missing some important fact about QSharedMemory which makes it undesired. (I don't see it much in our code) It adds a lot of things you'd need to be careful about (e.g. how to ensure the data is still in shared memory between when the signal is sent and when the DBus slots are finally called and processed, naming the key for the memory segment, etc.), but the idea is sound. Regards, - Michael Pyne signature.asc Description: This is a digitally signed message part.
Re: Review Request: Add spinlocks lock type, based on GCC intrisincs
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106224/#review18472 --- OK, I've committed a spinlock implementation that should work on OpenBSD without relying on GCC intrinsics alone (it does rely on Qt's atomic classes though). Instead of making the additional functions used wait for the CMake changes I've simply checked for _POSIX_PRIORITY_SCHEDULING as documented in the Linux manpage (and verified not to be defined in OpenBSD's unistd.h or cdefs.h, since sched_yield isn't provided in sched.h). Since the standards regarding sched_yield in particular seem to have changed several times recently (most recently moving to the Base specification for SUS) the only safe thing would be to have a CMake check for that specific function even if the POSIX feature checks appear to support it being available. But that will need to be worked on in the 4.10 branch. - Michael Pyne On Aug. 26, 2012, 7:09 p.m., Vadim Zhukov wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106224/ --- (Updated Aug. 26, 2012, 7:09 p.m.) Review request for kdelibs and Michael Pyne. Description --- Add simple spin locking mechanism. Written by Michael Pyne as from https://bugs.kde.org/attachment.cgi?id=73282 , with some tweaking by me. This addresses bug 305023. http://bugs.kde.org/show_bug.cgi?id=305023 Diffs - kdecore/util/ConfigureChecks.cmake fe9f47e kdecore/util/config-util.h.cmake 83ccdf7 kdecore/util/kshareddatacache_p.h ec5a7a0 Diff: http://git.reviewboard.kde.org/r/106224/diff/ Testing --- On OpenBSD-CURRENT, i386 Thanks, Vadim Zhukov
Re: Review Request: Move checks for locking primitives in KSharedDataCache to CMake
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106174/#review18473 --- I think I've figured out the issue with the missing files. The review request is indeed based against master, which isn't a big problem. If rebasing to KDE/4.10 will be an issue just let me know and I'll integrate the latest diff myself to KDE/4.10. - Michael Pyne On Aug. 27, 2012, 6:54 p.m., Vadim Zhukov wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106174/ --- (Updated Aug. 27, 2012, 6:54 p.m.) Review request for kdelibs and Michael Pyne. Description --- This patch does move some run-time checks (mainly for different locking mechanisms) in KSharedDataCache to build time (CMake). This addresses bug 305023. http://bugs.kde.org/show_bug.cgi?id=305023 Diffs - kdecore/util/ConfigureChecks.cmake fe9f47e kdecore/util/config-util.h.cmake 83ccdf7 kdecore/util/kshareddatacache.cpp 393902e kdecore/util/kshareddatacache_p.h ec5a7a0 Diff: http://git.reviewboard.kde.org/r/106174/diff/ Testing --- On OpenBSD-CURRENT. Thanks, Vadim Zhukov
Re: Review Request: Add spinlocks lock type, based on GCC intrisincs
On Wednesday, August 29, 2012 00:36:07 Vadim Zhukov wrote: 2012/8/28 Thiago Macieira thi...@kde.org: On terça-feira, 28 de agosto de 2012 12.28.24, Vadim Zhukov wrote: See the definition of SharedLock structure in kshareddatacache_p.h. Actually, other union members will not be accessed simultaneously with spinlock, but compiler doesn't know about that. I don't see the need for a union. The other types aren't related to a spinlock. The main thing there is char unused[64] below. The union is needed to keep the size of the whole structure constant. Or... is it impossible that there will be run two KDE-based apps with size of Qt atomic type simultaneously; e.g. during OS update? It's correct that the unused[64] is the key to the union. Since the SharedLock type (including the union) will be placed in shared memory it is desired that the size does that structure does not depend on the type of lock that is being used. It is true that once a cache is created with a given locktype that the locktype will never change for that cache again, so I suppose this feature is not an inherent requirement. I'll look into that too I suppose. Regards, - Michael Pyne signature.asc Description: This is a digitally signed message part.
Re: Review Request: Move checks for locking primitives in KSharedDataCache to CMake
On Aug. 26, 2012, 9:35 p.m., Michael Pyne wrote:
I've looked over everything and didn't see anything I didn't like. I'd like
to apply and and compile and run what little test cases do exist, but
git-apply won't run as kdecore/util/{ConfigureChecks,config-util.h}.cmake
don't exist yet. Are those from stray commits on your end or is there an
earlier review request that includes them?
I'm trying to apply to KDE/4.10 as this is not strictly a bugfix, though I
didn't see either of those in KDE/4.9 or frameworks branches either.
Vadim Zhukov wrote:
Hm-m-m, I've checked out trunk of kdelibs... See the log of my repo:
commit 2975583acb23334efc2f13860d0f9c36462c4524
Author: Vadim Zhukov persg...@gmail.com
Date: Mon Aug 27 12:01:31 2012 +0400
Improve error printing, and mention another (related) reason to avoid
semaphores.
commit 487edb5e59b16edb63c70d3100dc1c9cc608eea8
Author: Vadim Zhukov persg...@gmail.com
Date: Sun Aug 26 22:30:12 2012 +0400
- Remove support for thread-shared-only locks: they do not help at
all but makes app crash instead of slowly work; lack of decent locks to be
resolved later.
- Add error checking with console printouts.
- Consistently use :: before global functions.
commit 04adbc24f5bc14828fcd8da84a322993df4a8f35
Author: Vadim Zhukov persg...@gmail.com
Date: Sun Aug 26 21:17:12 2012 +0400
Move checks for functionality (especially locking mechanisms) to
CMake.
commit 8af088493827cb1eaf6bb4aa4df89f2147986a1a
Merge: 58fd025 fed5ee7
Author: David Faure fa...@kde.org
Date: Thu Aug 16 11:04:58 2012 +0200
Merge branch 'KDE/4.10'
commit 0e8325376aefb912ab318c2795f276fc23a9837c
Author: Sergio Martins iamser...@gmail.com
Date: Tue Aug 14 23:16:51 2012 +0100
s/newer then/newer than
(cherry picked from commit fef8d2c8ce38c735cbf2fa196006ead991a17171)
commit 75328c0cc379d80f179ce77fc66d19706290a674
Merge: 557c126 c67bfaf
Author: David Faure fa...@kde.org
Date: Fri Jul 20 13:23:52 2012 +0200
Merge branch 'KDE/4.9'
commit d178c64d53fcc6ab50d35c4f178b21322b7fa357
Author: Michael Pyne mp...@kde.org
Date: Sat Jun 16 23:32:09 2012 -0400
kshareddatacache: Correct the findEmptyPages loop boundary.
I did review the last patch to remove 'pagesNeeded' from the loop
test,
but I missed that it has '+ 1' instead of '- 1' (which is still too
conservative). The correct thing is to not have any addition at all
since we use operator() for the test.
Graciously pointed out by kde_pepo, thanks!
The current development branches on kdelibs are
* frameworks - for the upcoming KDE Platform 5. This is where most new features
would be going.
* KDE/4.10 - support the upcoming KDE 4.10 release. Strictly speaking no new
features can be added here but I'm willing to make the CMake changes necessary
starting from this branch since we will have time to fix any issues that might
arise in compilation.
* KDE/4.9 - support for the current stable branch. I would imagine the spinlock
support could be backported here, if only to make OpenBSD work.
master used to be a valid branch (and may be again after KDE 5) but for now it
shouldn't be used. Was there a separate patch of yours that created the CMake
files in kdecore/util? I'll go double check...
- Michael
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106174/#review18055
---
On Aug. 27, 2012, 6:54 p.m., Vadim Zhukov wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106174/
---
(Updated Aug. 27, 2012, 6:54 p.m.)
Review request for kdelibs and Michael Pyne.
Description
---
This patch does move some run-time checks (mainly for different locking
mechanisms) in KSharedDataCache to build time (CMake).
This addresses bug 305023.
http://bugs.kde.org/show_bug.cgi?id=305023
Diffs
-
kdecore/util/ConfigureChecks.cmake fe9f47e
kdecore/util/config-util.h.cmake 83ccdf7
kdecore/util/kshareddatacache.cpp 393902e
kdecore/util/kshareddatacache_p.h ec5a7a0
Diff: http://git.reviewboard.kde.org/r/106174/diff/
Testing
---
On OpenBSD-CURRENT.
Thanks,
Vadim Zhukov
Re: Review Request: Add spinlocks lock type, based on GCC intrisincs
On Tuesday, August 28, 2012 00:41:16 Thiago Macieira wrote: On segunda-feira, 27 de agosto de 2012 18.20.15, Michael Pyne wrote: Please use the Qt atomic types. Until GCC 4.7, they generate better code. I agree, the reason it wasn't that way initially is mentioned in the discussion on the bug (but basically because you can't simply put QBasicAtomicInt in the union used to store the different lock types that are possible). Why not? QBasicAtomicInt are permitted in unions. Besides, why do you want it in a union in the first place? You should not access the data that it holds *except* via the QBasicAtomicInt functions. That would be the idea, yes (to use the public QBAI functions). The problem with having it in a union was that it's a non-POD type according to C++ 03 rules (or at least, that seemed to be the issue when I had tried that initially). Regards, - Michael Pyne signature.asc Description: This is a digitally signed message part.
Re: Review Request: Move checks for locking primitives in KSharedDataCache to CMake
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106174/#review18055
---
I've looked over everything and didn't see anything I didn't like. I'd like to
apply and and compile and run what little test cases do exist, but git-apply
won't run as kdecore/util/{ConfigureChecks,config-util.h}.cmake don't exist
yet. Are those from stray commits on your end or is there an earlier review
request that includes them?
I'm trying to apply to KDE/4.10 as this is not strictly a bugfix, though I
didn't see either of those in KDE/4.9 or frameworks branches either.
- Michael Pyne
On Aug. 26, 2012, 5:15 p.m., Vadim Zhukov wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106174/
---
(Updated Aug. 26, 2012, 5:15 p.m.)
Review request for kdelibs and Michael Pyne.
Description
---
This patch does move some run-time checks (mainly for different locking
mechanisms) in KSharedDataCache to build time (CMake).
This addresses bug 305023.
http://bugs.kde.org/show_bug.cgi?id=305023
Diffs
-
kdecore/util/config-util.h.cmake 83ccdf7
kdecore/util/kshareddatacache.cpp 393902e
kdecore/util/kshareddatacache_p.h ec5a7a0
kdecore/util/ConfigureChecks.cmake fe9f47e
Diff: http://git.reviewboard.kde.org/r/106174/diff/
Testing
---
On OpenBSD-CURRENT.
Thanks,
Vadim Zhukov
Re: Review Request: Mutex cleanup in KSharedDataCache
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106221/#review18056 --- A few comments, but most of the changes are OK (though they will have to be KDE/4.10 items). kdecore/util/kshareddatacache_p.h http://git.reviewboard.kde.org/r/106221/#comment14302 Please pass errno through ::strerror() as this is a great resource to have when debugging. Likewise for other uses of rc. kdecore/util/kshareddatacache_p.h http://git.reviewboard.kde.org/r/106221/#comment14303 strerror() would be useful here as well, and in the other new uses of errno. - Michael Pyne On Aug. 26, 2012, 7:02 p.m., Vadim Zhukov wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106221/ --- (Updated Aug. 26, 2012, 7:02 p.m.) Review request for kdelibs and Michael Pyne. Description --- - Remove support for thread-shared-only locks: they do not help at all but makes app crash instead of slowly work; lack of decent locks to be resolved later. - Add error checking with console printouts. - Consistently use :: before global functions. Probably failed mutexes should be destroyed in case of some errors, any thoughts? This addresses bug 305023. http://bugs.kde.org/show_bug.cgi?id=305023 Diffs - kdecore/util/kshareddatacache.cpp 393902e kdecore/util/kshareddatacache_p.h ec5a7a0 Diff: http://git.reviewboard.kde.org/r/106221/diff/ Testing --- On OpenBSD-CURRENT. Thanks, Vadim Zhukov
Re: Review Request: Silence debug output in KBzip2Filter::uncompress
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106056/#review17566 --- Ship it! I like it, and it's even easier than what my fix was going to be. - Michael Pyne On Aug. 16, 2012, 6:16 p.m., Milian Wolff wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106056/ --- (Updated Aug. 16, 2012, 6:16 p.m.) Review request for kdelibs and Michael Pyne. Description --- Only output warning message if the return value of bzDecompress is negative, which represents an error. See e.g. bzlib.h: #define BZ_OK0 #define BZ_RUN_OK1 #define BZ_FLUSH_OK 2 #define BZ_FINISH_OK 3 #define BZ_STREAM_END4 #define BZ_SEQUENCE_ERROR(-1) #define BZ_PARAM_ERROR (-2) #define BZ_MEM_ERROR (-3) #define BZ_DATA_ERROR(-4) #define BZ_DATA_ERROR_MAGIC (-5) #define BZ_IO_ERROR (-6) #define BZ_UNEXPECTED_EOF(-7) #define BZ_OUTBUFF_FULL (-8) #define BZ_CONFIG_ERROR (-9) As such, all negative values (or anything below BZ_OK) represents an error. Without this patch, using the KBzip2Filter would always spam the console with the following debug messages: bzDecompress returned 4 KBzip2Filter::uncompress 1 Diffs - kdecore/compression/kbzip2filter.cpp 0f55334 Diff: http://git.reviewboard.kde.org/r/106056/diff/ Testing --- Thanks, Milian Wolff
Re: Review Request: [PATCH] Make kio_info produce valid HTML
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105590/#review16236 --- Ship it! Sorry about the delay (I was away). I don't see any other issues, please commit. - Michael Pyne On July 19, 2012, 8:21 p.m., Paul Walger wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105590/ --- (Updated July 19, 2012, 8:21 p.m.) Review request for KDE Runtime. Description --- Fixes Bug 295170. This addresses bug 295170. http://bugs.kde.org/show_bug.cgi?id=295170 Diffs - kioslave/info/kde-info2html bff85bbde3f3a66467f7f198ee8c4eae25aa1f6d Diff: http://git.reviewboard.kde.org/r/105590/diff/ Testing --- Tested via http://validator.w3.org Thanks, Paul Walger
Re: Review Request: [PATCH] Make kio_info produce valid HTML
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105590/#review15990 --- I think the current best-practice for HTML is a doctype of (and I'm not kidding) !DOCTYPE html, which also prevents requiring a manual annotation of text/css later in the style sheets. However these changes (except the question of the missing div) are harmless so they can be committed. A long-term project for those who are interested would be to find if there are more suitable info-html converters available instead of old Perl scripts (and likewise for man pages). Maybe pandoc? kioslave/info/kde-info2html http://git.reviewboard.kde.org/r/105590/#comment12595 Was there a reason this div was removed? - Michael Pyne On July 16, 2012, 11:38 a.m., Paul Walger wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105590/ --- (Updated July 16, 2012, 11:38 a.m.) Review request for KDE Runtime. Description --- Fixes Bug 295170. This addresses bug 295170. http://bugs.kde.org/show_bug.cgi?id=295170 Diffs - kioslave/info/kde-info2html bff85bbde3f3a66467f7f198ee8c4eae25aa1f6d Diff: http://git.reviewboard.kde.org/r/105590/diff/ Testing --- Tested via http://validator.w3.org Thanks, Paul Walger
Re: [HEADS-UP] New Shared Desktop Ontologies requirement (v1.10.0)
On Wednesday, June 20, 2012 12:19:51 Allen Winter wrote: Howdy, In a few days we will be requiring v1.10.0 (or higher) of the shared-desktop-ontologies package for building kdepim-runtime master (effectively for KDE SC 4.9) Probably your distro does not have this version yet, so you'll need to install from source. Get the tarball at http://sourceforge.net/projects/oscaf/ at the very least we will require this version for kdepim-runtime, perhaps for kdelibs too. so please install I haven't tried it lately but SDO's trunk release should also be buildable from kdesrc-build, and build-tool probably has a similar recipe (though I haven't checked). The kdesrc-build SDO configuration in the current sample configuration is: module shared-desktop-ontologies repository git://oscaf.git.sourceforge.net/gitroot/oscaf/shared-desktop- ontologies branch master end module Note that this is only truly useful if you have an existing KDE development build of some sort to install to without interfering with the system packages, but that same precaution applies to installing from source. :-/ Regards, - Michael Pyne signature.asc Description: This is a digitally signed message part.
Re: Question regarding compatibility for kdecore and KDE4_ENABLE_FINAL
On Thursday, June 14, 2012 15:36:47 David Faure wrote: On Tuesday 12 June 2012 20:42:28 Michael Pyne wrote: Hi all, Bug 301419 has been reported against kdelibs due to a build failure when KDE4_ENABLE_FINAL is used, introduced by some commits of mine to perform even more sanity checking in the KSharedDataCache. These commits use exceptions (as are already used in khtml) since they are actually the right tool in the context of where they are used, and because refactoring everything to use error codes everywhere (ECE) would have risked introducing more bugs. In order to minimize the changes to kdecore I only added the CMake magic to enable exceptions for only kshareddatacache.cpp. This doesn't work when KDE4_ENABLE_FINAL is used, as the project-wide CXXFLAGS are used in that case. The Mageia devs have a proposed patch [1] to enable exceptions for all of kdecore, which fixes the issue. Is it acceptable for me to go this route? The only real alternative this late in the game is to back out the sanity checks to the 4.8.3 status or to explicitly say that KDE4_ENABLE_FINAL will not work for this tarball although it worked for 4.8.3, both of which I consider less desirable. But I don't want to make the change if there are good reasons to avoid it. The alternative would be to enable exceptions for all of kdecore only if enable-final is enabled. Sorry to be unclear -- that's exactly what the Mageia patch accomplishes. They only flip the exceptions bit for enable-final builds. If that sounds agreeable I intend on making that alteration and forwarding the patch to kde-packager. Regards, - Michael Pyne signature.asc Description: This is a digitally signed message part.
Re: Question regarding compatibility for kdecore and KDE4_ENABLE_FINAL
On Thursday, June 14, 2012 20:18:04 Albert Astals Cid wrote: El Dijous, 14 de juny de 2012, a les 15:36:47, David Faure va escriure: On Tuesday 12 June 2012 20:42:28 Michael Pyne wrote: Hi all, Bug 301419 has been reported against kdelibs due to a build failure when KDE4_ENABLE_FINAL is used, introduced by some commits of mine to perform even more sanity checking in the KSharedDataCache. These commits use exceptions (as are already used in khtml) since they are actually the right tool in the context of where they are used, and because refactoring everything to use error codes everywhere (ECE) would have risked introducing more bugs. In order to minimize the changes to kdecore I only added the CMake magic to enable exceptions for only kshareddatacache.cpp. This doesn't work when KDE4_ENABLE_FINAL is used, as the project-wide CXXFLAGS are used in that case. The Mageia devs have a proposed patch [1] to enable exceptions for all of kdecore, which fixes the issue. Is it acceptable for me to go this route? The only real alternative this late in the game is to back out the sanity checks to the 4.8.3 status or to explicitly say that KDE4_ENABLE_FINAL will not work for this tarball although it worked for 4.8.3, both of which I consider less desirable. But I don't want to make the change if there are good reasons to avoid it. The alternative would be to enable exceptions for all of kdecore only if enable-final is enabled. Is that binary compatible? If I'm understanding it right, it would add extra typeinfo classes for kdecore but would remain binary-compatible with older code. It is not mentioned at all in our TechBase binary compatibility page, which is even the #1 Google hit now. :) Also the flag itself is claimed as binary compatible (in the context of code that doesn't actually /use/ exceptions) in some discussion on the Qt dev list, by Olivier Goffart and Thiago Macieira [1] (both of whom I trust more than myself on this topic). Researching further, the GCC libstdc++ page [2] mentions that exception handling is part of the ABI, but I can only assume this is in the context of does this code throw exceptions or not as opposed to whether non-exception- using code would care about that flag. The GCC docs on -fexceptions talk about extra code to propagate exceptions and perhaps data size overhead but there's no specific warning regarding changing the ABI visible outside of the object file. The detailed GCC ABI spec is at [3] and it seems to me from my reading of it that the changes involved are changes to data included with the .o, changes in function implementations, and extra calls to the C++ runtime library... but no changes to symbol names, mangling, etc. As far as I can tell this change should be binary compatible. As an experiment I've recompiled kdecore here with exceptions and run KDevelop (which I think is a fair exercise of much of the breadth of kdecore technologies) and wasn't able to find any new issues from that change. [1] http://permalink.gmane.org/gmane.comp.lib.qt.devel/3681 [2] http://gcc.gnu.org/onlinedocs/gcc/Code-Gen- Options.html#Code%20Gen%20Options [3] http://sourcery.mentor.com/public/cxx-abi/abi-eh.html#cxx-abi Regards, - Michael Pyne signature.asc Description: This is a digitally signed message part.
Question regarding compatibility for kdecore and KDE4_ENABLE_FINAL
Hi all, Bug 301419 has been reported against kdelibs due to a build failure when KDE4_ENABLE_FINAL is used, introduced by some commits of mine to perform even more sanity checking in the KSharedDataCache. These commits use exceptions (as are already used in khtml) since they are actually the right tool in the context of where they are used, and because refactoring everything to use error codes everywhere (ECE) would have risked introducing more bugs. In order to minimize the changes to kdecore I only added the CMake magic to enable exceptions for only kshareddatacache.cpp. This doesn't work when KDE4_ENABLE_FINAL is used, as the project-wide CXXFLAGS are used in that case. The Mageia devs have a proposed patch [1] to enable exceptions for all of kdecore, which fixes the issue. Is it acceptable for me to go this route? The only real alternative this late in the game is to back out the sanity checks to the 4.8.3 status or to explicitly say that KDE4_ENABLE_FINAL will not work for this tarball although it worked for 4.8.3, both of which I consider less desirable. But I don't want to make the change if there are good reasons to avoid it. Longer term (for frameworks) KSharedDataCache could be split into its own tier if necessary (it only really depends on Qt and KStandardDirs, which is now also in Qt...). Regards, - Michael Pyne [1] http://svnweb.mageia.org/packages/cauldron/kdelibs4/current/SOURCES/kdelibs-4.8.4- correct-use-fexception-switch.patch?revision=260068view=markup signature.asc Description: This is a digitally signed message part.
Re: Review Request: Remove additional directories from shortcuts scheme export path
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104981/#review14150 --- Ship it! In response to the ping, I like the concept of the bugfix and the patch appears sound. I haven't tested it though but if no one has complained in patch review yet then it's probably safe to commit. ;) - Michael Pyne On May 24, 2012, 10:01 p.m., Burkhard Lück wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104981/ --- (Updated May 24, 2012, 10:01 p.m.) Review request for kdelibs and Andreas Pakulat. Description --- The Configure Shortcuts dialog has an Action to export a scheme (Details-More Actions-Export Scheme) Using this action the user is asked for a export location and has to select a directory. Then the current scheme 'schemename' in application 'appname' is exported to a file named appnameschemenameshortcuts.rc. But this file is not saved in the selected directory as Joe User would expect, but in shortcuts/share/apps/appname/ below the selected folder. This patch removes the additional directories shortcuts/share/apps/appname/ from the export path to make it easier for the user to find the scheme file and move/copy it via command line (there is no GUI to import a scheme). Diffs - kdeui/dialogs/kshortcutschemeseditor.cpp 34a485a Diff: http://git.reviewboard.kde.org/r/104981/diff/ Testing --- Thanks, Burkhard Lück
Re: Review Request: include KolorManager in kdegraphics
On Wednesday, March 14, 2012 20:43:59 Daniel Nicoletti wrote: On the other hand if there are things that a mere 'power user' might find useful (that colord will not be supporting due to scope) then it might make sense to have extra U/I if Oyranos is available. Perhaps multi-monitor CMS would fit the bill (assuming colord will not support). I'm sure you were just giving an example but as someone earlier mentioned something about NVIDIA here's the explanation: Multi monitor color correction works as long as your video driver supports XRandR 1.3, which means NVIDIA proprietary driver is the only one not supporting this. If we support XRR 1.2 both monitors get the same correction. OK, thanks for the clarification. I didn't mean to further spread a possible misconception (although I can't go back and edit it now anyways). Regards, - Michael Pyne signature.asc Description: This is a digitally signed message part.
