| Message ID | 20181031213930.49837-1-mmayer@broadcom.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Config.in: add BR2_DL_DIR_OPTS | expand |
> 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
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
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@busybox.net > http://lists.busybox.net/mailman/listinfo/buildroot
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, 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.
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]
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@busybox.net > http://lists.busybox.net/mailman/listinfo/buildroot
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@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. | > '------------------------------^-------^------------------^--------------------'
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@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. | > > '------------------------------^-------^------------------^--------------------'
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@busybox.net > http://lists.busybox.net/mailman/listinfo/buildroot
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)' \
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(-)