[Buildroot] [PATCH 0/3] download: detect and refuse git branch by name

[Buildroot] [PATCH 0/3] download: detect and refuse git branch by name

[Yann E. MORIN]

Hello All!

This small series documents why using a branch by name is bad and is not
supoprted, and makes the git backend actually detect them and abort if
the version is a branch by name.

The series starts with a little bit of cleanup in the download wrapper.


Regards,
Yann E. MORIN.


The following changes since commit 81ea4a243b63d7bb1fec580910c553af4ae072c1

  package/lttng-tools: bump version to 2.10.5 (2018-08-01 14:28:30 +0200)


are available in the git repository at:

  git://git.buildroot.org/~ymorin/git/buildroot.git

for you to fetch changes up to d1cdffd986d2f2128396aa785d542fdd3d2c90e0

  support/download: detect and abort when using a git branch by name (2018-08-04 18:28:36 +0200)


----------------------------------------------------------------
Yann E. MORIN (3):
      support/download: remove help from wrapper
      docs/manual: expand on why using a branch name is not supported
      support/download: detect and abort when using a git branch by name

 docs/manual/adding-packages-generic.txt | 15 +++++++++--
 support/download/dl-wrapper             | 47 +--------------------------------
 support/download/git                    |  7 +++++
 3 files changed, 21 insertions(+), 48 deletions(-)

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

[Buildroot] [PATCH 1/3] support/download: remove help from wrapper

[Yann E. MORIN]

The download wrapper is a purely internal helper, and is not supposed to
be callable manually. No need to offer some help.

Besides, the help text was way out-dated.

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
---
 support/download/dl-wrapper | 47 +--------------------------------------------
 1 file changed, 1 insertion(+), 46 deletions(-)

diff --git a/support/download/dl-wrapper b/support/download/dl-wrapper
index 4059c37ebc..742bbd5428 100755
--- a/support/download/dl-wrapper
+++ b/support/download/dl-wrapper
@@ -4,8 +4,6 @@
 # Its role is to ensure atomicity when saving downloaded files
 # back to BR2_DL_DIR, and not clutter BR2_DL_DIR with partial,
 # failed downloads.
-#
-# Call it with -h to see some help.
 
 # To avoid cluttering BR2_DL_DIR, we download to a trashable
 # location, namely in $(BUILD_DIR).
@@ -27,9 +25,8 @@ main() {
     local -a uris
 
     # Parse our options; anything after '--' is for the backend
-    while getopts ":hc:d:D:o:n:N:H:rf:u:q" OPT; do
+    while getopts ":c:d:D:o:n:N:H:rf:u:q" OPT; do
         case "${OPT}" in
-        h)  help; exit 0;;
         c)  cset="${OPTARG}";;
         d)  dl_dir="${OPTARG}";;
         D)  old_dl_dir="${OPTARG}";;
@@ -212,48 +209,6 @@ main() {
     return ${rc}
 }
 
-help() {
-    cat <<_EOF_
-NAME
-    ${my_name} - download wrapper for Buildroot
-
-SYNOPSIS
-    ${my_name} [OPTION]... -- [BACKEND OPTION]...
-
-DESCRIPTION
-    Wrapper script around different download mechanisms. Ensures that
-    concurrent downloads do not conflict, that partial downloads are
-    properly evicted without leaving temporary files, and that access
-    rights are maintained.
-
-    -h  This help text.
-
-    -u URIs
-        The URI to get the file from, the URI must respect the format given in
-        the example.
-        You may give as many '-u URI' as you want, the script will stop@the
-        frist successful download.
-
-        Example: backend+URI; git+http://example.com or http+http://example.com
-
-    -o FILE
-        Store the downloaded archive in FILE.
-
-    -H FILE
-        Use FILE to read hashes from, and check them against the downloaded
-        archive.
-
-  Exit status:
-    0   if OK
-    !0  in case of error
-
-ENVIRONMENT
-
-    BUILD_DIR
-        The path to Buildroot's build dir
-_EOF_
-}
-
 trace()  { local msg="${1}"; shift; printf "%s: ${msg}" "${my_name}" "${@}"; }
 warn()   { trace "${@}" >&2; }
 errorN() { local ret="${1}"; shift; warn "${@}"; exit ${ret}; }
-- 
2.14.1

[Buildroot] [PATCH 1/3] support/download: remove help from wrapper

[Thomas Petazzoni]

Hello,

On Sat,  4 Aug 2018 18:33:03 +0200, Yann E. MORIN wrote:
> The download wrapper is a purely internal helper, and is not supposed to
> be callable manually. No need to offer some help.
> 
> Besides, the help text was way out-dated.
> 
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> ---
>  support/download/dl-wrapper | 47 +--------------------------------------------
>  1 file changed, 1 insertion(+), 46 deletions(-)

Applied to next, thanks.

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 2/3] docs/manual: expand on why using a branch name is not supported

[Yann E. MORIN]

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Cc: Thomas De Schampheleire <patrickdepinguin@gmail.com>
---
 docs/manual/adding-packages-generic.txt | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/docs/manual/adding-packages-generic.txt b/docs/manual/adding-packages-generic.txt
index cc91e894bd..b17d225a3e 100644
--- a/docs/manual/adding-packages-generic.txt
+++ b/docs/manual/adding-packages-generic.txt
@@ -200,11 +200,22 @@ information is (assuming the package name is +libfoo+) :
   package. Note that if +HOST_LIBFOO_VERSION+ doesn't exist, it is
   assumed to be the same as +LIBFOO_VERSION+. It can also be a
   revision number or a tag for packages that are fetched directly
-  from their version control system. Do not use a branch name as
-  version; it does not work. Examples:
+  from their version control system. Examples:
   ** a version for a release tarball: +LIBFOO_VERSION = 0.1.2+
   ** a sha1 for a git tree: +LIBFOO_VERSION = cb9d6aa9429e838f0e54faa3d455bcbab5eef057+
   ** a tag for a git tree +LIBFOO_VERSION = v0.1.2+
++
+.Note:
+Using a branch name as +FOO_VERSION+ is not supported, because it does
+not and can not work as people would expect it should:
++
+  1. Due to local caching, Buildroot will not re-fetch the repository,
+     so people that expect to be able to follow the remote repository
+     will be quite surprised and disapointed;
+  2. if the user removes the local cache, then the build is no longer
+     reproducible, because the remote repository may change any time
+     between two builds, and people will be quite surprised and
+     disapointed.
 
 * +LIBFOO_SOURCE+ may contain the name of the tarball of the package,
   which Buildroot will use to download the tarball from
-- 
2.14.1

[Buildroot] [PATCH 2/3] docs/manual: expand on why using a branch name is not supported

[Thomas Petazzoni]

Hello,

On Sat,  4 Aug 2018 18:33:04 +0200, Yann E. MORIN wrote:

> ++
> +.Note:
> +Using a branch name as +FOO_VERSION+ is not supported, because it does
> +not and can not work as people would expect it should:
> ++
> +  1. Due to local caching, Buildroot will not re-fetch the repository,
> +     so people that expect to be able to follow the remote repository
> +     will be quite surprised and disapointed;

disappointed with two "p"

> +  2. if the user removes the local cache, then the build is no longer
> +     reproducible, because the remote repository may change any time
> +     between two builds, and people will be quite surprised and
> +     disapointed.

Ditto here.

But in fact more importantly than removing the local cache, it means
that between two developers building the same Buildroot configuration,
they won't be building the same state of the source code. I think this
is more important than a user voluntarily removing stuff from its local
cache.

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 2/3] docs/manual: expand on why using a branch name is not supported

[Thomas Petazzoni]

Hello Yann,

On Sat,  4 Aug 2018 18:33:04 +0200, Yann E. MORIN wrote:
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> Cc: Thomas De Schampheleire <patrickdepinguin@gmail.com>

Following my comments, I've marked this patch as Changes Requested.

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 3/3] support/download: detect and abort when using a git branch by name

[Yann E. MORIN]

Using a git branch by its name does not work as people expect:

 1. due to local caching, Buildroot will not re-fetch the repository,
    so people that expect to be able to follow the remote repository
    will be quite surprised and disapointed;

 2. if the user removes the local cache, then the build is no longer
    reproducible, because the remote repository may change any time
    between two builds, and people will be quite surprised and
    disapointed.

In either case, users are surprised and disapointed, which is a sad
state of matters. :-(

So, detect if the changeset requested is a branch by name, and abort
in that case.

Note that this only applies to using a branch by name. Any other mean
of using the branch (tag, sha1) is still supported, of course.

Note also that the download wrapper still first tries from the local
cache, then from the primary site (if set), and falls back to trying
the mirror eventually (if set). This is not a problem per-se, because
a malicious user that is capable of pre-seeding either locations with
a matching archive already gamed the system, and there is nothing we
can do to prevent that...

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
 support/download/git | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/support/download/git b/support/download/git
index 11bb52c1e1..28391b908b 100755
--- a/support/download/git
+++ b/support/download/git
@@ -134,6 +134,13 @@ if ! _git rev-parse --quiet --verify "'${cset}^{commit}'" >/dev/null 2>&1; then
     exit 1
 fi
 
+# Check if the changeset is a branch name.
+if _git show-ref "${cset}" |grep -qv refs/tags; then
+    printf "Commit '%s' is a branch name.\n" "${cset}"
+    printf "Using a branch name is not supported.\n"
+    exit 1
+fi
+
 # The new cset we want to checkout might have different submodules, or
 # have sub-dirs converted to/from a submodule. So we would need to
 # deregister _current_ submodules before we checkout.
-- 
2.14.1

[Buildroot] [PATCH 3/3] support/download: detect and abort when using a git branch by name

[Ricardo Martincoski]

Hello,

On Sat, Aug 04, 2018 at 01:33 PM, Yann E. MORIN wrote:

[snip]
> +# Check if the changeset is a branch name.
> +if _git show-ref "${cset}" |grep -qv refs/tags; then

I didn't had time to test it yet.
Special refs still works after this?

Regards,
Ricardo

[Buildroot] [PATCH 3/3] support/download: detect and abort when using a git branch by name

[Yann E. MORIN]

Ricardo, All,

On 2018-08-06 00:14 -0300, Ricardo Martincoski spake thusly:
> On Sat, Aug 04, 2018 at 01:33 PM, Yann E. MORIN wrote:
> > +# Check if the changeset is a branch name.
> > +if _git show-ref "${cset}" |grep -qv refs/tags; then
> 
> I didn't had time to test it yet.
> Special refs still works after this?

It is not supposed to be broken, as those special refs are (AFAIU)
exposed as tags, not heads. But as I don't have access to a server
serving those special refs (support for which I find ugly, btw), I
could not test.

Regards,
Yann E. MORIN.

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

[Buildroot] [PATCH 3/3] support/download: detect and abort when using a git branch by name

[Ricardo Martincoski]

Hello,

On Mon, Aug 06, 2018 at 03:36 PM, Yann E. MORIN wrote:

> On 2018-08-06 00:14 -0300, Ricardo Martincoski spake thusly:
>> On Sat, Aug 04, 2018 at 01:33 PM, Yann E. MORIN wrote:
>> > +# Check if the changeset is a branch name.
>> > +if _git show-ref "${cset}" |grep -qv refs/tags; then
>> 
>> I didn't had time to test it yet.
>> Special refs still works after this?
> 
> It is not supposed to be broken, as those special refs are (AFAIU)
> exposed as tags, not heads. But as I don't have access to a server
> serving those special refs (support for which I find ugly, btw), I
> could not test.

One way to test special refs is by using a github package with some dirty
hacks (I picked a special ref at random by inspecting the output of ls-remote):
$ make defconfig
$ ./utils/config --set-str BR2_BACKUP_SITE ""
$ sed -e '/^TMUX_SITE_METHOD/d' \
      -e 's,^TMUX_VERSION =.*,TMUX_VERSION = refs/pull/1014/head,g' \
      -e 's,^TMUX_SITE.*,TMUX_SITE = https://github.com/tmux/tmux.git\nTMUX_SITE_METHOD = git,g' \
      -i package/tmux/tmux.mk
$ rm -f package/tmux/tmux.hash
$ BR2_DL_DIR=$(mktemp -d) make tmux-dirclean tmux-source
...
Commit 'refs/pull/1014/head' is a branch name.
Using a branch name is not supported.


The other way is by using this series:
http://patchwork.ozlabs.org/project/buildroot/list/?series=44009
current master:
https://gitlab.com/RicardoMartincoski/buildroot/pipelines/27301317
current master + this patch:
https://gitlab.com/RicardoMartincoski/buildroot/pipelines/27301358
It doesn't get to show that special-ref is broken because the tests run
sequentially (to avoid overpopulate the gitlab CI), but it shows (in the
-build.log) that also the download of a sha1 tip of a branch (search for
"git-sha1-branch-head" in the log) would be broken with this patch.
This can be reproduced locally:
$ make defconfig
$ ./utils/config --set-str BR2_BACKUP_SITE ""
$ BR2_DL_DIR=$(mktemp -d) make tremor-dirclean tremor-source
...
Commit '7c30a66346199f3f09017a09567c6c8a3a0eedc8' is a branch name.
Using a branch name is not supported.


So NACK for this patch as-is.
Of course, I am not against the concept of detecting the name of a branch being
used as package version and aborting the download with a nice message.
Unfortunately I currently don't have suggestions on how to fix the code.

Regards,
Ricardo

[Buildroot] [PATCH 3/3] support/download: detect and abort when using a git branch by name

[Yann E. MORIN]

Ricardo, All,

On 2018-08-06 21:39 -0300, Ricardo Martincoski spake thusly:
> On Mon, Aug 06, 2018 at 03:36 PM, Yann E. MORIN wrote:
> > On 2018-08-06 00:14 -0300, Ricardo Martincoski spake thusly:
> >> On Sat, Aug 04, 2018 at 01:33 PM, Yann E. MORIN wrote:
> >> > +# Check if the changeset is a branch name.
> >> > +if _git show-ref "${cset}" |grep -qv refs/tags; then
> >> 
> >> I didn't had time to test it yet.
> >> Special refs still works after this?
> > 
> > It is not supposed to be broken, as those special refs are (AFAIU)
> > exposed as tags, not heads. But as I don't have access to a server
> > serving those special refs (support for which I find ugly, btw), I
> > could not test.
> 
> One way to test special refs is by using a github package with some dirty
> hacks (I picked a special ref at random by inspecting the output of ls-remote):
> $ make defconfig
> $ ./utils/config --set-str BR2_BACKUP_SITE ""
> $ sed -e '/^TMUX_SITE_METHOD/d' \
>       -e 's,^TMUX_VERSION =.*,TMUX_VERSION = refs/pull/1014/head,g' \
>       -e 's,^TMUX_SITE.*,TMUX_SITE = https://github.com/tmux/tmux.git\nTMUX_SITE_METHOD = git,g' \
>       -i package/tmux/tmux.mk
> $ rm -f package/tmux/tmux.hash
> $ BR2_DL_DIR=$(mktemp -d) make tmux-dirclean tmux-source
> ...
> Commit 'refs/pull/1014/head' is a branch name.
> Using a branch name is not supported.

But then, the special refs also suffer from the same problems as
branches do: they can't work for the very same reasons as explained in
the previous patch.

So, I would be of the opinion that we should just drop support for those
special refs altogether, based on the same explanations.

Now, I do understand that one will want to be able to test github MRs,
or gerrit reviews or whatnots... But in that case, we already civer this
by way of local.mk and overide-srcdir.

So, I would argue (as I always had since we introduced this special refs
support) that this should better be handled by an upper layer.

Sure, you'd argue that an automated build job could do the build. But
you anyway have to write some scripting for that automated job anyway.
Just have it prepare a git clone of the affected package, checkout the
correct commit, and prepare a local.mk with the correct override-srcdir
befor attempting the buildroot build.

Regards,
Yann E. MORIN.

> The other way is by using this series:
> http://patchwork.ozlabs.org/project/buildroot/list/?series=44009
> current master:
> https://gitlab.com/RicardoMartincoski/buildroot/pipelines/27301317
> current master + this patch:
> https://gitlab.com/RicardoMartincoski/buildroot/pipelines/27301358
> It doesn't get to show that special-ref is broken because the tests run
> sequentially (to avoid overpopulate the gitlab CI), but it shows (in the
> -build.log) that also the download of a sha1 tip of a branch (search for
> "git-sha1-branch-head" in the log) would be broken with this patch.
> This can be reproduced locally:
> $ make defconfig
> $ ./utils/config --set-str BR2_BACKUP_SITE ""
> $ BR2_DL_DIR=$(mktemp -d) make tremor-dirclean tremor-source
> ...
> Commit '7c30a66346199f3f09017a09567c6c8a3a0eedc8' is a branch name.
> Using a branch name is not supported.
> 
> 
> So NACK for this patch as-is.
> Of course, I am not against the concept of detecting the name of a branch being
> used as package version and aborting the download with a nice message.
> Unfortunately I currently don't have suggestions on how to fix the code.
> 
> Regards,
> Ricardo


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

[Buildroot] [PATCH 3/3] support/download: detect and abort when using a git branch by name

[Thomas Petazzoni]

Yann, Ricardo,

On Sun, 12 Aug 2018 22:25:43 +0200, Yann E. MORIN wrote:

> > Commit 'refs/pull/1014/head' is a branch name.
> > Using a branch name is not supported.  
> 
> But then, the special refs also suffer from the same problems as
> branches do: they can't work for the very same reasons as explained in
> the previous patch.
> 
> So, I would be of the opinion that we should just drop support for those
> special refs altogether, based on the same explanations.
> 
> Now, I do understand that one will want to be able to test github MRs,
> or gerrit reviews or whatnots... But in that case, we already civer this
> by way of local.mk and overide-srcdir.
> 
> So, I would argue (as I always had since we introduced this special refs
> support) that this should better be handled by an upper layer.
> 
> Sure, you'd argue that an automated build job could do the build. But
> you anyway have to write some scripting for that automated job anyway.
> Just have it prepare a git clone of the affected package, checkout the
> correct commit, and prepare a local.mk with the correct override-srcdir
> befor attempting the buildroot build.

I don't remember all the discussion about special refs, but if I
understand correctly, it's for example a Github pull request. And if I
understood how Github works, you can push a new version of a pull
request as the same pull request number. This means that what you fetch
from a pull request is not stable. So it suffers from the same
reproducibility issue as regular branches.

Therefore, if we decide to officially not support branches, then we
should also not support special refs. Unless of course I misunderstood
the use case of special refs.

> > The other way is by using this series:
> > http://patchwork.ozlabs.org/project/buildroot/list/?series=44009
> > current master:
> > https://gitlab.com/RicardoMartincoski/buildroot/pipelines/27301317
> > current master + this patch:
> > https://gitlab.com/RicardoMartincoski/buildroot/pipelines/27301358
> > It doesn't get to show that special-ref is broken because the tests run
> > sequentially (to avoid overpopulate the gitlab CI), but it shows (in the
> > -build.log) that also the download of a sha1 tip of a branch (search for
> > "git-sha1-branch-head" in the log) would be broken with this patch.
> > This can be reproduced locally:
> > $ make defconfig
> > $ ./utils/config --set-str BR2_BACKUP_SITE ""
> > $ BR2_DL_DIR=$(mktemp -d) make tremor-dirclean tremor-source
> > ...
> > Commit '7c30a66346199f3f09017a09567c6c8a3a0eedc8' is a branch name.
> > Using a branch name is not supported.

Yann: this one I don't understand.
7c30a66346199f3f09017a09567c6c8a3a0eedc8 is a regular commit SHA1, so
it should not be considered as a branch. Why do we have this failure
for a commit SHA1 ?

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 3/3] support/download: detect and abort when using a git branch by name

[Yann E. MORIN]

Thomas, All,

On 2018-08-12 22:41 +0200, Thomas Petazzoni spake thusly:
> > > The other way is by using this series:
> > > http://patchwork.ozlabs.org/project/buildroot/list/?series=44009
> > > current master:
> > > https://gitlab.com/RicardoMartincoski/buildroot/pipelines/27301317
> > > current master + this patch:
> > > https://gitlab.com/RicardoMartincoski/buildroot/pipelines/27301358
> > > It doesn't get to show that special-ref is broken because the tests run
> > > sequentially (to avoid overpopulate the gitlab CI), but it shows (in the
> > > -build.log) that also the download of a sha1 tip of a branch (search for
> > > "git-sha1-branch-head" in the log) would be broken with this patch.
> > > This can be reproduced locally:
> > > $ make defconfig
> > > $ ./utils/config --set-str BR2_BACKUP_SITE ""
> > > $ BR2_DL_DIR=$(mktemp -d) make tremor-dirclean tremor-source
> > > ...
> > > Commit '7c30a66346199f3f09017a09567c6c8a3a0eedc8' is a branch name.
> > > Using a branch name is not supported.
> 
> Yann: this one I don't understand.
> 7c30a66346199f3f09017a09567c6c8a3a0eedc8 is a regular commit SHA1, so
> it should not be considered as a branch. Why do we have this failure
> for a commit SHA1 ?

That's because of our special refs support, for which we do [0]:

    git fetch origin "${cset}:${cset}"

This creates a local brnach named after the sha1, and git even whines:

    warning: refname '7c30a66346199f3f09017a09567c6c8a3a0eedc8' is ambiguous.
    Git normally never creates a ref that ends with 40 hex characters
    because it will be ignored when you just specify 40-hex. These refs
    may be created by mistake. For example,

      git checkout -b $br $(git rev-parse ...)

    where "$br" is somehow empty and a 40-hex ref is created. Please
    examine these refs and maybe delete them. Turn this message off by
    running "git config advice.objectNameWarning false"

[0] https://git.buildroot.org/buildroot/tree/support/download/git#n118

Regards,
Yann E. MORIN.

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

[Buildroot] [PATCH 3/3] support/download: detect and abort when using a git branch by name

[ricardo.martincoski at gmail.com]

Hello,

On Sun, Aug 12, 2018 at 05:48 PM, Yann E. MORIN wrote:

> On 2018-08-12 22:41 +0200, Thomas Petazzoni spake thusly:
>> > > The other way is by using this series:
>> > > http://patchwork.ozlabs.org/project/buildroot/list/?series=44009
>> > > current master:
>> > > https://gitlab.com/RicardoMartincoski/buildroot/pipelines/27301317
>> > > current master + this patch:
>> > > https://gitlab.com/RicardoMartincoski/buildroot/pipelines/27301358
>> > > It doesn't get to show that special-ref is broken because the tests run
>> > > sequentially (to avoid overpopulate the gitlab CI), but it shows (in the
>> > > -build.log) that also the download of a sha1 tip of a branch (search for
>> > > "git-sha1-branch-head" in the log) would be broken with this patch.
>> > > This can be reproduced locally:
>> > > $ make defconfig
>> > > $ ./utils/config --set-str BR2_BACKUP_SITE ""
>> > > $ BR2_DL_DIR=$(mktemp -d) make tremor-dirclean tremor-source
>> > > ...
>> > > Commit '7c30a66346199f3f09017a09567c6c8a3a0eedc8' is a branch name.
>> > > Using a branch name is not supported.
>> 
>> Yann: this one I don't understand.
>> 7c30a66346199f3f09017a09567c6c8a3a0eedc8 is a regular commit SHA1, so
>> it should not be considered as a branch. Why do we have this failure
>> for a commit SHA1 ?
> 
> That's because of our special refs support, for which we do [0]:
> 
>     git fetch origin "${cset}:${cset}"

But only removing this code will not make 'git show-ref' to work as expected.
There are git caches out there with the mentioned branch.
If for any reason the tarball is not there (like we do test in autobuilder) the
download will fail for the wrong reason, and it would require a manual fix.

And IMO we should not start removing all refs from the git caches otherwise we
can decrease the cache hit ratio (due to git gc), specially for linux trees.

So the best approach IMO is to ask "given we want a named ref, is that ref a
branch in the remote?". That obviously lead to using git ls-remote and check
the second column:
$ git ls-remote https://git.xiph.org/tremor.git
7c30a66346199f3f09017a09567c6c8a3a0eedc8        HEAD
293fd1c04f9d4489be6d4b2b1ca8698f2f902e8e        refs/heads/lowmem
7c30a66346199f3f09017a09567c6c8a3a0eedc8        refs/heads/master
f946f9acbf7aced75d3810a52c878e47680a18f6        refs/heads/nobyte
3175ff964c7d85d070df823189997f6b753cfef7        refs/heads/tremolo
0bcded806a0327c86d9246703724b45037d1bbaa        refs/heads/tremolo_mips

And the corner case is when there is a branch and a tag with the same name.
We must choose whether that case fails or the tag is used. I would use the tag.

Regards,
Ricardo

[Buildroot] [PATCH 3/3] support/download: detect and abort when using a git branch by name

[Yann E. MORIN]

Ricardo, All,

On 2018-08-13 11:13 -0300, ricardo.martincoski at gmail.com spake thusly:
> On Sun, Aug 12, 2018 at 05:48 PM, Yann E. MORIN wrote:
> > On 2018-08-12 22:41 +0200, Thomas Petazzoni spake thusly:
> >> > > The other way is by using this series:
> >> > > http://patchwork.ozlabs.org/project/buildroot/list/?series=44009
> >> > > current master:
> >> > > https://gitlab.com/RicardoMartincoski/buildroot/pipelines/27301317
> >> > > current master + this patch:
> >> > > https://gitlab.com/RicardoMartincoski/buildroot/pipelines/27301358
> >> > > It doesn't get to show that special-ref is broken because the tests run
> >> > > sequentially (to avoid overpopulate the gitlab CI), but it shows (in the
> >> > > -build.log) that also the download of a sha1 tip of a branch (search for
> >> > > "git-sha1-branch-head" in the log) would be broken with this patch.
> >> > > This can be reproduced locally:
> >> > > $ make defconfig
> >> > > $ ./utils/config --set-str BR2_BACKUP_SITE ""
> >> > > $ BR2_DL_DIR=$(mktemp -d) make tremor-dirclean tremor-source
> >> > > ...
> >> > > Commit '7c30a66346199f3f09017a09567c6c8a3a0eedc8' is a branch name.
> >> > > Using a branch name is not supported.
> >> 
> >> Yann: this one I don't understand.
> >> 7c30a66346199f3f09017a09567c6c8a3a0eedc8 is a regular commit SHA1, so
> >> it should not be considered as a branch. Why do we have this failure
> >> for a commit SHA1 ?
> > 
> > That's because of our special refs support, for which we do [0]:
> > 
> >     git fetch origin "${cset}:${cset}"
> 
> But only removing this code will not make 'git show-ref' to work as expected.
> There are git caches out there with the mentioned branch.
> If for any reason the tarball is not there (like we do test in autobuilder) the
> download will fail for the wrong reason, and it would require a manual fix.

Rightfully right.

So, we don't care about the ref being a local branch. Instead, we must
check whether it is a branch on the remote. I.e.:

    git show-ref "origin/${cset}" |grep -qv refs/tags

Thoughts?

> And IMO we should not start removing all refs from the git caches otherwise we
> can decrease the cache hit ratio (due to git gc), specially for linux trees.

Well, we can still remove local branches.

> So the best approach IMO is to ask "given we want a named ref, is that ref a
> branch in the remote?". That obviously lead to using git ls-remote and check
> the second column:

I would prefer to avoid another round-trip to the remote, when we have
the necessary information locally. What about the git-show-ref snippet I
pasted above instead?

> $ git ls-remote https://git.xiph.org/tremor.git
> 7c30a66346199f3f09017a09567c6c8a3a0eedc8        HEAD
> 293fd1c04f9d4489be6d4b2b1ca8698f2f902e8e        refs/heads/lowmem
> 7c30a66346199f3f09017a09567c6c8a3a0eedc8        refs/heads/master
> f946f9acbf7aced75d3810a52c878e47680a18f6        refs/heads/nobyte
> 3175ff964c7d85d070df823189997f6b753cfef7        refs/heads/tremolo
> 0bcded806a0327c86d9246703724b45037d1bbaa        refs/heads/tremolo_mips
> 
> And the corner case is when there is a branch and a tag with the same name.
> We must choose whether that case fails or the tag is used. I would use the tag.

Arggg... Is it really possible to have a tag and a branch with the same
name? Indeed it is... Sigh... :-(

OK, so some more hacking is needed...

Regards,
Yann E. MORIN.

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

[Buildroot] [PATCH 3/3] support/download: detect and abort when using a git branch by name

[Ricardo Martincoski]

Hello,

On Mon, Aug 13, 2018 at 01:06 PM, Yann E. MORIN wrote:

> On 2018-08-13 11:13 -0300, ricardo.martincoski at gmail.com spake thusly:
>> On Sun, Aug 12, 2018 at 05:48 PM, Yann E. MORIN wrote:
[snip]
>> >     git fetch origin "${cset}:${cset}"
>> 
>> But only removing this code will not make 'git show-ref' to work as expected.
>> There are git caches out there with the mentioned branch.
>> If for any reason the tarball is not there (like we do test in autobuilder) the
>> download will fail for the wrong reason, and it would require a manual fix.
> 
> Rightfully right.
> 
> So, we don't care about the ref being a local branch. Instead, we must
> check whether it is a branch on the remote. I.e.:
> 
>     git show-ref "origin/${cset}" |grep -qv refs/tags
> 
> Thoughts?

I didn't tested it, but it looks promising.

> 
>> And IMO we should not start removing all refs from the git caches otherwise we
>> can decrease the cache hit ratio (due to git gc), specially for linux trees.
> 
> Well, we can still remove local branches.

OK.

> 
>> So the best approach IMO is to ask "given we want a named ref, is that ref a
>> branch in the remote?". That obviously lead to using git ls-remote and check
>> the second column:
> 
> I would prefer to avoid another round-trip to the remote, when we have
> the necessary information locally. What about the git-show-ref snippet I
> pasted above instead?

In the other hand with ls-remote we could fail faster, before downloading any
refs.

Regards,
Ricardo

[Buildroot] [PATCH 3/3] support/download: detect and abort when using a git branch by name

[Arnout Vandecappelle]

On 16/08/2018 03:04, Ricardo Martincoski wrote:
> Hello,
> 
> On Mon, Aug 13, 2018 at 01:06 PM, Yann E. MORIN wrote:
> 
>> On 2018-08-13 11:13 -0300, ricardo.martincoski at gmail.com spake thusly:
>>> On Sun, Aug 12, 2018 at 05:48 PM, Yann E. MORIN wrote:
[snip]
>>> So the best approach IMO is to ask "given we want a named ref, is that ref a
>>> branch in the remote?". That obviously lead to using git ls-remote and check
>>> the second column:
>>
>> I would prefer to avoid another round-trip to the remote, when we have
>> the necessary information locally. What about the git-show-ref snippet I
>> pasted above instead?
> 
> In the other hand with ls-remote we could fail faster, before downloading any
> refs.

 A failure in this case is because the developer specified a branch name instead
of a tag or sha. He in the end still wants to download that specific ref. So
downloading it before the check does make sense, because that way the cache is
already populated and the next attempt will go faster.

 Regards,
 Arnout

-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [PATCH 3/3] support/download: detect and abort when using a git branch by name

[Ricardo Martincoski]

Hello,

On Tue, Aug 21, 2018 at 05:22 PM, Arnout Vandecappelle wrote:

> On 16/08/2018 03:04, Ricardo Martincoski wrote:
>> 
>> On Mon, Aug 13, 2018 at 01:06 PM, Yann E. MORIN wrote:
>> 
>>> On 2018-08-13 11:13 -0300, ricardo.martincoski at gmail.com spake thusly:
>>>> On Sun, Aug 12, 2018 at 05:48 PM, Yann E. MORIN wrote:
> [snip]
>>>> So the best approach IMO is to ask "given we want a named ref, is that ref a
>>>> branch in the remote?". That obviously lead to using git ls-remote and check
>>>> the second column:
>>>
>>> I would prefer to avoid another round-trip to the remote, when we have
>>> the necessary information locally. What about the git-show-ref snippet I
>>> pasted above instead?
>> 
>> In the other hand with ls-remote we could fail faster, before downloading any
>> refs.
> 
>  A failure in this case is because the developer specified a branch name instead
> of a tag or sha. He in the end still wants to download that specific ref. So
> downloading it before the check does make sense, because that way the cache is
> already populated and the next attempt will go faster.

... because in this case we 'exit 1' instead of ditching the cache.

OK. Now I got why v2 should use show-ref. Thanks.


Regards,
Ricardo

[Buildroot] [PATCH 3/3] support/download: detect and abort when using a git branch by name

[Ricardo Martincoski]

Hello,

On Sun, Aug 12, 2018 at 05:41 PM, Thomas Petazzoni wrote:

> On Sun, 12 Aug 2018 22:25:43 +0200, Yann E. MORIN wrote:
> 
>> > Commit 'refs/pull/1014/head' is a branch name.
>> > Using a branch name is not supported.  
>> 
>> But then, the special refs also suffer from the same problems as
>> branches do: they can't work for the very same reasons as explained in
>> the previous patch.
>> 
>> So, I would be of the opinion that we should just drop support for those
>> special refs altogether, based on the same explanations.

Indeed using the name of a special ref as version is not guaranteed to be
reproducible for all special refs providers.

>> 
>> Now, I do understand that one will want to be able to test github MRs,
>> or gerrit reviews or whatnots... But in that case, we already civer this
>> by way of local.mk and overide-srcdir.
>> 
>> So, I would argue (as I always had since we introduced this special refs
>> support) that this should better be handled by an upper layer.
>> 
>> Sure, you'd argue that an automated build job could do the build. But
>> you anyway have to write some scripting for that automated job anyway.
>> Just have it prepare a git clone of the affected package, checkout the
>> correct commit, and prepare a local.mk with the correct override-srcdir
>> befor attempting the buildroot build.

It seems a lot of scripting for something that git supports natively and that
could be added to the backend to support the right way: using sha1 for
reproducibility.

> 
> I don't remember all the discussion about special refs, but if I

Most can be seen on [0].

> understand correctly, it's for example a Github pull request. And if I
> understood how Github works, you can push a new version of a pull
> request as the same pull request number. This means that what you fetch
> from a pull request is not stable. So it suffers from the same
> reproducibility issue as regular branches.

Gerrit Changes (when the server has the 'Draft' mode disabled) do not suffer
from the same. Each new revision ('Patch Set') has its own sha1. The refs for
many revisions of Change 1001 look like this:
refs/changes/01/1001/1
refs/changes/01/1001/2
refs/changes/01/1001/3
...

Aside: if someone wants to give a try, a test server can be run in minutes:
docker run -ti -p 8080:8080 -p 29418:29418 gerritcodereview/gerrit
No. It's not for Buildroot as it doesn't fit well for e-mail oriented reviews.
But the diff between any pair of revisions in the web interface is its awesome
feature IMO.
No. I have nothing to do with the Gerrit project, I am just a happy user.

> 
> Therefore, if we decide to officially not support branches, then we
> should also not support special refs. Unless of course I misunderstood
> the use case of special refs.

Fair enough. Let's officially don't support name of branches and name of
special refs as package version as they are not guaranteed to be reproducible,
taking in account all special ref providers.

What I think we should do is:
 - to re-add support to download of the sha1 of any ref (branch, tag, special
   ref).
 - add automated tests to avoid going back and forth, supporting some refs,
   then dropping support for them and then supporting them.
 - drop the support to the name of a special ref as it is not guaranteed to be
   reproducible.

At DATACOM we never used the name of special-ref as version.
The only occasion was when we updated the buildroot version to 2016.08 and the
support to sha1 of special-ref was removed. Then we used the name for a while as
immediate workaround, but it brings double effort to the developers.
Then I created a patch, submitted to the mailing list, with tests, and we are
maintaining inhouse ever since. We did not updated to the new download infra.

I can send a patch series for the first 2. An early version is at [1].
But due to the lack of time (testing linux trees takes time) I so far only
tested it with all packages that use the git download method in the tree. More
tests are needed for git versions (specially git 1.7.1 for RHEL6 and the brand
new one at the time) and also changing among linux trees. It also lacks a
nice commit message and probably some patch splitting. Notice in the series I
already assumed the support for name of special ref as package version will be
dropped.
This series certainly will not be applied to 2018.08 as we are so close to the
release.

But maybe even this patch "detect and abort" should not be applied to 2018.08
either as it is not so trivial, demonstrated by this review.
Just to be clear: please do not apply v1 of this patch. It will break things.

[0] http://patchwork.ozlabs.org/patch/654418/
[1] https://gitlab.com/RicardoMartincoski/buildroot/pipelines/27791462

Regards,
Ricardo

[Buildroot] [PATCH 3/3] support/download: detect and abort when using a git branch by name

[Yann E. MORIN]

Ricardo, All,

On 2018-08-13 11:33 -0300, Ricardo Martincoski spake thusly:
> On Sun, Aug 12, 2018 at 05:41 PM, Thomas Petazzoni wrote:
> > On Sun, 12 Aug 2018 22:25:43 +0200, Yann E. MORIN wrote:
> >> > Commit 'refs/pull/1014/head' is a branch name.
> >> > Using a branch name is not supported.  
[--SNIP--]
> >> Sure, you'd argue that an automated build job could do the build. But
> >> you anyway have to write some scripting for that automated job anyway.
> >> Just have it prepare a git clone of the affected package, checkout the
> >> correct commit, and prepare a local.mk with the correct override-srcdir
> >> befor attempting the buildroot build.
> It seems a lot of scripting for something that git supports natively and that

Not true: they are not even fetched by default at all. If they were,
then we would not be having this discussion. As it stands, we currently
have to resort to unstable trickery to use those.

> could be added to the backend to support the right way: using sha1 for
> reproducibility.

I also thought about it and tried to replace the special ref by its
expanded sha1 and name the local archive by the sha1.

But that does not work, because locating the tarball is completely
decorelated from generating it.

I.e. the code that extracts the tarball has no way to know that the
version string has been changed!

[--SNIP--]
> Gerrit Changes (when the server has the 'Draft' mode disabled) do not suffer
> from the same. Each new revision ('Patch Set') has its own sha1. The refs for
> many revisions of Change 1001 look like this:
> refs/changes/01/1001/1
> refs/changes/01/1001/2
> refs/changes/01/1001/3

So, those would be stable, it looks like, indeed.

But then it means we're maybe currently doing it wrong anyway, as we
create a local branch for something that is more akin to a tag...

[--SNIP--]
> What I think we should do is:
>  - to re-add support to download of the sha1 of any ref (branch, tag, special
>    ref).

That is not supposed to be broken, and should work again if we check if
the remote has the ref as we both suggested in previous replies in this
thread (but you and I with different solutions).

>  - add automated tests to avoid going back and forth, supporting some refs,
>    then dropping support for them and then supporting them.

Yes, your series doing that is still pending... TBH, it is still marked
as unread here, because I still want to have a look at it...

>  - drop the support to the name of a special ref as it is not guaranteed to be
>    reproducible.

This should be done before we add tests, I think.

[--SNIP--]
> But maybe even this patch "detect and abort" should not be applied to 2018.08
> either as it is not so trivial, demonstrated by this review.
> Just to be clear: please do not apply v1 of this patch. It will break things.

Agreed. It is material for next.

Thanks! :-)

Regards,
Yann E. MORIN.

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

[Buildroot] [PATCH 3/3] support/download: detect and abort when using a git branch by name

[Ricardo Martincoski]

Hello,

On Mon, Aug 13, 2018 at 01:19 PM, Yann E. MORIN wrote:
> On 2018-08-13 11:33 -0300, Ricardo Martincoski spake thusly:
>> On Sun, Aug 12, 2018 at 05:41 PM, Thomas Petazzoni wrote:
>> > On Sun, 12 Aug 2018 22:25:43 +0200, Yann E. MORIN wrote:
>> >> > Commit 'refs/pull/1014/head' is a branch name.
>> >> > Using a branch name is not supported.  
> [--SNIP--]
>> >> Sure, you'd argue that an automated build job could do the build. But
>> >> you anyway have to write some scripting for that automated job anyway.
>> >> Just have it prepare a git clone of the affected package, checkout the
>> >> correct commit, and prepare a local.mk with the correct override-srcdir
>> >> befor attempting the buildroot build.
>> It seems a lot of scripting for something that git supports natively and that
> 
> Not true: they are not even fetched by default at all. If they were,
> then we would not be having this discussion. As it stands, we currently
> have to resort to unstable trickery to use those.

OK. The porcelain commands do support them but they are not fetched using the
default options.

> 
>> could be added to the backend to support the right way: using sha1 for
>> reproducibility.
> 
> I also thought about it and tried to replace the special ref by its
> expanded sha1 and name the local archive by the sha1.

No. Sorry. I meant the other way around:
To always use (for special refs) a sha1 as version of the package and get the
download infra to fetch it using the name expanded from the sha1 (by git
ls-remote), as it is reproducible.
It also works for branches tips and tags and provides some performance
improvement, nothing so dramatic as --depth 1 but without all the corner cases
that come with shallow clones, because it is not shallow, we just download more
like... on-demand.

[snip]
>> What I think we should do is:
>>  - to re-add support to download of the sha1 of any ref (branch, tag, special
>>    ref).
> 
> That is not supposed to be broken, and should work again if we check if
> the remote has the ref as we both suggested in previous replies in this
> thread (but you and I with different solutions).

Sorry. I failed to express what I wanted to. Let me try again:
  - to re-add support to download of the sha1 of *any* ref (sha1 of branch tip
    and sha1 of tag currently do work, sha1 of special ref worked in the past
    but don't work anymore).

> 
>>  - add automated tests to avoid going back and forth, supporting some refs,
>>    then dropping support for them and then supporting them.
> 
> Yes, your series doing that is still pending... TBH, it is still marked
> as unread here, because I still want to have a look at it...

Great! Please do review when you find some time.
They use br2-external, hash-check and git repos.

> 
>>  - drop the support to the name of a special ref as it is not guaranteed to be
>>    reproducible.
> 
> This should be done before we add tests, I think.

*should*: no. We can always adapt the tests when the behavior needs to change.
If we had already the test case for the name of a special ref as package
version, we would keep it, and just change it (in the same commit removing the
code from the backend) to ensure the download using the name of a special ref
always fails instead of always succeeding.

But yes. It *can* be done before.


Regards,
Ricardo