[Buildroot] [PATCH] Config.in: add BR2_DL_DIR_OPTS

[Buildroot] [PATCH] Config.in: add BR2_DL_DIR_OPTS

[Markus Mayer]

Provide a simple mechanism to pass extra arguments to "mkdir" when
creating download directories. This can be helpful if one needs the
download directories to be writable multiple users ("shared download
cache").

Signed-off-by: Markus Mayer <mmayer@broadcom.com>
---
 Config.in               | 14 ++++++++++++++
 package/pkg-download.mk |  2 +-
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/Config.in b/Config.in
index 42cdf7a3ebdc..0442d8403b4a 100644
--- a/Config.in
+++ b/Config.in
@@ -214,6 +214,20 @@ config BR2_DL_DIR
 
 	  The default is $(TOPDIR)/dl
 
+config BR2_DL_DIR_OPTS
+	string "Options for mkdir when it creates BR2_DL_DIR"
+	default ""
+	help
+	  Specify command line options for the mkdir command when it
+	  creates BR2_DL_DIR.
+
+	  For example, this can be used to specify directory permissions
+	  (say, "-m 0775") for directories inside BR2_DL_DIR. One would
+	  want this if the download directory is a shared "cache" for
+	  multiple users, so everybody can write to these directories.
+
+	  The default empty (no additional options).
+
 config BR2_HOST_DIR
 	string "Host dir"
 	default "$(BASE_DIR)/host"
diff --git a/package/pkg-download.mk b/package/pkg-download.mk
index 73ea2a69f829..d6bfe2bb06d0 100644
--- a/package/pkg-download.mk
+++ b/package/pkg-download.mk
@@ -91,7 +91,7 @@ endif
 endif
 
 define DOWNLOAD
-	$(Q)mkdir -p $($(PKG)_DL_DIR)
+	$(Q)mkdir $(BR2_DL_DIR_OPTS) -p $($(PKG)_DL_DIR)
 	$(Q)$(EXTRA_ENV) $(FLOCK) $(DL_WRAPPER) \
 		-c '$($(PKG)_DL_VERSION)' \
 		-d '$($(PKG)_DL_DIR)' \
-- 
2.17.1

[Buildroot] [PATCH] Config.in: add BR2_DL_DIR_OPTS

[Carlos Santos]

> From: "Markus Mayer" <mmayer@broadcom.com>
> To: "buildroot" <buildroot@buildroot.org>
> Cc: "Markus Mayer" <mmayer@broadcom.com>
> Sent: Wednesday, October 31, 2018 6:39:30 PM
> Subject: [Buildroot] [PATCH] Config.in: add BR2_DL_DIR_OPTS

> Provide a simple mechanism to pass extra arguments to "mkdir" when
> creating download directories. This can be helpful if one needs the
> download directories to be writable multiple users ("shared download

writable by multiple users

-- 
Carlos Santos (Casantos) - DATACOM, P&D

[Buildroot] [PATCH] Config.in: add BR2_DL_DIR_OPTS

[Markus Mayer]

On Wed, 31 Oct 2018 at 18:54, Carlos Santos <casantos@datacom.com.br> wrote:
>
> > From: "Markus Mayer" <mmayer@broadcom.com>
> > To: "buildroot" <buildroot@buildroot.org>
> > Cc: "Markus Mayer" <mmayer@broadcom.com>
> > Sent: Wednesday, October 31, 2018 6:39:30 PM
> > Subject: [Buildroot] [PATCH] Config.in: add BR2_DL_DIR_OPTS
>
> > Provide a simple mechanism to pass extra arguments to "mkdir" when
> > creating download directories. This can be helpful if one needs the
> > download directories to be writable multiple users ("shared download
>
> writable by multiple users

Thanks. I'll fix that.

> --
> Carlos Santos (Casantos) - DATACOM, P&D

[Buildroot] [PATCH] Config.in: add BR2_DL_DIR_OPTS

[Yann E. MORIN]

Markus, All,

On 2018-10-31 14:39 -0700, Markus Mayer spake thusly:
> Provide a simple mechanism to pass extra arguments to "mkdir" when
> creating download directories. This can be helpful if one needs the
> download directories to be writable multiple users ("shared download
> cache").

In retrospect, I think our enforcing of umask globally is wrong.

It was added in commit bee5745ccc2 (Makefile: don't depend on the umask)
under the requirements that modes in target/ be predicatble.

Although that is indeed a good reason, I think we should have instead
ensured those modes with a plain chmod on target/ during the
target-finalize step.

That way, we could drop our umask override, and let the user define
whatever umask they need.

Regards,
Yann E. MORIN.

> Signed-off-by: Markus Mayer <mmayer@broadcom.com>
> ---
>  Config.in               | 14 ++++++++++++++
>  package/pkg-download.mk |  2 +-
>  2 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/Config.in b/Config.in
> index 42cdf7a3ebdc..0442d8403b4a 100644
> --- a/Config.in
> +++ b/Config.in
> @@ -214,6 +214,20 @@ config BR2_DL_DIR
>  
>  	  The default is $(TOPDIR)/dl
>  
> +config BR2_DL_DIR_OPTS
> +	string "Options for mkdir when it creates BR2_DL_DIR"
> +	default ""
> +	help
> +	  Specify command line options for the mkdir command when it
> +	  creates BR2_DL_DIR.
> +
> +	  For example, this can be used to specify directory permissions
> +	  (say, "-m 0775") for directories inside BR2_DL_DIR. One would
> +	  want this if the download directory is a shared "cache" for
> +	  multiple users, so everybody can write to these directories.
> +
> +	  The default empty (no additional options).
> +
>  config BR2_HOST_DIR
>  	string "Host dir"
>  	default "$(BASE_DIR)/host"
> diff --git a/package/pkg-download.mk b/package/pkg-download.mk
> index 73ea2a69f829..d6bfe2bb06d0 100644
> --- a/package/pkg-download.mk
> +++ b/package/pkg-download.mk
> @@ -91,7 +91,7 @@ endif
>  endif
>  
>  define DOWNLOAD
> -	$(Q)mkdir -p $($(PKG)_DL_DIR)
> +	$(Q)mkdir $(BR2_DL_DIR_OPTS) -p $($(PKG)_DL_DIR)
>  	$(Q)$(EXTRA_ENV) $(FLOCK) $(DL_WRAPPER) \
>  		-c '$($(PKG)_DL_VERSION)' \
>  		-d '$($(PKG)_DL_DIR)' \
> -- 
> 2.17.1
> 
> _______________________________________________
> 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.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH] Config.in: add BR2_DL_DIR_OPTS

[Thomas Petazzoni]

Hello,

On Thu, 1 Nov 2018 10:06:16 +0100, Yann E. MORIN wrote:

> On 2018-10-31 14:39 -0700, Markus Mayer spake thusly:
> > Provide a simple mechanism to pass extra arguments to "mkdir" when
> > creating download directories. This can be helpful if one needs the
> > download directories to be writable multiple users ("shared download
> > cache").  
> 
> In retrospect, I think our enforcing of umask globally is wrong.
> 
> It was added in commit bee5745ccc2 (Makefile: don't depend on the umask)
> under the requirements that modes in target/ be predicatble.
> 
> Although that is indeed a good reason, I think we should have instead
> ensured those modes with a plain chmod on target/ during the
> target-finalize step.

But is it possible to have a plain "chmod" on target/ that will
properly preserve whatever packages have decided to use as permissions
when installing files ?

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

[Buildroot] [PATCH] Config.in: add BR2_DL_DIR_OPTS

[Yann E. MORIN]

Thomas, All,

On 2018-11-01 12:01 +0100, Thomas Petazzoni spake thusly:
> On Thu, 1 Nov 2018 10:06:16 +0100, Yann E. MORIN wrote:
> > On 2018-10-31 14:39 -0700, Markus Mayer spake thusly:
> > > Provide a simple mechanism to pass extra arguments to "mkdir" when
> > > creating download directories. This can be helpful if one needs the
> > > download directories to be writable multiple users ("shared download
> > > cache").  
> > 
> > In retrospect, I think our enforcing of umask globally is wrong.
> > 
> > It was added in commit bee5745ccc2 (Makefile: don't depend on the umask)
> > under the requirements that modes in target/ be predicatble.
> > 
> > Although that is indeed a good reason, I think we should have instead
> > ensured those modes with a plain chmod on target/ during the
> > target-finalize step.
> 
> But is it possible to have a plain "chmod" on target/ that will
> properly preserve whatever packages have decided to use as permissions
> when installing files ?

Right, but specifying modes at install time is not reliable anyway, as
it is not running root. The reason for explicitly setting permissions at
install time is mostly due to packages wanting to set suid bits, which
can't work. There can be outliers too, but they mostly are corner cases
which should be fixed not to set permissions.

So packages should instead provide FOO_PERMISSIONS, or users should use
permissions tables.

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] Config.in: add BR2_DL_DIR_OPTS

[Arnout Vandecappelle]

On 31/10/18 22:39, Markus Mayer wrote:
> Provide a simple mechanism to pass extra arguments to "mkdir" when
> creating download directories. This can be helpful if one needs the
> download directories to be writable multiple users ("shared download
> cache").

 I think having a Config.in option for this is way overkill. You can just make
the directory before calling Buildroot, right? And if it is a shared directory,
it should be created only once anyway.

 Regards,
 Arnout

[snip]

[Buildroot] [PATCH] Config.in: add BR2_DL_DIR_OPTS

[Yann E. MORIN]

Arnout, All,

On 2018-11-01 13:19 +0100, Arnout Vandecappelle spake thusly:
> On 31/10/18 22:39, Markus Mayer wrote:
> > Provide a simple mechanism to pass extra arguments to "mkdir" when
> > creating download directories. This can be helpful if one needs the
> > download directories to be writable multiple users ("shared download
> > cache").
> 
>  I think having a Config.in option for this is way overkill. You can just make
> the directory before calling Buildroot, right? And if it is a shared directory,
> it should be created only once anyway.

Yes for the top-level BR2_DL_DIR. But Buildroot does create sub-dirs,
one per package, and Bbuildroot also enforces the umask to 0022, which
means sub-dirs will not have the group-writable bit set.

Regards,
Yann E. MORIN.

>  Regards,
>  Arnout
> 
> [snip]
> _______________________________________________
> 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.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH] Config.in: add BR2_DL_DIR_OPTS

[Markus Mayer]

On Thu, 1 Nov 2018 at 05:23, Yann E. MORIN <yann.morin.1998@free.fr> wrote:
>
> Arnout, All,
>
> On 2018-11-01 13:19 +0100, Arnout Vandecappelle spake thusly:
> > On 31/10/18 22:39, Markus Mayer wrote:
> > > Provide a simple mechanism to pass extra arguments to "mkdir" when
> > > creating download directories. This can be helpful if one needs the
> > > download directories to be writable multiple users ("shared download
> > > cache").
> >
> >  I think having a Config.in option for this is way overkill. You can just make
> > the directory before calling Buildroot, right? And if it is a shared directory,
> > it should be created only once anyway.
>
> Yes for the top-level BR2_DL_DIR. But Buildroot does create sub-dirs,
> one per package, and Bbuildroot also enforces the umask to 0022, which
> means sub-dirs will not have the group-writable bit set.

Yep. That's exactly the problem we are running into. There was no
issue when buildroot wasn't creating sub-dirs per package. But now,
with a sub-dir per package, those sub-directories end up being
non-writable by group, which means that a different user building
buildroot at a time when there is a new source tar-ball to download,
will receive a permission error.

BTW, the whole exercise of using a shared "dl" directory is to
conserve disk space on build machines.

Thanks,
-Markus

> Regards,
> Yann E. MORIN.
>
> >  Regards,
> >  Arnout
> >
> > [snip]
> > _______________________________________________
> > 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.  |
> '------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH] Config.in: add BR2_DL_DIR_OPTS

[Yann E. MORIN]

Markus, All,

On 2018-11-01 08:16 -0700, Markus Mayer spake thusly:
> On Thu, 1 Nov 2018 at 05:23, Yann E. MORIN <yann.morin.1998@free.fr> wrote:
> > On 2018-11-01 13:19 +0100, Arnout Vandecappelle spake thusly:
> > > On 31/10/18 22:39, Markus Mayer wrote:
> > > > Provide a simple mechanism to pass extra arguments to "mkdir" when
> > > > creating download directories. This can be helpful if one needs the
> > > > download directories to be writable multiple users ("shared download
> > > > cache").
> > >
> > >  I think having a Config.in option for this is way overkill. You can just make
> > > the directory before calling Buildroot, right? And if it is a shared directory,
> > > it should be created only once anyway.
> >
> > Yes for the top-level BR2_DL_DIR. But Buildroot does create sub-dirs,
> > one per package, and Bbuildroot also enforces the umask to 0022, which
> > means sub-dirs will not have the group-writable bit set.
> 
> Yep. That's exactly the problem we are running into. There was no
> issue when buildroot wasn't creating sub-dirs per package. But now,
> with a sub-dir per package, those sub-directories end up being
> non-writable by group, which means that a different user building
> buildroot at a time when there is a new source tar-ball to download,
> will receive a permission error.
> 
> BTW, the whole exercise of using a shared "dl" directory is to
> conserve disk space on build machines.

Note: I'm all in favour of fixing this use-case, but I have a few other
suggestion in addition to fixing the issue:

0. one may use newgrp to switch to the grou that owns the download
   directory:
    - first, prepare the directory:
        $ mkdir /some/place/dl-dir
        $ chown some-group /some/place/dl-dir
        $ chmod g+rwxs /some/place/dl-dir
    - now, each user uses newgrp:
        $ export BR2_DL_DIR=/some/place/dl-dir
        $ newgrp some-group
        $ make source

1. one could also use a de-duplicating filesystem, like btrfs for example.

But as I said, I'd like we also fix your use-case, and I have a series
locally that gets rid of the umask altogether. But even that will
require that participating users use an appropriate umask of course.

Regards,
Yann E. MORIN.

> Thanks,
> -Markus
> 
> > Regards,
> > Yann E. MORIN.
> >
> > >  Regards,
> > >  Arnout
> > >
> > > [snip]
> > > _______________________________________________
> > > 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.  |
> > '------------------------------^-------^------------------^--------------------'

-- 
.-----------------.--------------------.------------------.--------------------.
|  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] Config.in: add BR2_DL_DIR_OPTS

[Yann E. MORIN]

Markus, all,

On 2018-10-31 14:39 -0700, Markus Mayer spake thusly:
> Provide a simple mechanism to pass extra arguments to "mkdir" when
> creating download directories. This can be helpful if one needs the
> download directories to be writable multiple users ("shared download
> cache").

We eventually talked about this with Thomas, and we think a better
solution would be that the original umask of the user is saved before
re-entering make, and then exported so that dl-wrapper can use it when
doing the downloads.

Would you like to try and have a go at this alternate solution, instead?

Regards,
Yann E. MORIN.

> Signed-off-by: Markus Mayer <mmayer@broadcom.com>
> ---
>  Config.in               | 14 ++++++++++++++
>  package/pkg-download.mk |  2 +-
>  2 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/Config.in b/Config.in
> index 42cdf7a3ebdc..0442d8403b4a 100644
> --- a/Config.in
> +++ b/Config.in
> @@ -214,6 +214,20 @@ config BR2_DL_DIR
>  
>  	  The default is $(TOPDIR)/dl
>  
> +config BR2_DL_DIR_OPTS
> +	string "Options for mkdir when it creates BR2_DL_DIR"
> +	default ""
> +	help
> +	  Specify command line options for the mkdir command when it
> +	  creates BR2_DL_DIR.
> +
> +	  For example, this can be used to specify directory permissions
> +	  (say, "-m 0775") for directories inside BR2_DL_DIR. One would
> +	  want this if the download directory is a shared "cache" for
> +	  multiple users, so everybody can write to these directories.
> +
> +	  The default empty (no additional options).
> +
>  config BR2_HOST_DIR
>  	string "Host dir"
>  	default "$(BASE_DIR)/host"
> diff --git a/package/pkg-download.mk b/package/pkg-download.mk
> index 73ea2a69f829..d6bfe2bb06d0 100644
> --- a/package/pkg-download.mk
> +++ b/package/pkg-download.mk
> @@ -91,7 +91,7 @@ endif
>  endif
>  
>  define DOWNLOAD
> -	$(Q)mkdir -p $($(PKG)_DL_DIR)
> +	$(Q)mkdir $(BR2_DL_DIR_OPTS) -p $($(PKG)_DL_DIR)
>  	$(Q)$(EXTRA_ENV) $(FLOCK) $(DL_WRAPPER) \
>  		-c '$($(PKG)_DL_VERSION)' \
>  		-d '$($(PKG)_DL_DIR)' \
> -- 
> 2.17.1
> 
> _______________________________________________
> 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 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'