[Buildroot] [v3 12/13] download: add flock call before dl-wrapper
Yann E. MORIN
yann.morin.1998 at free.fr
Sun Apr 1 17:53:57 UTC 2018
Maxime, All,
On 2018-04-01 16:09 +0200, Yann E. MORIN spake thusly:
> On 2018-03-31 16:24 +0200, Maxime Hadjinlian spake thusly:
> > In order to introduce the cache mechanisms, we need to have a lock on
> > the $(LIBFOO_DL_DIR), otherwise it would be impossible to do parallel
> > download (a shared DL_DIR for two buildroot instances).
> >
> > To make sure the directory exists, the mkdir call has been removed from
> > the dl-wrapper and put in the infrastructure.
> >
> > Signed-off-by: Maxime Hadjinlian <maxime.hadjinlian at gmail.com>
> > ---
> > package/pkg-download.mk | 4 +++-
> > support/download/dl-wrapper | 1 -
> > 2 files changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/package/pkg-download.mk b/package/pkg-download.mk
> > index 23f3403098..dff52007e6 100644
> > --- a/package/pkg-download.mk
> > +++ b/package/pkg-download.mk
> > @@ -19,6 +19,7 @@ SSH := $(call qstrip,$(BR2_SSH))
> > export LOCALFILES := $(call qstrip,$(BR2_LOCALFILES))
> >
> > DL_WRAPPER = support/download/dl-wrapper
> > +FLOCK = flock $($(PKG)_DL_DIR)/
>
> This means that two downloads running in two concurrent Buildroot
> builds, doing a wget from different tarballs from the same package
> (eg. foo-1.tar and foo-2.tar) will not be able to complete in parallel,
> and will be sequential, while we curently can do that, and it *is* safe
> to do so.
>
> We have a few objects we can flock, in a generic manner:
>
> - the .stamp_downloaded stanp file: this would allow concurrent
> Buildroot builds of wget downloads, but would not protect against
> concurrent git clones;
>
> - the $($(PKG)_DL_DIR): would protect against concurrent git clones,
> but forbids concurrent wget of the same package;
>
> And a third and fourth further options:
>
> - $($(PKG)_DL_DIR)/git: which would be the best of both world, but
> would need to be handled by the dl-wrapper itself when it calls
> the git backend (or hg, svn, cvs... when we later support those).
After a lie discussion, we've in fact decided to later go that route,
because it is not so complex to do afterall:
- in pkg-generic.mk: based on the _SITE_METHOD, set:
$(PKG)_SITE_LOCK = YES
- in pkg-download.mk, in macro DOWNLOAD, when calling dl-wrapper,
add something like:
$(if $($(PKG)_SITE_LOCK),-L $($PKG)_DL_DIR))
- in dl-wrapper, accept a new option, -L with as argument the resource
to lock, in which case dl-wrapper would lock it when calling the
wrapper, otherwise it would directly call the wrapper.
But as also said live, this can be an improvement for *after* this
series is applied.
Regards,
Yann E. MORIN.
> - push the flock even further down into each individual backends,
> which would each be responsible to flock only the individual
> commands that require locking.
>
> For the fourth solution, we may be able to shoe-horn the flock call into
> the internals _XXX wrappers (e.g. the _git() function of the git
> backend).
>
> TBH, I believe that what you propsoe is Good Enough (TM), because the
> third, optimised solution, is just getting a bit more complex, as there
> is no resource (file or directory) that we can flock, besides the
> package's own download dir. The fourth is pusshing into too fine-grained
> for being entirely reliable...
>
> So, I'm happy that we go with your proposed patch. But maybe expand the
> commit log to explain a bit that restriction.
>
> Regards,
> Yann E. MORIN.
>
> > # DL_DIR may have been set already from the environment
> > ifeq ($(origin DL_DIR),undefined)
> > @@ -91,7 +92,8 @@ endif
> >
> > define DOWNLOAD
> > $(Q)$(if $(filter bzr cvs hg svn,$($(PKG)_SITE_METHOD)),BR_NO_CHECK_HASH_FOR=$(notdir $(1));) \
> > - $(EXTRA_ENV) $(DL_WRAPPER) \
> > + $(Q)mkdir -p $($(PKG)_DL_DIR)/
> > + $(EXTRA_ENV) $(FLOCK) $(DL_WRAPPER) \
> > -c $($(PKG)_DL_VERSION) \
> > -f $(notdir $(1)) \
> > -H $(PKGDIR)/$($(PKG)_RAWNAME).hash \
> > diff --git a/support/download/dl-wrapper b/support/download/dl-wrapper
> > index 49caf3717b..67e9742767 100755
> > --- a/support/download/dl-wrapper
> > +++ b/support/download/dl-wrapper
> > @@ -50,7 +50,6 @@ main() {
> > if [ -z "${output}" ]; then
> > error "no output specified, use -o\n"
> > fi
> > - mkdir -p "$(dirname "${output}")"
> >
> > # If the output file already exists and:
> > # - there's no .hash file: do not download it again and exit promptly
> > --
> > 2.16.2
> >
>
> --
> .-----------------.--------------------.------------------.--------------------.
> | 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 mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot
--
.-----------------.--------------------.------------------.--------------------.
| 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. |
'------------------------------^-------^------------------^--------------------'
More information about the buildroot
mailing list