[Buildroot] [PATCH 1/4] core/pkg-infra: introduce download features concept

[Buildroot] [PATCH 1/4] core/pkg-infra: introduce download features concept

[Vincent Fazio]

Signed-off-by: Vincent Fazio <vfazio@xes-inc.com>
---
 package/pkg-download.mk     | 1 +
 package/pkg-generic.mk      | 7 +++++++
 support/download/dl-wrapper | 8 +++++---
 3 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/package/pkg-download.mk b/package/pkg-download.mk
index de619ba90a..889ff57ded 100644
--- a/package/pkg-download.mk
+++ b/package/pkg-download.mk
@@ -108,6 +108,7 @@ define DOWNLOAD
 		-n '$($(2)_BASENAME_RAW)' \
 		-N '$($(2)_RAWNAME)' \
 		-o '$($(2)_DL_DIR)/$(notdir $(1))' \
+		-x '$($(2)_DL_FEATURES)' \
 		$(if $($(2)_GIT_SUBMODULES),-r) \
 		$(foreach uri,$(call DOWNLOAD_URIS,$(1),$(2)),-u $(uri)) \
 		$(QUIET) \
diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
index 268d999efb..3ea818fc15 100644
--- a/package/pkg-generic.mk
+++ b/package/pkg-generic.mk
@@ -567,6 +567,13 @@ ifndef $(2)_DL_OPTS
  endif
 endif
 
+ifndef $(2)_DL_FEATURES
+ ifdef $(3)_DL_FEATURES
+  $(2)_DL_FEATURES = $$($(3)_DL_FEATURES)
+ endif
+endif
+
+
 ifneq ($$(filter bzr cvs hg svn,$$($(2)_SITE_METHOD)),)
 BR_NO_CHECK_HASH_FOR += $$($(2)_SOURCE)
 endif
diff --git a/support/download/dl-wrapper b/support/download/dl-wrapper
index 3315bd410e..b77ee06b86 100755
--- a/support/download/dl-wrapper
+++ b/support/download/dl-wrapper
@@ -17,15 +17,15 @@
 # We want to catch any unexpected failure, and exit immediately.
 set -e
 
-export BR_BACKEND_DL_GETOPTS=":hc:d:o:n:N:H:ru:qf:e"
+export BR_BACKEND_DL_GETOPTS=":hc:d:o:n:N:H:ru:qf:x:e"
 
 main() {
     local OPT OPTARG
-    local backend output hfile recurse quiet rc
+    local backend output hfile recurse quiet rc features
     local -a uris
 
     # Parse our options; anything after '--' is for the backend
-    while getopts ":c:d:D:o:n:N:H:rf:u:q" OPT; do
+    while getopts ":c:d:D:o:n:N:H:rf:u:x:q" OPT; do
         case "${OPT}" in
         c)  cset="${OPTARG}";;
         d)  dl_dir="${OPTARG}";;
@@ -37,6 +37,7 @@ main() {
         r)  recurse="-r";;
         f)  filename="${OPTARG}";;
         u)  uris+=( "${OPTARG}" );;
+        x)  features="${OPTARG}";;
         q)  quiet="-q";;
         :)  error "option '%s' expects a mandatory argument\n" "${OPTARG}";;
         \?) error "unknown option '%s'\n" "${OPTARG}";;
@@ -127,6 +128,7 @@ main() {
                 -f "${filename}" \
                 -u "${uri}" \
                 -o "${tmpf}" \
+                -x "${features}" \
                 ${quiet} ${recurse} -- "${@}"
         then
             # cd back to keep path coherence
-- 
2.25.0

[Buildroot] [PATCH 2/4] support/download/git: support download features

[Vincent Fazio]

Initial support is limited to submodules.

Signed-off-by: Vincent Fazio <vfazio@xes-inc.com>
---
 support/download/git | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/support/download/git b/support/download/git
index 075f665bbf..eb1778a2f9 100755
--- a/support/download/git
+++ b/support/download/git
@@ -43,8 +43,16 @@ _on_error() {
     exec "${myname}" "${OPTS[@]}" || exit ${ret}
 }
 
+feature_requested() {
+    local feature="${1}"
+    local features_requested="${2}"
+
+    grep -qow "${feature}" <<<"${features_requested}"
+}
+
 verbose=
 recurse=0
+features=
 while getopts "${BR_BACKEND_DL_GETOPTS}" OPT; do
     case "${OPT}" in
     q)  verbose=-q; exec >/dev/null;;
@@ -54,6 +62,7 @@ while getopts "${BR_BACKEND_DL_GETOPTS}" OPT; do
     c)  cset="${OPTARG}";;
     d)  dl_dir="${OPTARG}";;
     n)  basename="${OPTARG}";;
+    x)  features="${OPTARG}";;
     :)  printf "option '%s' expects a mandatory argument\n" "${OPTARG}"; exit 1;;
     \?) printf "unknown option '%s'\n" "${OPTARG}" >&2; exit 1;;
     esac
@@ -75,6 +84,9 @@ _git() {
     eval GIT_DIR="${git_cache}/.git" ${GIT} "${@}"
 }
 
+# check for requested features we support
+if feature_requested "submodules" "${features}"; then recurse=1; fi
+
 # Create a warning file, that the user should not use the git cache.
 # It's ours. Our precious.
 cat <<-_EOF_ >"${dl_dir}/git.readme"
-- 
2.25.0

[Buildroot] [PATCH 3/4] core/pkg-infra: deprecate GIT_SUBMODULES

[Vincent Fazio]

`${PKG}_DL_FEATURES = submodules` is the new method to indicate the need
for git submodules. Any package with ${PKG}_GIT_SUBMODULES set will
implicitly define this value with a warning that the package should be
updated to use the feature flags.

Update documentation to note this change.

Signed-off-by: Vincent Fazio <vfazio@xes-inc.com>
---
 docs/manual/adding-packages-generic.txt | 13 +++++++------
 docs/manual/migrating.txt               | 15 +++++++++++++++
 package/pkg-download.mk                 |  1 -
 package/pkg-generic.mk                  |  4 ++++
 support/download/dl-wrapper             |  9 ++++-----
 support/download/git                    |  1 -
 6 files changed, 30 insertions(+), 13 deletions(-)

diff --git a/docs/manual/adding-packages-generic.txt b/docs/manual/adding-packages-generic.txt
index baa052e31c..fba3fc84aa 100644
--- a/docs/manual/adding-packages-generic.txt
+++ b/docs/manual/adding-packages-generic.txt
@@ -335,12 +335,13 @@ not and can not work as people would expect it should:
      still patch the source code, use +LIBFOO_POST_RSYNC_HOOKS+, see
      xref:hooks-rsync[].
 
-* +LIBFOO_GIT_SUBMODULES+ can be set to +YES+ to create an archive
-  with the git submodules in the repository.  This is only available
-  for packages downloaded with git (i.e. when
-  +LIBFOO_SITE_METHOD=git+).  Note that we try not to use such git
-  submodules when they contain bundled libraries, in which case we
-  prefer to use those libraries from their own package.
+* +LIBFOO_DL_FEATURES+ allows optional features to be specified to the
+  downloading backend. Currently the following are supported:
+  ** +submodules+ will create an archive with the git submodules in the
+     repository.  This is only available for packages downloaded with
+     git (i.e. when +LIBFOO_SITE_METHOD=git+).  Note that we try not to
+     use such git submodules when they contain bundled libraries, in
+     which case we prefer to use those libraries from their own package.
 
 * +LIBFOO_STRIP_COMPONENTS+ is the number of leading components
   (directories) that tar must strip from file names on extraction.
diff --git a/docs/manual/migrating.txt b/docs/manual/migrating.txt
index 92e487c71e..27f53f71a1 100644
--- a/docs/manual/migrating.txt
+++ b/docs/manual/migrating.txt
@@ -56,3 +56,18 @@ Whenever a package installs an executable that is linked with a library
 in +$(HOST_DIR)/lib+, it must have an RPATH pointing to that directory.
 
 An RPATH pointing to +$(HOST_DIR)/usr/lib+ is no longer accepted.
+
+[[migrating-git-submodules]]
+=== Migrating to 2020.02
+
+Before Buildroot 2020.02, packages that required git submodules would indicate
+the requirement by defining:
+---------------------
+LIBFOO_GIT_SUBMODULES = YES
+---------------------
+
+Submodule support has been made a feature flag supported by the git downloading
+mechanism. Packages should now define the following:
+---------------------
+LIBFOO_DL_FEATURES = submodules
+---------------------
diff --git a/package/pkg-download.mk b/package/pkg-download.mk
index 889ff57ded..424880b51d 100644
--- a/package/pkg-download.mk
+++ b/package/pkg-download.mk
@@ -109,7 +109,6 @@ define DOWNLOAD
 		-N '$($(2)_RAWNAME)' \
 		-o '$($(2)_DL_DIR)/$(notdir $(1))' \
 		-x '$($(2)_DL_FEATURES)' \
-		$(if $($(2)_GIT_SUBMODULES),-r) \
 		$(foreach uri,$(call DOWNLOAD_URIS,$(1),$(2)),-u $(uri)) \
 		$(QUIET) \
 		-- \
diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
index 3ea818fc15..d26a6b2bbf 100644
--- a/package/pkg-generic.mk
+++ b/package/pkg-generic.mk
@@ -584,6 +584,10 @@ ifneq ($$($(2)_GIT_SUBMODULES),)
   $$(error $(2) declares having git sub-modules, but does not use the \
 	   'git' method (uses '$$($(2)_SITE_METHOD)' instead))
  endif
+ # Transitional: set the submodules feature based on the old GIT_SUBMODULES flag
+ $$(warning $(2)_GIT_SUBMODULES is defined, this package should transition \
+	 to $(2)_DL_FEATURES = submodules)
+ $(2)_DL_FEATURES += submodules
 endif
 
 ifeq ($$($(2)_SITE_METHOD),local)
diff --git a/support/download/dl-wrapper b/support/download/dl-wrapper
index b77ee06b86..855b269a81 100755
--- a/support/download/dl-wrapper
+++ b/support/download/dl-wrapper
@@ -17,15 +17,15 @@
 # We want to catch any unexpected failure, and exit immediately.
 set -e
 
-export BR_BACKEND_DL_GETOPTS=":hc:d:o:n:N:H:ru:qf:x:e"
+export BR_BACKEND_DL_GETOPTS=":hc:d:o:n:N:H:u:qf:x:e"
 
 main() {
     local OPT OPTARG
-    local backend output hfile recurse quiet rc features
+    local backend output hfile quiet rc features
     local -a uris
 
     # Parse our options; anything after '--' is for the backend
-    while getopts ":c:d:D:o:n:N:H:rf:u:x:q" OPT; do
+    while getopts ":c:d:D:o:n:N:H:f:u:x:q" OPT; do
         case "${OPT}" in
         c)  cset="${OPTARG}";;
         d)  dl_dir="${OPTARG}";;
@@ -34,7 +34,6 @@ main() {
         n)  raw_base_name="${OPTARG}";;
         N)  base_name="${OPTARG}";;
         H)  hfile="${OPTARG}";;
-        r)  recurse="-r";;
         f)  filename="${OPTARG}";;
         u)  uris+=( "${OPTARG}" );;
         x)  features="${OPTARG}";;
@@ -129,7 +128,7 @@ main() {
                 -u "${uri}" \
                 -o "${tmpf}" \
                 -x "${features}" \
-                ${quiet} ${recurse} -- "${@}"
+                ${quiet} -- "${@}"
         then
             # cd back to keep path coherence
             cd "${OLDPWD}"
diff --git a/support/download/git b/support/download/git
index eb1778a2f9..add9c4bfbb 100755
--- a/support/download/git
+++ b/support/download/git
@@ -56,7 +56,6 @@ features=
 while getopts "${BR_BACKEND_DL_GETOPTS}" OPT; do
     case "${OPT}" in
     q)  verbose=-q; exec >/dev/null;;
-    r)  recurse=1;;
     o)  output="${OPTARG}";;
     u)  uri="${OPTARG}";;
     c)  cset="${OPTARG}";;
-- 
2.25.0

[Buildroot] [PATCH 4/4] package/*: migrate GIT_SUBMODULES to DL_FEATURES

[Vincent Fazio]

Update the submodule test package as well.

Signed-off-by: Vincent Fazio <vfazio@xes-inc.com>
---
 package/azure-iot-sdk-c/azure-iot-sdk-c.mk                      | 2 +-
 package/brickd/brickd.mk                                        | 2 +-
 package/c-capnproto/c-capnproto.mk                              | 2 +-
 package/gstreamer1/gst1-interpipe/gst1-interpipe.mk             | 2 +-
 package/gstreamer1/gst1-shark/gst1-shark.mk                     | 2 +-
 .../package/git-submodule-enabled/git-submodule-enabled.mk      | 2 +-
 6 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/package/azure-iot-sdk-c/azure-iot-sdk-c.mk b/package/azure-iot-sdk-c/azure-iot-sdk-c.mk
index 8b3f670399..ec28010cfd 100644
--- a/package/azure-iot-sdk-c/azure-iot-sdk-c.mk
+++ b/package/azure-iot-sdk-c/azure-iot-sdk-c.mk
@@ -7,7 +7,7 @@
 AZURE_IOT_SDK_C_VERSION = 2018-12-13
 AZURE_IOT_SDK_C_SITE = https://github.com/Azure/azure-iot-sdk-c
 AZURE_IOT_SDK_C_SITE_METHOD = git
-AZURE_IOT_SDK_C_GIT_SUBMODULES = YES
+AZURE_IOT_SDK_C_DL_FEATURES = submodules
 AZURE_IOT_SDK_C_LICENSE = MIT
 AZURE_IOT_SDK_C_LICENSE_FILES = LICENSE
 AZURE_IOT_SDK_C_INSTALL_STAGING = YES
diff --git a/package/brickd/brickd.mk b/package/brickd/brickd.mk
index 7fe28daf78..682273b31b 100644
--- a/package/brickd/brickd.mk
+++ b/package/brickd/brickd.mk
@@ -7,7 +7,7 @@
 BRICKD_VERSION = ev3dev-stretch/1.2.1
 BRICKD_SITE = https://github.com/ev3dev/brickd
 BRICKD_SITE_METHOD = git
-BRICKD_GIT_SUBMODULES = YES
+BRICKD_DL_FEATURES = submodules
 
 BRICKD_LICENSE = GPL-2.0
 BRICKD_LICENSE_FILES = LICENSE.txt
diff --git a/package/c-capnproto/c-capnproto.mk b/package/c-capnproto/c-capnproto.mk
index c144a4becc..538430095c 100644
--- a/package/c-capnproto/c-capnproto.mk
+++ b/package/c-capnproto/c-capnproto.mk
@@ -7,7 +7,7 @@
 C_CAPNPROTO_VERSION = 9053ebe6eeb2ae762655b982e27c341cb568366d
 C_CAPNPROTO_SITE = https://github.com/opensourcerouting/c-capnproto.git
 C_CAPNPROTO_SITE_METHOD = git
-C_CAPNPROTO_GIT_SUBMODULES = YES
+C_CAPNPROTO_DL_FEATURES = submodules
 C_CAPNPROTO_LICENSE = MIT
 C_CAPNPROTO_LICENSE_FILES = COPYING
 C_CAPNPROTO_INSTALL_STAGING = YES
diff --git a/package/gstreamer1/gst1-interpipe/gst1-interpipe.mk b/package/gstreamer1/gst1-interpipe/gst1-interpipe.mk
index 87e5f2e5d2..5a125726d3 100644
--- a/package/gstreamer1/gst1-interpipe/gst1-interpipe.mk
+++ b/package/gstreamer1/gst1-interpipe/gst1-interpipe.mk
@@ -8,7 +8,7 @@ GST1_INTERPIPE_VERSION = 9af5b40d106f35ce75f8baa5efc8c59fc5f7eda1
 GST1_INTERPIPE_SITE = https://github.com/RidgeRun/gst-interpipe
 GST1_INTERPIPE_SITE_METHOD = git
 # fetch gst-interpipe/common sub module
-GST1_INTERPIPE_GIT_SUBMODULES = YES
+GST1_INTERPIPE_DL_FEATURES = submodules
 
 GST1_INTERPIPE_LICENSE = LGPL-2.1
 GST1_INTERPIPE_LICENSE_FILES = COPYING
diff --git a/package/gstreamer1/gst1-shark/gst1-shark.mk b/package/gstreamer1/gst1-shark/gst1-shark.mk
index 6dc702b482..1cd0681c85 100644
--- a/package/gstreamer1/gst1-shark/gst1-shark.mk
+++ b/package/gstreamer1/gst1-shark/gst1-shark.mk
@@ -7,7 +7,7 @@
 GST1_SHARK_VERSION = v0.6.1
 GST1_SHARK_SITE =  https://github.com/RidgeRun/gst-shark.git
 GST1_SHARK_SITE_METHOD = git
-GST1_SHARK_GIT_SUBMODULES = YES
+GST1_SHARK_DL_FEATURES = submodules
 
 GST1_SHARK_LICENSE = LGPL-2.1+
 GST1_SHARK_LICENSE_FILES = COPYING
diff --git a/support/testing/tests/download/br2-external/git-refs/package/git-submodule-enabled/git-submodule-enabled.mk b/support/testing/tests/download/br2-external/git-refs/package/git-submodule-enabled/git-submodule-enabled.mk
index 48a42f5e8a..90cddbc434 100644
--- a/support/testing/tests/download/br2-external/git-refs/package/git-submodule-enabled/git-submodule-enabled.mk
+++ b/support/testing/tests/download/br2-external/git-refs/package/git-submodule-enabled/git-submodule-enabled.mk
@@ -6,6 +6,6 @@
 
 GIT_SUBMODULE_ENABLED_VERSION = a9dbc1e23c45e8e1b88c0448763f54d714eb6f8f
 GIT_SUBMODULE_ENABLED_SITE = git://localhost:$(GITREMOTE_PORT_NUMBER)/repo.git
-GIT_SUBMODULE_ENABLED_GIT_SUBMODULES = YES
+GIT_SUBMODULE_ENABLED_DL_FEATURES = submodules
 
 $(eval $(generic-package))
-- 
2.25.0

[Buildroot] [PATCH 1/4] core/pkg-infra: introduce download features concept

[Ryan Barnett]

Vincent,

Please see my few comments while just quickly looking over this patchset.

On Thu, Jan 16, 2020 at 1:39 PM Vincent Fazio <vfazio@xes-inc.com> wrote:
>

There should be more of a commit message which describe the feature
being added here. Initially I was unsure what was being proposed here
without looking at the changes.

Please also include updating the buildroot documentation as well to
describe the DL_FEATURES variable as well in this commit instead of
when deprecating the git submodules.

> Signed-off-by: Vincent Fazio <vfazio@xes-inc.com>
> ---
>  package/pkg-download.mk     | 1 +
>  package/pkg-generic.mk      | 7 +++++++
>  support/download/dl-wrapper | 8 +++++---
>  3 files changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/package/pkg-download.mk b/package/pkg-download.mk
> index de619ba90a..889ff57ded 100644
> --- a/package/pkg-download.mk
> +++ b/package/pkg-download.mk
> @@ -108,6 +108,7 @@ define DOWNLOAD
>                 -n '$($(2)_BASENAME_RAW)' \
>                 -N '$($(2)_RAWNAME)' \
>                 -o '$($(2)_DL_DIR)/$(notdir $(1))' \
> +               -x '$($(2)_DL_FEATURES)' \
>                 $(if $($(2)_GIT_SUBMODULES),-r) \
>                 $(foreach uri,$(call DOWNLOAD_URIS,$(1),$(2)),-u $(uri)) \
>                 $(QUIET) \
> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> index 268d999efb..3ea818fc15 100644
> --- a/package/pkg-generic.mk
> +++ b/package/pkg-generic.mk
> @@ -567,6 +567,13 @@ ifndef $(2)_DL_OPTS
>   endif
>  endif
>
> +ifndef $(2)_DL_FEATURES
> + ifdef $(3)_DL_FEATURES
> +  $(2)_DL_FEATURES = $$($(3)_DL_FEATURES)
> + endif
> +endif
> +
> +

Remove extra new line.

[...]

Thanks,
-Ryan

---
Ryan Barnett | Sr Systems Engineer | Commercial Avionics
COLLINS AEROSPACE
400 Collins Rd NE, Cedar Rapids, IA 52498 USA
ryan.barnett at collins.com | collinsaerospace.com

CONFIDENTIALITY WARNING: This message may contain proprietary and/or
privileged information of Collins Aerospace and its affiliated
companies. If you are not the intended recipient, please 1) Do not
disclose, copy, distribute or use this message or its contents. 2)
Advise the sender by return email. 3) Delete all copies (including all
attachments) from your computer. Your cooperation is greatly
appreciated.

[Buildroot] [PATCH 1/4] core/pkg-infra: introduce download features concept

[Vincent Fazio]

Ryan,

On 1/16/20 1:49 PM, Ryan Barnett wrote:
> Vincent,
>
> Please see my few comments while just quickly looking over this patchset.
>
> On Thu, Jan 16, 2020 at 1:39 PM Vincent Fazio <vfazio@xes-inc.com> wrote:
> There should be more of a commit message which describe the feature
> being added here. Initially I was unsure what was being proposed here
> without looking at the changes.
Yea, sorry about that. kinda fired it off a bit sooner than I expected. 
I'll expand the commit messages
> Please also include updating the buildroot documentation as well to
> describe the DL_FEATURES variable as well in this commit instead of
> when deprecating the git submodules.
Makes sense
>> Signed-off-by: Vincent Fazio <vfazio@xes-inc.com>
>> ---
>>   package/pkg-download.mk     | 1 +
>>   package/pkg-generic.mk      | 7 +++++++
>>   support/download/dl-wrapper | 8 +++++---
>>   3 files changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/package/pkg-download.mk b/package/pkg-download.mk
>> index de619ba90a..889ff57ded 100644
>> --- a/package/pkg-download.mk
>> +++ b/package/pkg-download.mk
>> @@ -108,6 +108,7 @@ define DOWNLOAD
>>                  -n '$($(2)_BASENAME_RAW)' \
>>                  -N '$($(2)_RAWNAME)' \
>>                  -o '$($(2)_DL_DIR)/$(notdir $(1))' \
>> +               -x '$($(2)_DL_FEATURES)' \
>>                  $(if $($(2)_GIT_SUBMODULES),-r) \
>>                  $(foreach uri,$(call DOWNLOAD_URIS,$(1),$(2)),-u $(uri)) \
>>                  $(QUIET) \
>> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
>> index 268d999efb..3ea818fc15 100644
>> --- a/package/pkg-generic.mk
>> +++ b/package/pkg-generic.mk
>> @@ -567,6 +567,13 @@ ifndef $(2)_DL_OPTS
>>    endif
>>   endif
>>
>> +ifndef $(2)_DL_FEATURES
>> + ifdef $(3)_DL_FEATURES
>> +  $(2)_DL_FEATURES = $$($(3)_DL_FEATURES)
>> + endif
>> +endif
>> +
>> +
> Remove extra new line.
nice catch
>
> [...]
>
> Thanks,
> -Ryan
>
> ---
> Ryan Barnett | Sr Systems Engineer | Commercial Avionics
> COLLINS AEROSPACE
> 400 Collins Rd NE, Cedar Rapids, IA 52498 USA
> ryan.barnett at collins.com | collinsaerospace.com
>
> CONFIDENTIALITY WARNING: This message may contain proprietary and/or
> privileged information of Collins Aerospace and its affiliated
> companies. If you are not the intended recipient, please 1) Do not
> disclose, copy, distribute or use this message or its contents. 2)
> Advise the sender by return email. 3) Delete all copies (including all
> attachments) from your computer. Your cooperation is greatly
> appreciated.
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot

-- 
Vincent Fazio
Embedded Software Engineer - Linux
Extreme Engineering Solutions, Inc
http://www.xes-inc.com

[Buildroot] [PATCH 1/4] core/pkg-infra: introduce download features concept

[Arnout Vandecappelle]

 Hi Vincent,

 Thanks for these patches. Nice idea.


On 16/01/2020 20:53, Vincent Fazio wrote:
> Ryan,
> 
> On 1/16/20 1:49 PM, Ryan Barnett wrote:
>> Vincent,
>>
>> Please see my few comments while just quickly looking over this patchset.
>>
>> On Thu, Jan 16, 2020 at 1:39 PM Vincent Fazio <vfazio@xes-inc.com> wrote:
>> There should be more of a commit message which describe the feature
>> being added here. Initially I was unsure what was being proposed here
>> without looking at the changes.
> Yea, sorry about that. kinda fired it off a bit sooner than I expected. I'll
> expand the commit messages

 It's good to fire it off early (as an RFC) to avoid wasting work if it's going
in the wrong direction.

 ... which I think this is.

 I guess the idea is to generalize the submodules feature so it can be used by
several VCS systems, and to combine it with the transient option.

 However, I don't think the additional complexity is worth it.

 Separate _GIT_SUBMODULES = YES and _DOWNLOAD_TRANSIENT = YES are IMO easier to
understand than _DL_FEATURES = submodules transient, because we don't really
have the latter pattern anywhere else in Buildroot. In addition, it's hard to
remember if it's comma-separated or space-separated.

 I also think the infrastructural code becomes a bit harder to understand
(particularly how the features are parsed out in the git helper script).

 Bottom line: I think this is only worth it if we have at least four or so of
these features.

 Regards,
 Arnout

[Buildroot] [PATCH 1/4] core/pkg-infra: introduce download features concept

[Thomas Petazzoni]

On Thu, 16 Jan 2020 23:59:00 +0100
Arnout Vandecappelle <arnout@mind.be> wrote:

>  It's good to fire it off early (as an RFC) to avoid wasting work if it's going
> in the wrong direction.
> 
>  ... which I think this is.
> 
>  I guess the idea is to generalize the submodules feature so it can be used by
> several VCS systems, and to combine it with the transient option.
> 
>  However, I don't think the additional complexity is worth it.
> 
>  Separate _GIT_SUBMODULES = YES and _DOWNLOAD_TRANSIENT = YES are IMO easier to
> understand than _DL_FEATURES = submodules transient, because we don't really
> have the latter pattern anywhere else in Buildroot. In addition, it's hard to
> remember if it's comma-separated or space-separated.

I asked the exact same question on IRC before Vincent sent his patch
series, and both Yann and me suggested that it should be sent with
actual examples of how the DL_FEATURES mechanism will be used.

The intention of Vincent is not to use this DL_FEATURES variable for
"transient", but rather to implement download method specific things,
and the first example was git-lfs support.

transient would remain a separate variable. transient is independent of
the download method: it only tells the download infrastructure to
ignore the cache, and always redownlaod from upstream, regardless of
the download method.

DL_FEATURES however would be to pass backend-specific flags.

That being said, I agree that with just two flags for the moment
(submodules and git-lfs), it's not clear this is really needed. Also,
this DL_FEATURES mechanism only allows to pass booleans, which works
fine for submodules and git-lfs, but what if we want to pass some
actual value to a download backend (like a timeout value, or some other
thing).

So maybe with just two booleans for now, we should stick to
<pkg>_GIT_SUBMODULES = YES and <pkg>_GIT_LFS = YES ?

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 1/4] core/pkg-infra: introduce download features concept

[Vincent Fazio]

On 1/17/20 2:17 AM, Thomas Petazzoni wrote:
> On Thu, 16 Jan 2020 23:59:00 +0100
> Arnout Vandecappelle <arnout@mind.be> wrote:
>
>>   It's good to fire it off early (as an RFC) to avoid wasting work if it's going
>> in the wrong direction.
>>
>>   ... which I think this is.
>>
>>   I guess the idea is to generalize the submodules feature so it can be used by
>> several VCS systems, and to combine it with the transient option.
>>
>>   However, I don't think the additional complexity is worth it.
>>
>>   Separate _GIT_SUBMODULES = YES and _DOWNLOAD_TRANSIENT = YES are IMO easier to
>> understand than _DL_FEATURES = submodules transient, because we don't really
>> have the latter pattern anywhere else in Buildroot. In addition, it's hard to
>> remember if it's comma-separated or space-separated.
> I asked the exact same question on IRC before Vincent sent his patch
> series, and both Yann and me suggested that it should be sent with
> actual examples of how the DL_FEATURES mechanism will be used.
>
> The intention of Vincent is not to use this DL_FEATURES variable for
> "transient", but rather to implement download method specific things,
> and the first example was git-lfs support.
>
> transient would remain a separate variable. transient is independent of
> the download method: it only tells the download infrastructure to
> ignore the cache, and always redownlaod from upstream, regardless of
> the download method.
>
> DL_FEATURES however would be to pass backend-specific flags.
>
> That being said, I agree that with just two flags for the moment
> (submodules and git-lfs), it's not clear this is really needed. Also,
> this DL_FEATURES mechanism only allows to pass booleans, which works
> fine for submodules and git-lfs, but what if we want to pass some
> actual value to a download backend (like a timeout value, or some other
> thing).
I'm not sure this is tied down to boolean values (you could certainly 
specify timeout=X) but switches passed directly to the backend 
(git/hg/etc) can be specified via <pkg>_DL_OPTS already, right?
> So maybe with just two booleans for now, we should stick to
> <pkg>_GIT_SUBMODULES = YES and <pkg>_GIT_LFS = YES ?
I'm fine with just having git-lfs use <pkg>_GIT_LFS = YES and this 
series can be voided if git-lfs using this define will be accepted. The 
original git-lfs patch submission was stalled and after talking with 
Yann it seemed to be due to the requirement of adding yet another switch 
to the dl-wrapper that only applied to git, so this was proposed as a 
way to generalize it and get away from updating multiple files 
(pkg-generic, pkg-download, dl-wrapper, and the backend) to add a single 
feature.
> Best regards,
>
> Thomas

-- 
Vincent Fazio
Embedded Software Engineer - Linux
Extreme Engineering Solutions, Inc
http://www.xes-inc.com

[Buildroot] [PATCH 1/4] core/pkg-infra: introduce download features concept

[Yann E. MORIN]

Thomas, Arnout, all,

On 2020-01-17 09:17 +0100, Thomas Petazzoni spake thusly:
> On Thu, 16 Jan 2020 23:59:00 +0100
> Arnout Vandecappelle <arnout@mind.be> wrote:
> >  It's good to fire it off early (as an RFC) to avoid wasting work if it's going
> > in the wrong direction.
> > 
> >  ... which I think this is.
> > 
> >  I guess the idea is to generalize the submodules feature so it can be used by
> > several VCS systems, and to combine it with the transient option.
> > 
> >  However, I don't think the additional complexity is worth it.
> > 
> >  Separate _GIT_SUBMODULES = YES and _DOWNLOAD_TRANSIENT = YES are IMO easier to
> > understand than _DL_FEATURES = submodules transient, because we don't really
> > have the latter pattern anywhere else in Buildroot. In addition, it's hard to
> > remember if it's comma-separated or space-separated.
> I asked the exact same question on IRC before Vincent sent his patch
> series, and both Yann and me suggested that it should be sent with
> actual examples of how the DL_FEATURES mechanism will be used.

Which is indeed lacking in this series from Vincent. At least, a cover
letter explaining the goal of the series would have cleared things up a
bit.

> The intention of Vincent is not to use this DL_FEATURES variable for
> "transient", but rather to implement download method specific things,
> and the first example was git-lfs support.

Well, git-lfs would be the second one, git submodules being the first.
;-)

But as I also explained on IRC, this would also be usefull to implement
Merucial forests (there was a patch pending from Thomas DS last year,
for example), of for handling svn externals. That would male four
(possible) users.

> transient would remain a separate variable. transient is independent of
> the download method: it only tells the download infrastructure to
> ignore the cache, and always redownlaod from upstream, regardless of
> the download method.

Totally in-line with this.

> DL_FEATURES however would be to pass backend-specific flags.
> 
> That being said, I agree that with just two flags for the moment
> (submodules and git-lfs), it's not clear this is really needed. Also,
> this DL_FEATURES mechanism only allows to pass booleans, which works
> fine for submodules and git-lfs, but what if we want to pass some
> actual value to a download backend (like a timeout value, or some other
> thing).
> 
> So maybe with just two booleans for now, we should stick to
> <pkg>_GIT_SUBMODULES = YES and <pkg>_GIT_LFS = YES ?

At some point in the past, Arnout expressed some concern about adding yet
more variables, and this feature thingy would be the opportunity to
coalesce many variables into a single one (or rather, avoiud adding new
variables).

Additionally, as as been expresed elsewhere in the thread, this would
not be limited to booleans. The feature string is just that: a string
that is passed as-is to the backend, which is then responsible for
interpreting it.

As an example, this could very well be something like the following, to
limit the submodules to use, as well as enabling lfs and using a program
to ask for credentials (note that this is just to demonstrate the
possibilities, and is completely made-up, even though the submodule list
is probably intreresting in practice):

    FOO_DL_FEATURES = \
        submodule:path/to/one/submodule \
        submodule:path/to/another/submodule \
        lfs:enable=yes \
        lfs:credentials-method=program \
        lfs:credentials-program=/bin/gpg-ask-key

We just have to be pretty careful about what we provision for this
variable...

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH 1/4] core/pkg-infra: introduce download features concept

[Arnout Vandecappelle]

On 17/01/2020 17:44, Yann E. MORIN wrote:

[snip]
> At some point in the past, Arnout expressed some concern about adding yet
> more variables,

 That was more about internal variables, not user-facing variables. And also
about variables that are actually assigned to. Each variable that is set in
inner-generic-package gets multiplied a thousandfold and this puts pressure on
the internal tables in make.

> and this feature thingy would be the opportunity to
> coalesce many variables into a single one (or rather, avoiud adding new
> variables).

 Iff there are several users of it. So, as I said, it's a good idea if we get 4
features.


 Regards,
 Arnout

[Buildroot] [PATCH 1/4] core/pkg-infra: introduce download features concept

[Vincent Fazio]

Arnout, Yann, et al

On 1/20/20 2:56 AM, Arnout Vandecappelle wrote:
>
> On 17/01/2020 17:44, Yann E. MORIN wrote:
>
> [snip]
>> At some point in the past, Arnout expressed some concern about adding yet
>> more variables,
>   That was more about internal variables, not user-facing variables. And also
> about variables that are actually assigned to. Each variable that is set in
> inner-generic-package gets multiplied a thousandfold and this puts pressure on
> the internal tables in make.
>
>> and this feature thingy would be the opportunity to
>> coalesce many variables into a single one (or rather, avoiud adding new
>> variables).
>   Iff there are several users of it. So, as I said, it's a good idea if we get 4
> features.

So it sounds like this is too much infrastructure change until we have 4 
definitive use-cases.

At this time, we only have one (git submodules). My intent with this 
series was to add the foundation for supporting git-lfs (which would be 
the second use-case) and others.

Based on the conversation, it seems like until we have a hard 
requirement from other backends (hg and svn as previously mentioned), 
this series is DOA and that git-lfs support should be added via some 
<pkg>_GIT_LFS define as was originally submitted here: 
http://lists.busybox.net/pipermail/buildroot/2018-April/219716.html

My main goal here is to add git-lfs support, regardless of how that 
needs to happen. If resubmitting the original patch gets that done, 
that's what I'll do.

Please let me know your thoughts.

>
>   Regards,
>   Arnout

-- 
Vincent Fazio
Embedded Software Engineer - Linux
Extreme Engineering Solutions, Inc
http://www.xes-inc.com

[Buildroot] [PATCH 1/4] core/pkg-infra: introduce download features concept

[Arnout Vandecappelle]

On 20/01/2020 21:38, Vincent Fazio wrote:
> Arnout, Yann, et al
> 
> On 1/20/20 2:56 AM, Arnout Vandecappelle wrote:
>>
>> On 17/01/2020 17:44, Yann E. MORIN wrote:
>>
>> [snip]
>>> At some point in the past, Arnout expressed some concern about adding yet
>>> more variables,
>> ? That was more about internal variables, not user-facing variables. And also
>> about variables that are actually assigned to. Each variable that is set in
>> inner-generic-package gets multiplied a thousandfold and this puts pressure on
>> the internal tables in make.
>>
>>> and this feature thingy would be the opportunity to
>>> coalesce many variables into a single one (or rather, avoiud adding new
>>> variables).
>> ? Iff there are several users of it. So, as I said, it's a good idea if we get 4
>> features.
> 
> So it sounds like this is too much infrastructure change until we have 4
> definitive use-cases.
> 
> At this time, we only have one (git submodules). My intent with this series was
> to add the foundation for supporting git-lfs (which would be the second
> use-case) and others.
> 
> Based on the conversation, it seems like until we have a hard requirement from
> other backends (hg and svn as previously mentioned), this series is DOA and that
> git-lfs support should be added via some <pkg>_GIT_LFS define as was originally
> submitted here: http://lists.busybox.net/pipermail/buildroot/2018-April/219716.html
> 
> My main goal here is to add git-lfs support, regardless of how that needs to
> happen. If resubmitting the original patch gets that done, that's what I'll do.

 Yes, I think the original patch should be fine. Just also add git-lfs to
DL_TOOLS_DEPENDENCIES, and test if it still works correctly if you don't have
git-lfs installed on your host.

 Ideally there should also be a package in-tree that uses it, but I can imagine
that it will be hard to find one... Failing that, a test case would be nice -
but again, that may be difficult to implement without relying on some
server-side support for it.

 Regards,
 Arnout


> 
> Please let me know your thoughts.
> 
>>
>> ? Regards,
>> ? Arnout
> 

[Buildroot] [PATCH 1/4] core/pkg-infra: introduce download features concept

[Vincent Fazio]

Arnout,

On 1/20/20 3:07 PM, Arnout Vandecappelle wrote:
>
> On 20/01/2020 21:38, Vincent Fazio wrote:
>> Arnout, Yann, et al
>>
>> On 1/20/20 2:56 AM, Arnout Vandecappelle wrote:
>>> On 17/01/2020 17:44, Yann E. MORIN wrote:
>>>
>>> [snip]
>>>> At some point in the past, Arnout expressed some concern about adding yet
>>>> more variables,
>>>  ? That was more about internal variables, not user-facing variables. And also
>>> about variables that are actually assigned to. Each variable that is set in
>>> inner-generic-package gets multiplied a thousandfold and this puts pressure on
>>> the internal tables in make.
>>>
>>>> and this feature thingy would be the opportunity to
>>>> coalesce many variables into a single one (or rather, avoiud adding new
>>>> variables).
>>>  ? Iff there are several users of it. So, as I said, it's a good idea if we get 4
>>> features.
>> So it sounds like this is too much infrastructure change until we have 4
>> definitive use-cases.
>>
>> At this time, we only have one (git submodules). My intent with this series was
>> to add the foundation for supporting git-lfs (which would be the second
>> use-case) and others.
>>
>> Based on the conversation, it seems like until we have a hard requirement from
>> other backends (hg and svn as previously mentioned), this series is DOA and that
>> git-lfs support should be added via some <pkg>_GIT_LFS define as was originally
>> submitted here: http://lists.busybox.net/pipermail/buildroot/2018-April/219716.html
>>
>> My main goal here is to add git-lfs support, regardless of how that needs to
>> happen. If resubmitting the original patch gets that done, that's what I'll do.
>   Yes, I think the original patch should be fine. Just also add git-lfs to
> DL_TOOLS_DEPENDENCIES, and test if it still works correctly if you don't have
> git-lfs installed on your host.

I have a series staged here with most of that support: 
https://gitlab.com/vfazio/buildroot/merge_requests/4/commits

I opted to create a host package for git-lfs in the case the host was 
missing it.. but can drop that if we'd rather enforce the host to have 
it pre-installed.

I was a bit confused on the difference between DL_TOOLS_DEPENDENCIES and 
<PKG>_DOWNLOAD_DEPENDENCIES and opted for the latter, but can change that.

>   Ideally there should also be a package in-tree that uses it, but I can imagine
> that it will be hard to find one... Failing that, a test case would be nice -
> but again, that may be difficult to implement without relying on some
> server-side support for it.
As you mention, a test case will be difficult without a server 
implementing the protocol and I expect most packages that need this 
functionality will be external tree packages (we need it for vendor 
supplied binaries for which sources are not available).
>   Regards,
>   Arnout
>
>
>> Please let me know your thoughts.
>>
>>>  ? Regards,
>>>  ? Arnout

-- 
Vincent Fazio
Embedded Software Engineer - Linux
Extreme Engineering Solutions, Inc
http://www.xes-inc.com

[Buildroot] [PATCH 1/4] core/pkg-infra: introduce download features concept

[Arnout Vandecappelle]

On 20/01/2020 22:26, Vincent Fazio wrote:
> Arnout,
> 
> On 1/20/20 3:07 PM, Arnout Vandecappelle wrote:
>>
>> On 20/01/2020 21:38, Vincent Fazio wrote:
>>> Arnout, Yann, et al
>>>
>>> On 1/20/20 2:56 AM, Arnout Vandecappelle wrote:
>>>> On 17/01/2020 17:44, Yann E. MORIN wrote:
>>>>
>>>> [snip]
>>>>> At some point in the past, Arnout expressed some concern about adding yet
>>>>> more variables,
>>>> ?? That was more about internal variables, not user-facing variables. And also
>>>> about variables that are actually assigned to. Each variable that is set in
>>>> inner-generic-package gets multiplied a thousandfold and this puts pressure on
>>>> the internal tables in make.
>>>>
>>>>> and this feature thingy would be the opportunity to
>>>>> coalesce many variables into a single one (or rather, avoiud adding new
>>>>> variables).
>>>> ?? Iff there are several users of it. So, as I said, it's a good idea if we
>>>> get 4
>>>> features.
>>> So it sounds like this is too much infrastructure change until we have 4
>>> definitive use-cases.
>>>
>>> At this time, we only have one (git submodules). My intent with this series was
>>> to add the foundation for supporting git-lfs (which would be the second
>>> use-case) and others.
>>>
>>> Based on the conversation, it seems like until we have a hard requirement from
>>> other backends (hg and svn as previously mentioned), this series is DOA and that
>>> git-lfs support should be added via some <pkg>_GIT_LFS define as was originally
>>> submitted here:
>>> http://lists.busybox.net/pipermail/buildroot/2018-April/219716.html
>>>
>>> My main goal here is to add git-lfs support, regardless of how that needs to
>>> happen. If resubmitting the original patch gets that done, that's what I'll do.
>> ? Yes, I think the original patch should be fine. Just also add git-lfs to
>> DL_TOOLS_DEPENDENCIES, and test if it still works correctly if you don't have
>> git-lfs installed on your host.
> 
> I have a series staged here with most of that support:
> https://gitlab.com/vfazio/buildroot/merge_requests/4/commits
> 
> I opted to create a host package for git-lfs in the case the host was missing
> it.. but can drop that if we'd rather enforce the host to have it pre-installed.
> 
> I was a bit confused on the difference between DL_TOOLS_DEPENDENCIES and
> <PKG>_DOWNLOAD_DEPENDENCIES and opted for the latter, but can change that.

 DL_TOOLS_DEPENDENCIES means that we check if the tool is already installed on
the system.

 <PKG>_DOWNLOAD_DEPENDENCIES means that the host-tool will be built (and the one
on the system is never used, unless you add check-host-git-lfs support).

 Personally I think for something like git-lfs we can expect it to be installed
on the system, since it will probably never be used by any in-tree package. IOW,
I think DL_TOOLS_DEPENDENCIES is the easiest and good-enough solution.


>> ? Ideally there should also be a package in-tree that uses it, but I can imagine
>> that it will be hard to find one... Failing that, a test case would be nice -
>> but again, that may be difficult to implement without relying on some
>> server-side support for it.
> As you mention, a test case will be difficult without a server implementing the
> protocol and I expect most packages that need this functionality will be
> external tree packages (we need it for vendor supplied binaries for which
> sources are not available).

 So no testing. Oh well.

 Regards,
 Arnout

[Buildroot] [PATCH 1/4] core/pkg-infra: introduce download features concept

[Vincent Fazio]

Arnout, et al,

I've rejected this series in favor of reintroducing the original patch + 
suggested changes.

http://patchwork.ozlabs.org/patch/1226759/

On 1/21/20 4:21 PM, Arnout Vandecappelle wrote:
>
> On 20/01/2020 22:26, Vincent Fazio wrote:
>> Arnout,
>>
>> On 1/20/20 3:07 PM, Arnout Vandecappelle wrote:
>>> On 20/01/2020 21:38, Vincent Fazio wrote:
>>>> Arnout, Yann, et al
>>>>
>>>> On 1/20/20 2:56 AM, Arnout Vandecappelle wrote:
>>>>> On 17/01/2020 17:44, Yann E. MORIN wrote:
>>>>>
>>>>> [snip]
>>>>>> At some point in the past, Arnout expressed some concern about adding yet
>>>>>> more variables,
>>>>>  ?? That was more about internal variables, not user-facing variables. And also
>>>>> about variables that are actually assigned to. Each variable that is set in
>>>>> inner-generic-package gets multiplied a thousandfold and this puts pressure on
>>>>> the internal tables in make.
>>>>>
>>>>>> and this feature thingy would be the opportunity to
>>>>>> coalesce many variables into a single one (or rather, avoiud adding new
>>>>>> variables).
>>>>>  ?? Iff there are several users of it. So, as I said, it's a good idea if we
>>>>> get 4
>>>>> features.
>>>> So it sounds like this is too much infrastructure change until we have 4
>>>> definitive use-cases.
>>>>
>>>> At this time, we only have one (git submodules). My intent with this series was
>>>> to add the foundation for supporting git-lfs (which would be the second
>>>> use-case) and others.
>>>>
>>>> Based on the conversation, it seems like until we have a hard requirement from
>>>> other backends (hg and svn as previously mentioned), this series is DOA and that
>>>> git-lfs support should be added via some <pkg>_GIT_LFS define as was originally
>>>> submitted here:
>>>> http://lists.busybox.net/pipermail/buildroot/2018-April/219716.html
>>>>
>>>> My main goal here is to add git-lfs support, regardless of how that needs to
>>>> happen. If resubmitting the original patch gets that done, that's what I'll do.
>>>  ? Yes, I think the original patch should be fine. Just also add git-lfs to
>>> DL_TOOLS_DEPENDENCIES, and test if it still works correctly if you don't have
>>> git-lfs installed on your host.
>> I have a series staged here with most of that support:
>> https://gitlab.com/vfazio/buildroot/merge_requests/4/commits
>>
>> I opted to create a host package for git-lfs in the case the host was missing
>> it.. but can drop that if we'd rather enforce the host to have it pre-installed.
>>
>> I was a bit confused on the difference between DL_TOOLS_DEPENDENCIES and
>> <PKG>_DOWNLOAD_DEPENDENCIES and opted for the latter, but can change that.
>   DL_TOOLS_DEPENDENCIES means that we check if the tool is already installed on
> the system.
>
>   <PKG>_DOWNLOAD_DEPENDENCIES means that the host-tool will be built (and the one
> on the system is never used, unless you add check-host-git-lfs support).
>
>   Personally I think for something like git-lfs we can expect it to be installed
> on the system, since it will probably never be used by any in-tree package. IOW,
> I think DL_TOOLS_DEPENDENCIES is the easiest and good-enough solution.
>
>
>>>  ? Ideally there should also be a package in-tree that uses it, but I can imagine
>>> that it will be hard to find one... Failing that, a test case would be nice -
>>> but again, that may be difficult to implement without relying on some
>>> server-side support for it.
>> As you mention, a test case will be difficult without a server implementing the
>> protocol and I expect most packages that need this functionality will be
>> external tree packages (we need it for vendor supplied binaries for which
>> sources are not available).
>   So no testing. Oh well.
>
>   Regards,
>   Arnout

-- 
Vincent Fazio
Embedded Software Engineer - Linux
Extreme Engineering Solutions, Inc
http://www.xes-inc.com