download: detect and refuse git branch by name
cover download: detect and refuse git branch by name
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(-)
Signature · 6 lines
.-----------------.--------------------.------------------.--------------------. | 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. | '------------------------------^-------^------------------^--------------------'
PATCH 1/3 support/download: remove help from wrapper
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.
--- support/download/dl-wrapper | 47 +-------------------------------------------- 1 file changed, 1 insertion(+), 46 deletions(-)
support/download/dl-wrapper +1 -46
@@ -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}; }
Signature · 1 line
2.14.1
Thomas Petazzoni
AppliedPATCH 1/3 support/download: remove help from wrapper
Hello,
Quoted 9 lines · 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" <[email protected]>--- support/download/dl-wrapper | 47 +-------------------------------------------- 1 file changed, 1 insertion(+), 46 deletions(-)
Applied to next, thanks.
Thomas
Signature · 3 lines
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com
PATCH 2/3 docs/manual: expand on why using a branch name is not supported
In reply to Yann E. MORIN--- docs/manual/adding-packages-generic.txt | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)
docs/manual/adding-packages-generic.txt +13 -2
@@ -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
Signature · 1 line
2.14.1
PATCH 2/3 docs/manual: expand on why using a branch name is not supported
Hello,
docs/manual/adding-packages-generic.txt information is (assuming the package name is +libfoo+) : · line 214
Quoted 8 lines · Yann E. MORIN
++
+.Note:docs/manual/adding-packages-generic.txt +6
-0+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"
docs/manual/adding-packages-generic.txt information is (assuming the package name is +libfoo+) : · line 218
Quoted 4 lines
docs/manual/adding-packages-generic.txt +4
-0+ 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
Signature · 3 lines
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com
PATCH 2/3 docs/manual: expand on why using a branch name is not supported
In reply to Yann E. MORINHello Yann,
Quoted 3 lines · Yann E. MORIN
Signed-off-by "Yann E. MORIN" <[email protected]>Cc Thomas Petazzoni <[email protected]>Cc Thomas De Schampheleire <[email protected]>
Following my comments, I've marked this patch as Changes Requested.
Thomas
Signature · 3 lines
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com
PATCH 3/3 support/download: detect and abort when using a git branch by name
In reply to Yann E. MORINUsing 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...
--- support/download/git | 7 +++++++ 1 file changed, 7 insertions(+)
support/download/git +7 -0
@@ -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.
Signature · 1 line
2.14.1
PATCH 3/3 support/download: detect and abort when using a git branch by name
Hello,
On Sat, Aug 04, 2018 at 01:33 PM, Yann E. MORIN wrote:
[snip]
support/download/git if ! _git rev-parse --quiet --verify "'${cset}^{commit}'" >/dev/null 2>&1; then · line 138
Quoted 2 lines
support/download/git +2
-0+# 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
PATCH 3/3 support/download: detect and abort when using a git branch by name
Ricardo, All,
On 2018-08-06 00:14 -0300, Ricardo Martincoski spake thusly:
Quoted 6 lines · Yann E. MORIN
support/download/git if ! _git rev-parse --quiet --verify "'${cset}^{commit}'" >/dev/null 2>&1; then · line 138
Quoted 2 lines
support/download/git +2
-0+# Check if the changeset is a branch name. +if _git show-ref "${cset}" |grep -qv refs/tags; thenI 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.
Signature · 6 lines
.-----------------.--------------------.------------------.--------------------. | 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. | '------------------------------^-------^------------------^--------------------'
PATCH 3/3 support/download: detect and abort when using a git branch by name
Hello,
Quoted 8 lines · Yann E. MORIN
On 2018-08-06 00:14 -0300, Ricardo Martincoski spake thusly:
Quoted 6 lines · Yann E. MORIN
support/download/git if ! _git rev-parse --quiet --verify "'${cset}^{commit}'" >/dev/null 2>&1; then · line 138
Quoted 2 lines
support/download/git +2
-0+# Check if the changeset is a branch name. +if _git show-ref "${cset}" |grep -qv refs/tags; thenI didn't had time to test it yet.
Special refs still works after this?
Quoted 4 lines
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
PATCH 3/3 support/download: detect and abort when using a git branch by name
Ricardo, All,
On 2018-08-06 21:39 -0300, Ricardo Martincoski spake thusly:
Quoted 16 lines · Yann E. MORIN
Quoted 8 lines
On 2018-08-06 00:14 -0300, Ricardo Martincoski spake thusly:
Quoted 6 lines · Yann E. MORIN
support/download/git if ! _git rev-parse --quiet --verify "'${cset}^{commit}'" >/dev/null 2>&1; then · line 138
Quoted 2 lines
support/download/git +2
-0+# Check if the changeset is a branch name. +if _git show-ref "${cset}" |grep -qv refs/tags; thenI didn't had time to test it yet.
Special refs still works after this?Quoted 4 lines
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):
Quoted 11 lines
$ 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.
Quoted 26 lines
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
Signature · 6 lines
.-----------------.--------------------.------------------.--------------------. | 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. | '------------------------------^-------^------------------^--------------------'
PATCH 3/3 support/download: detect and abort when using a git branch by name
Yann, Ricardo,
Quoted 14 lines · Yann E. MORIN
Quoted 2 lines
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.
Quoted 8 lines
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.
Quoted 9 lines
Quoted 9 lines
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
Quoted 8 lines
Quoted 8 lines
"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
Signature · 3 lines
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com
PATCH 3/3 support/download: detect and abort when using a git branch by name
Thomas, All,
On 2018-08-12 22:41 +0200, Thomas Petazzoni spake thusly:
Quoted 18 lines
Quoted 9 lines
Quoted 9 lines
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 forQuoted 8 lines
Quoted 8 lines
"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.
Quoted 4 lines
Yann: this one I don't understand.
7c30a66346199f3f09017a09567c6c8a3a0eedc8is 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.
Signature · 6 lines
.-----------------.--------------------.------------------.--------------------. | 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. | '------------------------------^-------^------------------^--------------------'
PATCH 3/3 support/download: detect and abort when using a git branch by name
Hello,
Quoted 19 lines · Yann E. MORIN
On 2018-08-12 22:41 +0200, Thomas Petazzoni spake thusly:
Quoted 18 lines
Quoted 17 lines
Quoted 17 lines
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.
Quoted 8 lines
Quoted 4 lines
Yann: this one I don't understand.
7c30a66346199f3f09017a09567c6c8a3a0eedc8is 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
PATCH 3/3 support/download: detect and abort when using a git branch by name
Ricardo, All,
On 2018-08-13 11:13 -0300, ricardo.martincoski at gmail.com spake thusly:
Quoted 25 lines · Yann E. MORIN
Quoted 24 lines
On 2018-08-12 22:41 +0200, Thomas Petazzoni spake thusly:
Quoted 22 lines
Quoted 9 lines
Quoted 9 lines
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 forQuoted 8 lines
Quoted 8 lines
"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.
7c30a66346199f3f09017a09567c6c8a3a0eedc8is a regular commit SHA1, so it should not be considered as a branch. Why do we have this failure for a commit SHA1 ?
Quoted 8 lines
Quoted 3 lines
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/tagsThoughts?
Quoted 2 lines
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.
Quoted 3 lines
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?
Quoted 10 lines
$ 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_mipsAnd 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.
Signature · 6 lines
.-----------------.--------------------.------------------.--------------------. | 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. | '------------------------------^-------^------------------^--------------------'
PATCH 3/3 support/download: detect and abort when using a git branch by name
Hello,
Quoted 2 lines · Yann E. MORIN
On 2018-08-13 11:13 -0300, ricardo.martincoski at gmail.com spake thusly:
Quoted 1 line · Yann E. MORIN
[snip]
Quoted 7 lines
Quoted 6 lines
Quoted 1 line
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.
Quoted 8 lines
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/tagsThoughts?
I didn't tested it, but it looks promising.
Quoted 5 lines
Quoted 3 lines
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.
Quoted 8 lines
Quoted 4 lines
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
PATCH 3/3 support/download: detect and abort when using a git branch by name
Quoted 6 lines · Ricardo Martincoski
Hello,
Quoted 2 lines · Yann E. MORIN
On 2018-08-13 11:13 -0300, ricardo.martincoski at gmail.com spake thusly:
Quoted 1 line · Yann E. MORIN
[snip]
Quoted 10 lines
Quoted 7 lines
Quoted 3 lines
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
Signature · 6 lines
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
PATCH 3/3 support/download: detect and abort when using a git branch by name
Hello,
Quoted 15 lines · Arnout Vandecappelle
Quoted 5 lines · Ricardo Martincoski
Quoted 2 lines · Yann E. MORIN
On 2018-08-13 11:13 -0300, ricardo.martincoski at gmail.com spake thusly:
Quoted 1 line · Yann E. MORIN
[snip]
Quoted 8 lines
Quoted 7 lines
Quoted 3 lines
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?
Quoted 7 lines
Quoted 2 lines
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
PATCH 3/3 support/download: detect and abort when using a git branch by name
In reply to Thomas PetazzoniHello,
Quoted 11 lines · Thomas Petazzoni
Quoted 9 lines · Yann E. MORIN
Quoted 2 lines
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.
Quoted 5 lines
Quoted 5 lines
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.
Quoted 8 lines
Quoted 8 lines
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.
Quoted 2 lines
I don't remember all the discussion about special refs, but if I
Most can be seen on [0].
Quoted 5 lines
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.
Quoted 4 lines
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
PATCH 3/3 support/download: detect and abort when using a git branch by name
Ricardo, All,
On 2018-08-13 11:33 -0300, Ricardo Martincoski spake thusly:
Quoted 4 lines · Thomas Petazzoni
Quoted 3 lines · Yann E. MORIN
Quoted 2 lines
Quoted 2 lines
Commit 'refs/pull/1014/head' is a branch name.
Using a branch name is not supported.
[--SNIP--]
Quoted 6 lines
Quoted 5 lines
Quoted 5 lines
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.
Quoted 2 lines
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--]
Quoted 6 lines
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--]
Quoted 3 lines
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).
Quoted 2 lines
- 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...
Quoted 2 lines
- 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--]
Quoted 3 lines
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.
Signature · 6 lines
.-----------------.--------------------.------------------.--------------------. | 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. | '------------------------------^-------^------------------^--------------------'
PATCH 3/3 support/download: detect and abort when using a git branch by name
Hello,
Quoted 11 lines · Yann E. MORIN
On 2018-08-13 11:33 -0300, Ricardo Martincoski spake thusly:
Quoted 4 lines · Thomas Petazzoni
Quoted 3 lines · Yann E. MORIN
Quoted 2 lines
Quoted 2 lines
Commit 'refs/pull/1014/head' is a branch name.
Using a branch name is not supported.[--SNIP--]
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
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.
Quoted 5 lines
Quoted 1 line
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.
Quoted 6 lines
Quoted 3 lines
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]
Quoted 7 lines
Quoted 3 lines
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).
Quoted 6 lines
Quoted 3 lines
- 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.
Quoted 5 lines
Quoted 3 lines
- 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