another design question -- proper (meaningful) content of recipe patches?

another design question -- proper (meaningful) content of recipe patches?

[Robert P. J. Day]

  got into an animated discussion the other day regarding proper
design of patch files associated with a recipe. as a demo, i can
actually use part of a recipe file ripped from the meta-digi-arm
layer, for "kernel-module-qualcomm", whose recipe file contains (in
part):

  SRC_URI = " \
    ${CAF_MIRROR};destsuffix=${PV};branch=${SRCBRANCH} \
    file://qualcomm-pre-up \
    file://modprobe-qualcomm.conf \
    file://0001-qcacld-Fix-compiling-errors-when-BUILD_DEBUG_VERSION.patch \
    file://0002-Update-cfg80211_vendor_event_alloc-call-for-newer-ke.patch \
    file://0003-wlan_hdd_main-Update-cfg80211_ap_stopped-to-nl80211_.patch \
    file://0004-qcacld-2.0-remove-unused-code.patch \
    file://0005-Including-header-file-for-regulatory_hint_user.patch \
    file://0006-Updating-calls-to-alloc_netdev_mq.patch \
    file://0007-wlan_hdd_cfg80211-update-cfg80211_inform_bss-params-.patch \
    file://0008-wlan_hdd_p2p-Update-call-to-cfg80211_rx_mgmt-for-dif.patch \
    file://0009-linux_ac-Fix-for-f_dentry.patch \
    file://0010-native_sdio-src-hif-Do-not-call-to-HIGH-SPEED-functi.patch \
    file://0011-osdep_adf.h-fix-for-undefined-ath_sysctl_pktlog_size.patch \
    file://0012-Kbuild-Add-compilation-flag-based-on-kernel-support.patch \
    file://0013-Kbuild-do-not-compile-the-DEBUG-version-inconditiona.patch \
    file://0014-Kbuild-Group-most-of-the-relevant-DEBUG-options.patch \
    file://0015-wlan_hdd_cfg80211-fix-missing-ifdef-clause.patch \
    file://0016-Add-.gitignore-rules.patch \
    file://0017-wlan_hdd_main-initialize-all-adapter-completion-vari.patch \
    file://0018-qcacld-Indicate-disconnect-event-to-upper-layers.patch \
    file://0019-wdd_hdd_main-Print-con_mode-to-clearly-see-if-in-FTM.patch \
    file://0020-Makefile-Pass-BUILD_DEBUG_VERSION-to-kbuild-system.patch \
    file://0021-cosmetic-change-log-level.patch \
    file://0022-fix-issue-with-_scan_callback.patch \
  "

  when i was presented with something resembling the above the other
day, my immediate reaction was , "yuck." at the time, i opined that,
if the purpose of the above was to simply add a qualcomm module, it
was silly to do that with a lengthy, numbered series of patches that
were almost certainly inter-dependent and even numbered to enforce a
particular processing order.

  i insisted that the above was unnecessarily messy and, if a
developer tried to examine the patches, it would be next to impossible
to do that easily, as it would require walking through each patch,
then mentally adding the next one, and so on and so on.

  i suggested that, in cases like this, adding a well-defined
functionality should be done with a single all-encompassing patch or,
at the very worst, a small number of patches that might be applied
independently. as in, "Add-qualcomm-driver.patch".

  the argument i got coming back was that the above made sense as it
reflected the *history* of how that driver came to be added. i
shuddered, and pointed out that if one wanted to see the history,
that's what version control is for -- there is no reason that the
information from version control should be used *verbatim* to generate
the patches for a recipe.

  in any event, i can appreciate the value of sometimes adding
numbering to patches to enforce a processing order, but when one has a
numbered series of patches getting up to 20 or 25, that strikes me as
time to collapse all that into larger and more modular and more
meaningful patches.

  thoughts?

rday

-- 

========================================================================
Robert P. J. Day                                 Ottawa, Ontario, CANADA
                        http://crashcourse.ca

Twitter:                                       http://twitter.com/rpjday
LinkedIn:                               http://ca.linkedin.com/in/rpjday
========================================================================

Re: another design question -- proper (meaningful) content of recipe patches?

[Alexander Kanavin]

On 03/22/2017 03:22 PM, Robert P. J. Day wrote:
>   in any event, i can appreciate the value of sometimes adding
> numbering to patches to enforce a processing order, but when one has a
> numbered series of patches getting up to 20 or 25, that strikes me as
> time to collapse all that into larger and more modular and more
> meaningful patches.
>
>   thoughts?

Why are the patches not in the upstream git repo or tarball release? If 
the upstream is refusing to take it, then someone needs to fork a repo, 
and place all those patches there, and drop them from the recipe.

Otherwise it will be a tremendous maintenance headache each time that 
patch series needs to be rebased to a new upstream release. But whoever 
wrote the recipe that way basically asked for it, no pity for them :)

Alex

Re: another design question -- proper (meaningful) content of recipe patches?

[Burton, Ross]

[-- Attachment #1: Type: text/plain, Size: 517 bytes --]

On 22 March 2017 at 13:22, Robert P. J. Day <rpjday@crashcourse.ca> wrote:

>   in any event, i can appreciate the value of sometimes adding
> numbering to patches to enforce a processing order, but when one has a
> numbered series of patches getting up to 20 or 25, that strikes me as
> time to collapse all that into larger and more modular and more
> meaningful patches.
>

Note that by the naming you can tell that the patches came from
git-format-patch, so the origin of them is a git branch.

Ross

[-- Attachment #2: Type: text/html, Size: 929 bytes --]

Re: another design question -- proper (meaningful) content of recipe patches?

[Bruce Ashfield]

[-- Attachment #1: Type: text/plain, Size: 5339 bytes --]

On Wed, Mar 22, 2017 at 9:22 AM, Robert P. J. Day <rpjday@crashcourse.ca>
wrote:

>
>   got into an animated discussion the other day regarding proper
> design of patch files associated with a recipe. as a demo, i can
> actually use part of a recipe file ripped from the meta-digi-arm
> layer, for "kernel-module-qualcomm", whose recipe file contains (in
> part):
>
>   SRC_URI = " \
>     ${CAF_MIRROR};destsuffix=${PV};branch=${SRCBRANCH} \
>     file://qualcomm-pre-up \
>     file://modprobe-qualcomm.conf \
>     file://0001-qcacld-Fix-compiling-errors-when-BUILD_DEBUG_VERSION.patch
> \
>     file://0002-Update-cfg80211_vendor_event_alloc-call-for-newer-ke.patch
> \
>     file://0003-wlan_hdd_main-Update-cfg80211_ap_stopped-to-nl80211_.patch
> \
>     file://0004-qcacld-2.0-remove-unused-code.patch \
>     file://0005-Including-header-file-for-regulatory_hint_user.patch \
>     file://0006-Updating-calls-to-alloc_netdev_mq.patch \
>     file://0007-wlan_hdd_cfg80211-update-cfg80211_inform_bss-params-.patch
> \
>     file://0008-wlan_hdd_p2p-Update-call-to-cfg80211_rx_mgmt-for-dif.patch
> \
>     file://0009-linux_ac-Fix-for-f_dentry.patch \
>     file://0010-native_sdio-src-hif-Do-not-call-to-HIGH-SPEED-functi.patch
> \
>     file://0011-osdep_adf.h-fix-for-undefined-ath_sysctl_pktlog_size.patch
> \
>     file://0012-Kbuild-Add-compilation-flag-based-on-kernel-support.patch
> \
>     file://0013-Kbuild-do-not-compile-the-DEBUG-version-inconditiona.patch
> \
>     file://0014-Kbuild-Group-most-of-the-relevant-DEBUG-options.patch \
>     file://0015-wlan_hdd_cfg80211-fix-missing-ifdef-clause.patch \
>     file://0016-Add-.gitignore-rules.patch \
>     file://0017-wlan_hdd_main-initialize-all-adapter-completion-vari.patch
> \
>     file://0018-qcacld-Indicate-disconnect-event-to-upper-layers.patch \
>     file://0019-wdd_hdd_main-Print-con_mode-to-clearly-see-if-in-FTM.patch
> \
>     file://0020-Makefile-Pass-BUILD_DEBUG_VERSION-to-kbuild-system.patch \
>     file://0021-cosmetic-change-log-level.patch \
>     file://0022-fix-issue-with-_scan_callback.patch \
>   "
>
>   when i was presented with something resembling the above the other
> day, my immediate reaction was , "yuck." at the time, i opined that,
> if the purpose of the above was to simply add a qualcomm module, it
> was silly to do that with a lengthy, numbered series of patches that
> were almost certainly inter-dependent and even numbered to enforce a
> particular processing order.
>
>   i insisted that the above was unnecessarily messy and, if a
> developer tried to examine the patches, it would be next to impossible
> to do that easily, as it would require walking through each patch,
> then mentally adding the next one, and so on and so on.
>
>   i suggested that, in cases like this, adding a well-defined
> functionality should be done with a single all-encompassing patch or,
> at the very worst, a small number of patches that might be applied
> independently. as in, "Add-qualcomm-driver.patch".
>
>   the argument i got coming back was that the above made sense as it
> reflected the *history* of how that driver came to be added. i
> shuddered, and pointed out that if one wanted to see the history,
> that's what version control is for -- there is no reason that the
> information from version control should be used *verbatim* to generate
> the patches for a recipe.
>

If the patches have been posted (or are otherwise public), then tracking
the history
on the same granularity is a good thing. It helps maintenance and
debugging.

By having the functionality in an "upstream acceptable" history, i.e. one
you'd submit
to the mainline kernel, you have a level of bisectability and other debug
techniques
that will work.

If it is tracking development history, that would be squashed for an
upstream
submission .. then yes, you don't need it in the patches either.

But really, if there are a lot of patches, then they should already be in a
repo and
built from that repo directly (versus exporting patches and maintaining them
separately). But perhaps the export is done to save time and the size of the
build, etc, etc.

Bruce


>
>   in any event, i can appreciate the value of sometimes adding
> numbering to patches to enforce a processing order, but when one has a
> numbered series of patches getting up to 20 or 25, that strikes me as
> time to collapse all that into larger and more modular and more
> meaningful patches.
>
>   thoughts?
>
> rday
>
> --
>
> ========================================================================
> Robert P. J. Day                                 Ottawa, Ontario, CANADA
>                         http://crashcourse.ca
>
> Twitter:                                       http://twitter.com/rpjday
> LinkedIn:                               http://ca.linkedin.com/in/rpjday
> ========================================================================
>
>
> --
> _______________________________________________
> Openembedded-core mailing list
> Openembedded-core@lists.openembedded.org
> http://lists.openembedded.org/mailman/listinfo/openembedded-core
>



-- 
"Thou shalt not follow the NULL pointer, for chaos and madness await thee
at its end"

[-- Attachment #2: Type: text/html, Size: 7054 bytes --]