> On June 15, 2013, 7:06 p.m., Albert Astals Cid wrote:
> > Is this some kind of joke?
> >
> > Honestly upstreaming patches is good, but when they make sense, and adding
> > billions of "If #SUSE" to our code does not make any sense. Please discard
> > this review, break stuff in separate patches and if they make sense we can
> > discuss adding them upstream without the "IF SUSE".
>
> Johannes Obermayr wrote:
> Do you remember on
> http://tsdgeos.blogspot.de/2010/05/complex-systems-are-complex.html and how
> long I had to study openSUSE's kdelibs4 package which of the patches were
> applied and which not, rebuilding packages, installing -debuginfo, etc.? Last
> sentence is one of the reasons for my wish to get also distro specific
> patches upstream - not only openSUSE's.
> If I remember right there were and should be some distro specific bug
> reports on bko: The more people have easy access to source code in "Complex
> systems [which] are complex" the likelier reasons for specific bugs can be
> tracked down ...
>
> Yes, binding upstream developers to downstream changes/problems is not
> really nice but could also mean a win-win-situation for both: downstream devs
> will do more upstream work and upstream devs can review downstream work,
> better find distro specific bugs and assign them to downstream devs.
>
> Intention of this review request is to start a discussion ...
>
> Martin Gräßlin wrote:
> Upstreaming the patches with if #SUSE doesn't solve the problem: one
> still doesn't know which code is run by a user's setup. Even worse: the
> chances for bitrot increase. The current system of downstream patches mean
> that the distribution has to ensure that the patches apply on new versions. A
> human has to look at each of them. That's the cost of having downstream
> patches. If you currently not do that (e.g. debug areas for non shipped
> software) you should rethink whether you have too many patches.
>
> For downstream patches there are the following possible solutions:
> * Try to upstream them: if upstream accepts them you get rid of the
> patch, if upstream doesn't accept you should best drop the patch, too
> * For distro-specific behavior: add abstraction with plugin system and
> upstream
> * keep patches for build issues (common example: compile errors with non
> C++ compliant compilers)
>
> Personally I would go so far that we as upstream say that we don't accept
> bug reports in case the distributions carries downstream patches.
>
> Luca Beltrame wrote:
> [Johannes]
> > Intention of this review request is to start a discussion ...
>
> Use the mailing list, not a review request, to start a discussion (and do
> not upload a broken patch).
>
> > how long I had to study openSUSE's kdelibs4 package which of the patches
>
> Then you should have contacted the openSUSE community KDE team first,
> since we had already started a patch review of the distro-shipped patches. I
> reiterate that the rest of the team is absolutely against this RR. And you
> should have noticed by now that we strive already to push things upstream.
>
> [Martin]
> > * Try to upstream them: if upstream accepts them you get rid of the
> patch, if upstream doesn't accept you should best drop the patch, too
>
> For some case, it is not possible as desirable functionality will not be
> integrated upstream in some cases (e.g. KDM with plymouth, to make an
> example).
@Martin:
> Upstreaming the patches with if #SUSE doesn't solve the problem: one still
> doesn't know which code is run by a user's setup.
I don't understand this. If an user tells (s)he uses openSUSE's RPM which are
compiled with code within "#if defined(DISTRO_SUSE) ... #endif" you should be
able to say which code is running.
> Even worse: the chances for bitrot increase. [...] * For distro-specific
> behavior: add abstraction with plugin system and upstream
Can't bitrot increase also happen on an abstraction layer?
I think the preprocessor solution is the better one because it results in
faster and smaller binaries without binding other distributions to code they
don't use:
if (access ("/etc/SuSE-release", R_OK) != 0) {
openSUSE_specific_code
}
takes more CPU cycles in binary than
Add -DDISTRO_SUSE=1 to FLAGS
#if defined(DISTRO_SUSE)
openSUSE_specific_code
#endif
Also this patch could be upstreamed only with preprocessor solution:
https://build.opensuse.org/package/view_file?expand=1&file=add-suse-translations.diff&package=kdelibs4&project=KDE:Distro:Factory
> Personally I would go so far that we as upstream say that we don't accept bug
> reports in case the distributions carries downstream patches.
So sb. must adapt drkonqi to deny sending bug reports from openSUSE RPMs to bko.
@Luca:
> - ksuseinstall is actually on its way to be *killed* in future openSUSE
> versions.
> - Stuff like YMP handlers (one-click installs)
> - The patch is *broken* in many ways, including adding debug areas for non
> shipped software (kupdateapplet), a lot of needless compatibility layers
> (KDE3) etc. It's just a wholesale port made without any serious efforts at
> reviewing.
Why don't you drop obsolete patches and waste binaries then?
[Off-topic] Grumble and then don't change anything (e. g.
kdebug-areas-update.diff is still applied in KDF - it takes <30 seconds to
remove it). That's my long experience with elitist openSUSE KDE maintainers who
cannot accept new maintainers for KDE: projects because of missing rules for
that [1] (or maybe my accepted requests [2] were insufficient and nobody wants
to say this).[/Off-topic]
>> Intention of this review request is to start a discussion ...
> Use the mailing list, not a review request, to start a discussion (and do not
> upload a broken patch).
This way the [final] code can be easily reviewed and commented. I will upload a
patch series and close this review request if a solutions is found ...
[1] https://bugzilla.novell.com/show_bug.cgi?id=706813#c16
[2] https://build.opensuse.org/home/requests?user=jobermayr (please change to
"accepted")
- Johannes
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/111042/#review34388
-----------------------------------------------------------
On June 15, 2013, 6:27 p.m., Johannes Obermayr wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111042/
> -----------------------------------------------------------
>
> (Updated June 15, 2013, 6:27 p.m.)
>
>
> Review request for kdelibs.
>
>
> Description
> -------
>
> Distributions should upstream their patches / changes:
> - Upstream / other distributions can easily see distro specific changes and
> enable them by default by removing "#if defined(DISTRO_xxx)"
> - Maybe duplicate work can be avoided and other distributions can easily use
> them by "|| defined(DISTRO_xxx)"
> - Less adaptions of downstream patches ...
>
>
> Diffs
> -----
>
> CMakeLists.txt 705c84e
> kdecore/config/kconfig.cpp 048605d
> kdecore/config/kconfig_p.h 7751242
> kdecore/config/kconfigdata.h e5dd7da
> kdecore/config/kconfiggroup.h 8eddfd5
> kdecore/config/kconfiggroup.cpp 9e73eb7
> kdecore/config/kdesktopfile.h 1c4eae6
> kdecore/config/kdesktopfile.cpp 54e5910
> kdecore/kdebug.areas 29a4415
> kdecore/localization/klocale_kde.cpp b010e74
> kdecore/services/kservice.h 3843bad
> kdecore/services/kservice.cpp e2cc86f
> kdecore/services/kservicegroup.h 9fdf2b0
> kdecore/services/kservicegroup.cpp 08bc587
> kdecore/services/kservicegroup_p.h 5f21497
> kded/vfolder_menu.cpp f0b0b35
> kdesu/defaults.h 706a088
> kdeui/kernel/kglobalsettings.cpp 2e3a7eb
> khtml/html/html_objectimpl.cpp f0f590d
> kio/CMakeLists.txt 4854829
> kio/kio/kprotocolmanager.cpp 05bb547
> kio/kio/krun.cpp ad5656e
> kjs/collector.cpp cdd8421
> plasma/containment.h e725a99
> plasma/containment.cpp fc2ca70
> plasma/private/containment_p.h 75a6f80
> plasma/theme.cpp 4554de7
> suseinstall/CMakeLists.txt PRE-CREATION
> suseinstall/kbuildsycocaprogressdialog.h PRE-CREATION
> suseinstall/kbuildsycocaprogressdialog.cpp PRE-CREATION
> suseinstall/ksuseinstall.h PRE-CREATION
> suseinstall/ksuseinstall.cpp PRE-CREATION
> suseinstall/ksuseinstall_export.h PRE-CREATION
>
> Diff: http://git.reviewboard.kde.org/r/111042/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Johannes Obermayr
>
>
