Review Request 111042: Upstream SUSE specific changes and introduce a DISTRO switch to enable
Thomas Lübking
thomas.luebking at gmail.com
Sun Jun 30 19:57:49 BST 2013
> On June 15, 2013, 5: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).
>
> Johannes Obermayr wrote:
> @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")
> I don't understand this. If an user tells (s)he uses openSUSE's RPM...
Keyword: "if".
The majority of users tells you nothing to less relevant. At best they tell you which ppa or package they use.
Usually you're lucky when it crashes and you get the required data from Dr. Konqui.
Also that's not the point.
> if (access ("/etc/SuSE-release", R_OK) != 0) {
> openSUSE_specific_code
> }
You missed Martins key argument:
"There shall be no whatsoever openSUSE_specific_code"
No matter how.
The idea to have those downstream patches "maintained" upstream is entirely flawed.
The only "benifit" would be upstream maintaining downstream patches - w/o however even having every upstream developer the required installation base to do so.
-> Effectively the code becomes unmaintained, ie. bitrot.
On the other hand, there's a bunch of issues with the approach.
1. #ifdef orgies make code hard to read and even harder to trace.
---> Most important, they add build complexity and several less to not at all tested or testable code paths.
Any #ifdef needs to be justified and the example below and anything similar is *certainly* not. That is because
2. Either a downstream patch is sane and fixes a bug or adds a valuable feature. Then it should certainly be added upstream.
Or downstream should check on the comments on the patch or seriously consider whether actually wanting to keep it.
Or a downstream patch has vendor customizations what requires adding a generic vendor support system.
3. A fork does not become "no fork" by sharing some repository. KDE would still actively encourage forking by this.
4. Git has a very nice branch feature, the sane way to temporarily "fork" in even the same repo before merging to master.
5. This patch example:
Index: kdecore/localization/klocale_kde.cpp
===================================================================
--- kdecore/localization/klocale_kde.cpp.orig
+++ kdecore/localization/klocale_kde.cpp
@@ -319,6 +319,9 @@ void KLocalePrivate::initMainCatalogs()
m_catalogNames.append(KCatalogName(QString::fromLatin1("kdeqt")));
m_catalogNames.append(KCatalogName(QString::fromLatin1("solid_qt")));
m_catalogNames.append(KCatalogName(QString::fromLatin1("kdecalendarsystems")));
+ m_catalogNames.append(KCatalogName(QString::fromLatin1("desktop_translations")));
+ m_catalogNames.append(KCatalogName(QString::fromLatin1("kde4-openSUSE")));
+ m_catalogNames.append(KCatalogName(QString::fromLatin1("kde4-SLE")));
m_numberOfSysCatalogs = m_catalogNames.size() - numberOfCatalogs;
updateCatalogs(); // evaluate this for all languages
is a hack and no solution.
And it does no way scale. Distrowatch lists the Top 300 distros and regardless of their ranking system, do you suggest to add
#if DISTRO_SUSE
+ m_catalogNames.append(KCatalogName(QString::fromLatin1("desktop_translations")));
+ m_catalogNames.append(KCatalogName(QString::fromLatin1("kde4-openSUSE")));
+ m_catalogNames.append(KCatalogName(QString::fromLatin1("kde4-SLE")));
#elseif DISTRO_KUBUNTU
+ m_catalogNames.append(KCatalogName(QString::fromLatin1("desktop_translations")));
+ m_catalogNames.append(KCatalogName(QString::fromLatin1("kde4-kubuntu")));
#elseif DISTRO_UBUNTU
+ m_catalogNames.append(KCatalogName(QString::fromLatin1("desktop_translations")));
+ m_catalogNames.append(KCatalogName(QString::fromLatin1("kde4-ubuntu")));
#elseif DISTRO_FEDORA
+ m_catalogNames.append(KCatalogName(QString::fromLatin1("desktop_translations")));
+ m_catalogNames.append(KCatalogName(QString::fromLatin1("kde4-fedora")));
+ m_catalogNames.append(KCatalogName(QString::fromLatin1("kde4-RHEL")));
#elseif DISTRO_MINT
+ m_catalogNames.append(KCatalogName(QString::fromLatin1("desktop_translations")));
+ m_catalogNames.append(KCatalogName(QString::fromLatin1("kde4-mint")));
........
#elseif DISTRO_LXLE
+ m_catalogNames.append(KCatalogName(QString::fromLatin1("desktop_translations")));
+ m_catalogNames.append(KCatalogName(QString::fromLatin1("kde4-LXLE")));
#endif
I admit that i've zip knowledge about i18n requirements, but if there's apparently downstream requirement to add more catalogues (or to only serve other downstream patches that should maybe move upstream?), the simple upstream solution would be to have sth. like
foreach (const QString &vendorCatalog, m_config->group("VendorStuff").readEntry("i18nCatalogs", QStringList())
m_catalogNames.append(KCatalogName(vendorCatalog));
6. Finally this:
"Grumble and then don't change anything. [...] That's my long experience with elitist openSUSE KDE maintainers [...]"
perfectly prospects where this is gonna head.
Do you believe any upstream would be the least interested in any dowsntream moving up their internal quarrels - rather than their solutions?
The next step to this seems obvious.
You're apparently unhappy with something on OpenSuSE patches and -justified or not does here not matter- therefore you'll claim your SuSE derivate (hey: there're a bazillion Ubuntu derivates, must be space for one or two OpenSuSE ones, that's just fair, isn't?) to be supported.
Bottom Line:
What you -at least apparently- seem to do, is trying to solve social issues by introducing technical ones.
Sorry if I sounded harsh. I don't want to be mean, but don't see this here really leading anywhere.
I hope you understood why the approach cannot work and what the suggested (and i hope and assume: generally wanted) alternative is:
Distros should not hesitate to present their patches for review for upstream inclusion. Not even Canonical =)
- Thomas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/111042/#review34388
-----------------------------------------------------------
On June 15, 2013, 4: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, 4: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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20130630/950eef1f/attachment.htm>
More information about the kde-core-devel
mailing list