All of lore.kernel.org
 help / color / mirror / Atom feed
* [Buildroot] [PATCH v3 0/5] Target permissions
@ 2014-11-17 17:19 Guido Martínez
  2014-11-17 17:19 ` [Buildroot] [PATCH v3 1/5] Makefile: don't depend on the umask Guido Martínez
                   ` (4 more replies)
  0 siblings, 5 replies; 24+ messages in thread
From: Guido Martínez @ 2014-11-17 17:19 UTC (permalink / raw)
  To: buildroot

Hi all,

this patchset fixes the target permissions problem we observed, meaning,
it makes the target permissions not depend on the user's umask and the
current permissions of the source files on the repo. This is a problem,
since the build is not exactly reproducible unless we take all those
modes into account.

The way we accomplish this is by using the --chmod option when using
rsync -a, and by setting a fixed umask in the Makefile (previously done
by wrapping all shell calls, new in this v3 is that it's done with phony
rules only once a top level, I consider it a lot better but I'm open for
discussion, of course)

Another way to not depend on "volatile" modes would be to add a 'chmod
-R' call on the fakeroot script created in fs/common.mk, and just
expect that packages that care about their permissions set them via
$(PKG)_PERMISSIONS. However, some packages (at least: at, ssh [for
ssh-keysign], sudo and ngircd) set the correct permissions they want in
their INSTALL_CMDs, so we'd have to check each one and write the correct
permission set.

By clearing the umask, we remove the variability, while also allowing
packages to do whatever they want.

The last two patches fix two packages that used 'cp -p' from their BR
directory, so the mode on the target would depend on the those files
(for which we don't track nor care about permissions). As such, these
last two patches are independent from the others.

Only difference from v2 is that I removed the shell wrapper script and
used phony Makefile rules to set the umask, which is then inherited by
the whole build process. I believe this is better, since we don't add
any measurable work to the build and it's functionally equivalent.

Guido Mart?nez (5):
  Makefile: don't depend on the umask
  Makefile: don't depend on current skeleton/overlay permissions
  pkg-generic.mk: don't depend on external package permissions
  package: matchbox-keyboard: use install instead of cp
  package: linux-fusion: use install instead of cp

 Makefile                                              | 19 +++++++++++++++++--
 package/linux-fusion/linux-fusion.mk                  |  4 ++--
 .../matchbox/matchbox-keyboard/matchbox-keyboard.mk   |  2 +-
 package/pkg-generic.mk                                |  2 +-
 4 files changed, 21 insertions(+), 6 deletions(-)

-- 
2.1.3

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [Buildroot] [PATCH v3 1/5] Makefile: don't depend on the umask
  2014-11-17 17:19 [Buildroot] [PATCH v3 0/5] Target permissions Guido Martínez
@ 2014-11-17 17:19 ` Guido Martínez
  2014-11-17 21:11   ` Arnout Vandecappelle
  2014-11-17 21:36   ` Yann E. MORIN
  2014-11-17 17:19 ` [Buildroot] [PATCH v3 2/5] Makefile: don't depend on current skeleton/overlay permissions Guido Martínez
                   ` (3 subsequent siblings)
  4 siblings, 2 replies; 24+ messages in thread
From: Guido Martínez @ 2014-11-17 17:19 UTC (permalink / raw)
  To: buildroot

Some packages and BR itself create files and directories on the target
with cp/mkdir/etc which depend on the umask at the time of building.

To fix this, use a trick inside the Makefile which wraps all rules when
the umask is not 0022. This sets the umask at the top level, and then
the building process continues as usual.

Signed-off-by: Guido Mart?nez <guido@vanguardiasur.com.ar>
---
 Makefile | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/Makefile b/Makefile
index 471edf9..5ad8235 100644
--- a/Makefile
+++ b/Makefile
@@ -24,6 +24,19 @@
 # You shouldn't need to mess with anything beyond this point...
 #--------------------------------------------------------------
 
+# Trick for always running with a fixed umask
+UMASK=0022
+ifneq ($(shell umask),$(UMASK))
+.PHONY:*
+
+all:
+	@umask $(UMASK) && make --no-print-directory
+
+%:
+	@umask $(UMASK) && make --no-print-directory $@
+
+else # umask
+
 # This is our default rule, so must come first
 all:
 
@@ -932,3 +945,5 @@ include docs/manual/manual.mk
 -include $(BR2_EXTERNAL)/docs/*/*.mk
 
 .PHONY: $(noconfig_targets)
+
+endif #umask
-- 
2.1.3

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [Buildroot] [PATCH v3 2/5] Makefile: don't depend on current skeleton/overlay permissions
  2014-11-17 17:19 [Buildroot] [PATCH v3 0/5] Target permissions Guido Martínez
  2014-11-17 17:19 ` [Buildroot] [PATCH v3 1/5] Makefile: don't depend on the umask Guido Martínez
@ 2014-11-17 17:19 ` Guido Martínez
  2014-11-17 21:39   ` Yann E. MORIN
  2014-11-17 17:19 ` [Buildroot] [PATCH v3 3/5] pkg-generic.mk: don't depend on external package permissions Guido Martínez
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 24+ messages in thread
From: Guido Martínez @ 2014-11-17 17:19 UTC (permalink / raw)
  To: buildroot

We use 'rsync -a' to copy the skeleton and overlays, so the target ends
up with the exact same permissions as on the repo. The problem is we
don't track these permissions, since Git doesn't allow for that (except
for the exec bit). This means users with different umasks at the time of
cloning could end up with different target permissions.

Fix this by using --chmod on rsync calls so we don't depend on the
current permission set for the skeleton and overlays. We do depend on
the exec bit, but that's fine since that one is tracked by Git.

Signed-off-by: Guido Mart?nez <guido@vanguardiasur.com.ar>
---
 Makefile | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index 5ad8235..9c79dd9 100644
--- a/Makefile
+++ b/Makefile
@@ -489,7 +489,7 @@ RSYNC_VCS_EXCLUSIONS = \
 $(BUILD_DIR)/.root:
 	mkdir -p $(TARGET_DIR)
 	rsync -a --ignore-times $(RSYNC_VCS_EXCLUSIONS) \
-		--chmod=Du+w --exclude .empty --exclude '*~' \
+		--chmod=u=rwX,go=rX --exclude .empty --exclude '*~' \
 		$(TARGET_SKELETON)/ $(TARGET_DIR)/
 	$(INSTALL) -m 0644 support/misc/target-dir-warning.txt $(TARGET_DIR_WARNING_FILE)
 	@ln -snf lib $(TARGET_DIR)/$(LIB_SYMLINK)
@@ -625,7 +625,7 @@ endif
 	@$(foreach d, $(call qstrip,$(BR2_ROOTFS_OVERLAY)), \
 		$(call MESSAGE,"Copying overlay $(d)"); \
 		rsync -a --ignore-times $(RSYNC_VCS_EXCLUSIONS) \
-			--chmod=Du+w --exclude .empty --exclude '*~' \
+			--chmod=u=rwX,go=rX --exclude .empty --exclude '*~' \
 			$(d)/ $(TARGET_DIR)$(sep))
 
 	@$(foreach s, $(call qstrip,$(BR2_ROOTFS_POST_BUILD_SCRIPT)), \
-- 
2.1.3

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [Buildroot] [PATCH v3 3/5] pkg-generic.mk: don't depend on external package permissions
  2014-11-17 17:19 [Buildroot] [PATCH v3 0/5] Target permissions Guido Martínez
  2014-11-17 17:19 ` [Buildroot] [PATCH v3 1/5] Makefile: don't depend on the umask Guido Martínez
  2014-11-17 17:19 ` [Buildroot] [PATCH v3 2/5] Makefile: don't depend on current skeleton/overlay permissions Guido Martínez
@ 2014-11-17 17:19 ` Guido Martínez
  2014-11-17 21:41   ` Yann E. MORIN
  2014-11-17 17:19 ` [Buildroot] [PATCH v3 4/5] package: matchbox-keyboard: use install instead of cp Guido Martínez
  2014-11-17 17:19 ` [Buildroot] [PATCH v3 5/5] package: linux-fusion: " Guido Martínez
  4 siblings, 1 reply; 24+ messages in thread
From: Guido Martínez @ 2014-11-17 17:19 UTC (permalink / raw)
  To: buildroot

Reset permissions for rsynced packages (when using OVERRIDE_SRCDIR) to
755/644. We do this under the assumption that source files shouldn't
care about their permissions, except for possibly the exec bit.

This guarantees that if a package uses 'rsync -a' or 'cp -p' to copy
a file from its build dir to the target, it'll end up with the same
permissions on the target every time.

Signed-off-by: Guido Mart?nez <guido@vanguardiasur.com.ar>
---
 package/pkg-generic.mk | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
index 9643a30..ec2989f 100644
--- a/package/pkg-generic.mk
+++ b/package/pkg-generic.mk
@@ -117,7 +117,7 @@ $(BUILD_DIR)/%/.stamp_rsynced:
 	@$(call MESSAGE,"Syncing from source dir $(SRCDIR)")
 	@test -d $(SRCDIR) || (echo "ERROR: $(SRCDIR) does not exist" ; exit 1)
 	$(foreach hook,$($(PKG)_PRE_RSYNC_HOOKS),$(call $(hook))$(sep))
-	rsync -au $(RSYNC_VCS_EXCLUSIONS) $(SRCDIR)/ $(@D)
+	rsync -au --chmod=u=rwX,go=rX $(RSYNC_VCS_EXCLUSIONS) $(SRCDIR)/ $(@D)
 	$(foreach hook,$($(PKG)_POST_RSYNC_HOOKS),$(call $(hook))$(sep))
 	$(Q)touch $@
 
-- 
2.1.3

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [Buildroot] [PATCH v3 4/5] package: matchbox-keyboard: use install instead of cp
  2014-11-17 17:19 [Buildroot] [PATCH v3 0/5] Target permissions Guido Martínez
                   ` (2 preceding siblings ...)
  2014-11-17 17:19 ` [Buildroot] [PATCH v3 3/5] pkg-generic.mk: don't depend on external package permissions Guido Martínez
@ 2014-11-17 17:19 ` Guido Martínez
  2014-11-17 21:44   ` Yann E. MORIN
  2014-11-17 17:19 ` [Buildroot] [PATCH v3 5/5] package: linux-fusion: " Guido Martínez
  4 siblings, 1 reply; 24+ messages in thread
From: Guido Martínez @ 2014-11-17 17:19 UTC (permalink / raw)
  To: buildroot

in order to not depend on the previous permissions of the file

Signed-off-by: Guido Mart?nez <guido@vanguardiasur.com.ar>
---
 package/matchbox/matchbox-keyboard/matchbox-keyboard.mk | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/package/matchbox/matchbox-keyboard/matchbox-keyboard.mk b/package/matchbox/matchbox-keyboard/matchbox-keyboard.mk
index ebf23e4..48a50de 100644
--- a/package/matchbox/matchbox-keyboard/matchbox-keyboard.mk
+++ b/package/matchbox/matchbox-keyboard/matchbox-keyboard.mk
@@ -15,7 +15,7 @@ MATCHBOX_KEYBOARD_DEPENDENCIES = host-pkgconf matchbox-lib matchbox-fakekey expa
 MATCHBOX_KEYBOARD_CONF_ENV = expat=yes
 
 define MATCHBOX_KEYBOARD_POST_INSTALL_FIXES
-	cp -dpf ./package/matchbox/matchbox-keyboard/mb-applet-kbd-wrapper.sh $(TARGET_DIR)/usr/bin/
+	$(INSTALL) -m 0755 ./package/matchbox/matchbox-keyboard/mb-applet-kbd-wrapper.sh $(TARGET_DIR)/usr/bin/
 endef
 
 MATCHBOX_KEYBOARD_POST_INSTALL_TARGET_HOOKS += MATCHBOX_KEYBOARD_POST_INSTALL_FIXES
-- 
2.1.3

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [Buildroot] [PATCH v3 5/5] package: linux-fusion: use install instead of cp
  2014-11-17 17:19 [Buildroot] [PATCH v3 0/5] Target permissions Guido Martínez
                   ` (3 preceding siblings ...)
  2014-11-17 17:19 ` [Buildroot] [PATCH v3 4/5] package: matchbox-keyboard: use install instead of cp Guido Martínez
@ 2014-11-17 17:19 ` Guido Martínez
  2014-11-17 21:49   ` Yann E. MORIN
                     ` (2 more replies)
  4 siblings, 3 replies; 24+ messages in thread
From: Guido Martínez @ 2014-11-17 17:19 UTC (permalink / raw)
  To: buildroot

in order to not depend on the previous permissions of the file

Signed-off-by: Guido Mart?nez <guido@vanguardiasur.com.ar>
---
 package/linux-fusion/linux-fusion.mk | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/package/linux-fusion/linux-fusion.mk b/package/linux-fusion/linux-fusion.mk
index c5e7976..001388c 100644
--- a/package/linux-fusion/linux-fusion.mk
+++ b/package/linux-fusion/linux-fusion.mk
@@ -36,8 +36,8 @@ define LINUX_FUSION_INSTALL_TARGET_CMDS
 		$(LINUX_FUSION_MAKE_OPTS) \
 		INSTALL_MOD_PATH=$(TARGET_DIR) \
 		-C $(@D) install
-	mkdir -p $(LINUX_FUSION_ETC_DIR)
-	cp -dpf package/linux-fusion/40-fusion.rules $(LINUX_FUSION_ETC_DIR)
+	$(INSTALL) -D -m 644 package/linux-fusion/40-fusion.rules \
+		$(LINUX_FUSION_ETC_DIR)/40-fusion.rules
 endef
 
 $(eval $(generic-package))
-- 
2.1.3

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [Buildroot] [PATCH v3 1/5] Makefile: don't depend on the umask
  2014-11-17 17:19 ` [Buildroot] [PATCH v3 1/5] Makefile: don't depend on the umask Guido Martínez
@ 2014-11-17 21:11   ` Arnout Vandecappelle
  2014-11-18 17:01     ` Guido Martínez
  2014-11-17 21:36   ` Yann E. MORIN
  1 sibling, 1 reply; 24+ messages in thread
From: Arnout Vandecappelle @ 2014-11-17 21:11 UTC (permalink / raw)
  To: buildroot

On 17/11/14 18:19, Guido Mart?nez wrote:
> Some packages and BR itself create files and directories on the target
> with cp/mkdir/etc which depend on the umask at the time of building.
> 
> To fix this, use a trick inside the Makefile which wraps all rules when
> the umask is not 0022. This sets the umask at the top level, and then
> the building process continues as usual.
> 
> Signed-off-by: Guido Mart?nez <guido@vanguardiasur.com.ar>
> ---
>  Makefile | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/Makefile b/Makefile
> index 471edf9..5ad8235 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -24,6 +24,19 @@
>  # You shouldn't need to mess with anything beyond this point...
>  #--------------------------------------------------------------
>  
> +# Trick for always running with a fixed umask
> +UMASK=0022
> +ifneq ($(shell umask),$(UMASK))
> +.PHONY:*

 What is this * supposed to do? It will expand to all existing files and
directories in the buildroot directory... Don't you mean:

.PHONY: all $(MAKECMDGOALS)

?


 Otherwise, looks good.

 BTW wouldn't it be nice to add the umask setting to the generated Makefile in
the $(O) directory, to avoid another level of make?


 Regards,
 Arnout

> +
> +all:
> +	@umask $(UMASK) && make --no-print-directory
> +
> +%:
> +	@umask $(UMASK) && make --no-print-directory $@
> +
> +else # umask
> +
>  # This is our default rule, so must come first
>  all:
>  
> @@ -932,3 +945,5 @@ include docs/manual/manual.mk
>  -include $(BR2_EXTERNAL)/docs/*/*.mk
>  
>  .PHONY: $(noconfig_targets)
> +
> +endif #umask
> 


-- 
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:  7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [Buildroot] [PATCH v3 1/5] Makefile: don't depend on the umask
  2014-11-17 17:19 ` [Buildroot] [PATCH v3 1/5] Makefile: don't depend on the umask Guido Martínez
  2014-11-17 21:11   ` Arnout Vandecappelle
@ 2014-11-17 21:36   ` Yann E. MORIN
  2014-11-17 21:41     ` Arnout Vandecappelle
  1 sibling, 1 reply; 24+ messages in thread
From: Yann E. MORIN @ 2014-11-17 21:36 UTC (permalink / raw)
  To: buildroot

Guido, All,

On 2014-11-17 14:19 -0300, Guido Mart?nez spake thusly:
> Some packages and BR itself create files and directories on the target
> with cp/mkdir/etc which depend on the umask at the time of building.
> 
> To fix this, use a trick inside the Makefile which wraps all rules when
> the umask is not 0022. This sets the umask at the top level, and then
> the building process continues as usual.
> 
> Signed-off-by: Guido Mart?nez <guido@vanguardiasur.com.ar>
> ---
>  Makefile | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/Makefile b/Makefile
> index 471edf9..5ad8235 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -24,6 +24,19 @@
>  # You shouldn't need to mess with anything beyond this point...
>  #--------------------------------------------------------------
>  
> +# Trick for always running with a fixed umask
> +UMASK=0022
> +ifneq ($(shell umask),$(UMASK))
> +.PHONY:*
> +
> +all:
> +	@umask $(UMASK) && make --no-print-directory

You should use $(MAKE) instead of just 'make'.

I'm not too fond of --no-print-directory, because it is gonna be
inherited by all sub-makes, and especially the sub-makes that actually
build the packages. I don't care we have an extra "Entering directory"
message.

Also, did you check how that plays with top-level parallel builds? I'm
not too fond of it, but some users want to use it. See the comment a bit
further in the Makefile.

> +
> +%:
> +	@umask $(UMASK) && make --no-print-directory $@

Ditto.

Regards,
Yann E. MORIN.

> +else # umask
> +
>  # This is our default rule, so must come first
>  all:
>  
> @@ -932,3 +945,5 @@ include docs/manual/manual.mk
>  -include $(BR2_EXTERNAL)/docs/*/*.mk
>  
>  .PHONY: $(noconfig_targets)
> +
> +endif #umask
> -- 
> 2.1.3
> 

-- 
.-----------------.--------------------.------------------.--------------------.
|  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.  |
'------------------------------^-------^------------------^--------------------'

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [Buildroot] [PATCH v3 2/5] Makefile: don't depend on current skeleton/overlay permissions
  2014-11-17 17:19 ` [Buildroot] [PATCH v3 2/5] Makefile: don't depend on current skeleton/overlay permissions Guido Martínez
@ 2014-11-17 21:39   ` Yann E. MORIN
  0 siblings, 0 replies; 24+ messages in thread
From: Yann E. MORIN @ 2014-11-17 21:39 UTC (permalink / raw)
  To: buildroot

Guido, All,

On 2014-11-17 14:19 -0300, Guido Mart?nez spake thusly:
> We use 'rsync -a' to copy the skeleton and overlays, so the target ends
> up with the exact same permissions as on the repo. The problem is we
> don't track these permissions, since Git doesn't allow for that (except
> for the exec bit). This means users with different umasks at the time of
> cloning could end up with different target permissions.
> 
> Fix this by using --chmod on rsync calls so we don't depend on the
> current permission set for the skeleton and overlays. We do depend on
> the exec bit, but that's fine since that one is tracked by Git.
> 
> Signed-off-by: Guido Mart?nez <guido@vanguardiasur.com.ar>

Reviewed-by: "Yann E. MORIN" <yann.morin.1998@free.fr>

Regards,
Yann E. MORIN.

> ---
>  Makefile | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 5ad8235..9c79dd9 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -489,7 +489,7 @@ RSYNC_VCS_EXCLUSIONS = \
>  $(BUILD_DIR)/.root:
>  	mkdir -p $(TARGET_DIR)
>  	rsync -a --ignore-times $(RSYNC_VCS_EXCLUSIONS) \
> -		--chmod=Du+w --exclude .empty --exclude '*~' \
> +		--chmod=u=rwX,go=rX --exclude .empty --exclude '*~' \
>  		$(TARGET_SKELETON)/ $(TARGET_DIR)/
>  	$(INSTALL) -m 0644 support/misc/target-dir-warning.txt $(TARGET_DIR_WARNING_FILE)
>  	@ln -snf lib $(TARGET_DIR)/$(LIB_SYMLINK)
> @@ -625,7 +625,7 @@ endif
>  	@$(foreach d, $(call qstrip,$(BR2_ROOTFS_OVERLAY)), \
>  		$(call MESSAGE,"Copying overlay $(d)"); \
>  		rsync -a --ignore-times $(RSYNC_VCS_EXCLUSIONS) \
> -			--chmod=Du+w --exclude .empty --exclude '*~' \
> +			--chmod=u=rwX,go=rX --exclude .empty --exclude '*~' \
>  			$(d)/ $(TARGET_DIR)$(sep))
>  
>  	@$(foreach s, $(call qstrip,$(BR2_ROOTFS_POST_BUILD_SCRIPT)), \
> -- 
> 2.1.3
> 

-- 
.-----------------.--------------------.------------------.--------------------.
|  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.  |
'------------------------------^-------^------------------^--------------------'

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [Buildroot] [PATCH v3 3/5] pkg-generic.mk: don't depend on external package permissions
  2014-11-17 17:19 ` [Buildroot] [PATCH v3 3/5] pkg-generic.mk: don't depend on external package permissions Guido Martínez
@ 2014-11-17 21:41   ` Yann E. MORIN
  2014-11-18 17:02     ` Guido Martínez
  0 siblings, 1 reply; 24+ messages in thread
From: Yann E. MORIN @ 2014-11-17 21:41 UTC (permalink / raw)
  To: buildroot

Guido, All,

On 2014-11-17 14:19 -0300, Guido Mart?nez spake thusly:
> Reset permissions for rsynced packages (when using OVERRIDE_SRCDIR) to
> 755/644. We do this under the assumption that source files shouldn't
> care about their permissions, except for possibly the exec bit.

Nit-picking:  except possibly for...

> This guarantees that if a package uses 'rsync -a' or 'cp -p' to copy
> a file from its build dir to the target, it'll end up with the same
> permissions on the target every time.
> 
> Signed-off-by: Guido Mart?nez <guido@vanguardiasur.com.ar>

Reviewed-by: "Yann E. MORIN" <yann.morin.1998@free.fr>

Regards,
Yann E. MORIN.

> ---
>  package/pkg-generic.mk | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> index 9643a30..ec2989f 100644
> --- a/package/pkg-generic.mk
> +++ b/package/pkg-generic.mk
> @@ -117,7 +117,7 @@ $(BUILD_DIR)/%/.stamp_rsynced:
>  	@$(call MESSAGE,"Syncing from source dir $(SRCDIR)")
>  	@test -d $(SRCDIR) || (echo "ERROR: $(SRCDIR) does not exist" ; exit 1)
>  	$(foreach hook,$($(PKG)_PRE_RSYNC_HOOKS),$(call $(hook))$(sep))
> -	rsync -au $(RSYNC_VCS_EXCLUSIONS) $(SRCDIR)/ $(@D)
> +	rsync -au --chmod=u=rwX,go=rX $(RSYNC_VCS_EXCLUSIONS) $(SRCDIR)/ $(@D)
>  	$(foreach hook,$($(PKG)_POST_RSYNC_HOOKS),$(call $(hook))$(sep))
>  	$(Q)touch $@
>  
> -- 
> 2.1.3
> 

-- 
.-----------------.--------------------.------------------.--------------------.
|  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.  |
'------------------------------^-------^------------------^--------------------'

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [Buildroot] [PATCH v3 1/5] Makefile: don't depend on the umask
  2014-11-17 21:36   ` Yann E. MORIN
@ 2014-11-17 21:41     ` Arnout Vandecappelle
  2014-11-17 21:48       ` Yann E. MORIN
  0 siblings, 1 reply; 24+ messages in thread
From: Arnout Vandecappelle @ 2014-11-17 21:41 UTC (permalink / raw)
  To: buildroot

On 17/11/14 22:36, Yann E. MORIN wrote:
> Guido, All,
> 
> On 2014-11-17 14:19 -0300, Guido Mart?nez spake thusly:
>> Some packages and BR itself create files and directories on the target
>> with cp/mkdir/etc which depend on the umask at the time of building.
>>
>> To fix this, use a trick inside the Makefile which wraps all rules when
>> the umask is not 0022. This sets the umask at the top level, and then
>> the building process continues as usual.
>>
>> Signed-off-by: Guido Mart?nez <guido@vanguardiasur.com.ar>
>> ---
>>  Makefile | 15 +++++++++++++++
>>  1 file changed, 15 insertions(+)
>>
>> diff --git a/Makefile b/Makefile
>> index 471edf9..5ad8235 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -24,6 +24,19 @@
>>  # You shouldn't need to mess with anything beyond this point...
>>  #--------------------------------------------------------------
>>  
>> +# Trick for always running with a fixed umask
>> +UMASK=0022
>> +ifneq ($(shell umask),$(UMASK))
>> +.PHONY:*
>> +
>> +all:
>> +	@umask $(UMASK) && make --no-print-directory
> 
> You should use $(MAKE) instead of just 'make'.
> 
> I'm not too fond of --no-print-directory, because it is gonna be
> inherited by all sub-makes, and especially the sub-makes that actually
> build the packages. I don't care we have an extra "Entering directory"
> message.

 Note that the generated Makefile in $(O) does add --no-print-directory...


 Regards,
 Arnout

> 
> Also, did you check how that plays with top-level parallel builds? I'm
> not too fond of it, but some users want to use it. See the comment a bit
> further in the Makefile.
> 
>> +
>> +%:
>> +	@umask $(UMASK) && make --no-print-directory $@
> 
> Ditto.
> 
> Regards,
> Yann E. MORIN.
> 
>> +else # umask
>> +
>>  # This is our default rule, so must come first
>>  all:
>>  
>> @@ -932,3 +945,5 @@ include docs/manual/manual.mk
>>  -include $(BR2_EXTERNAL)/docs/*/*.mk
>>  
>>  .PHONY: $(noconfig_targets)
>> +
>> +endif #umask
>> -- 
>> 2.1.3
>>
> 


-- 
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:  7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [Buildroot] [PATCH v3 4/5] package: matchbox-keyboard: use install instead of cp
  2014-11-17 17:19 ` [Buildroot] [PATCH v3 4/5] package: matchbox-keyboard: use install instead of cp Guido Martínez
@ 2014-11-17 21:44   ` Yann E. MORIN
  2014-11-18 17:06     ` Guido Martínez
  0 siblings, 1 reply; 24+ messages in thread
From: Yann E. MORIN @ 2014-11-17 21:44 UTC (permalink / raw)
  To: buildroot

Guido, All,

On 2014-11-17 14:19 -0300, Guido Mart?nez spake thusly:
> in order to not depend on the previous permissions of the file
> 
> Signed-off-by: Guido Mart?nez <guido@vanguardiasur.com.ar>
> ---
>  package/matchbox/matchbox-keyboard/matchbox-keyboard.mk | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/package/matchbox/matchbox-keyboard/matchbox-keyboard.mk b/package/matchbox/matchbox-keyboard/matchbox-keyboard.mk
> index ebf23e4..48a50de 100644
> --- a/package/matchbox/matchbox-keyboard/matchbox-keyboard.mk
> +++ b/package/matchbox/matchbox-keyboard/matchbox-keyboard.mk
> @@ -15,7 +15,7 @@ MATCHBOX_KEYBOARD_DEPENDENCIES = host-pkgconf matchbox-lib matchbox-fakekey expa
>  MATCHBOX_KEYBOARD_CONF_ENV = expat=yes
>  
>  define MATCHBOX_KEYBOARD_POST_INSTALL_FIXES
> -	cp -dpf ./package/matchbox/matchbox-keyboard/mb-applet-kbd-wrapper.sh $(TARGET_DIR)/usr/bin/
> +	$(INSTALL) -m 0755 ./package/matchbox/matchbox-keyboard/mb-applet-kbd-wrapper.sh $(TARGET_DIR)/usr/bin/

This should be:

    $(INSTALL) -D -m 0755 package/matchbox/matchbox-keyboard/mb-applet-kbd-wrapper.sh \
                          $(TARGET_DIR)/usr/bin/mb-applet-kbd-wrapper.sh

That is:
  - full path to the destination file. Otherwise, if $(TARGET_DIR)/usr/bin
    does not exist (in case of a custom skeleton, for example),
    'install' will create a file named "$(TARGET_DIR)/usr/bin" instead
    of creating a directory and copying into it;
  - the leading ./ to the source is not needed.

Regards,
Yann E. MORIN.

>  endef
>  
>  MATCHBOX_KEYBOARD_POST_INSTALL_TARGET_HOOKS += MATCHBOX_KEYBOARD_POST_INSTALL_FIXES
> -- 
> 2.1.3
> 

-- 
.-----------------.--------------------.------------------.--------------------.
|  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.  |
'------------------------------^-------^------------------^--------------------'

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [Buildroot] [PATCH v3 1/5] Makefile: don't depend on the umask
  2014-11-17 21:41     ` Arnout Vandecappelle
@ 2014-11-17 21:48       ` Yann E. MORIN
  0 siblings, 0 replies; 24+ messages in thread
From: Yann E. MORIN @ 2014-11-17 21:48 UTC (permalink / raw)
  To: buildroot

Arnout, All,

On 2014-11-17 22:41 +0100, Arnout Vandecappelle spake thusly:
> On 17/11/14 22:36, Yann E. MORIN wrote:
> > On 2014-11-17 14:19 -0300, Guido Mart?nez spake thusly:
[--SNIP--]
> >> +all:
> >> +	@umask $(UMASK) && make --no-print-directory
[--SNIP--]
> > I'm not too fond of --no-print-directory, because it is gonna be
> > inherited by all sub-makes, and especially the sub-makes that actually
> > build the packages. I don't care we have an extra "Entering directory"
> > message.
> 
>  Note that the generated Makefile in $(O) does add --no-print-directory...

And guess who added that generated file?

    # git blame support/scripts/mkmakefile
    [...]
    aefad531 scripts/mkmakefile         (Yann E. MORIN    2010-09-26 10:56:12 +0200 31) MAKEFLAGS += --no-print-directory
    [...]

Ok, Ok. :-)

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.  |
'------------------------------^-------^------------------^--------------------'

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [Buildroot] [PATCH v3 5/5] package: linux-fusion: use install instead of cp
  2014-11-17 17:19 ` [Buildroot] [PATCH v3 5/5] package: linux-fusion: " Guido Martínez
@ 2014-11-17 21:49   ` Yann E. MORIN
  2014-11-17 22:06   ` Arnout Vandecappelle
  2014-11-18 22:59   ` Thomas Petazzoni
  2 siblings, 0 replies; 24+ messages in thread
From: Yann E. MORIN @ 2014-11-17 21:49 UTC (permalink / raw)
  To: buildroot

Guido, All,

On 2014-11-17 14:19 -0300, Guido Mart?nez spake thusly:
> in order to not depend on the previous permissions of the file
> 
> Signed-off-by: Guido Mart?nez <guido@vanguardiasur.com.ar>

Reviewed-by: "Yann E. MORIN" <yann.morin.1998@free.fr>

But see a comment below...

> ---
>  package/linux-fusion/linux-fusion.mk | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/package/linux-fusion/linux-fusion.mk b/package/linux-fusion/linux-fusion.mk
> index c5e7976..001388c 100644
> --- a/package/linux-fusion/linux-fusion.mk
> +++ b/package/linux-fusion/linux-fusion.mk
> @@ -36,8 +36,8 @@ define LINUX_FUSION_INSTALL_TARGET_CMDS
>  		$(LINUX_FUSION_MAKE_OPTS) \
>  		INSTALL_MOD_PATH=$(TARGET_DIR) \
>  		-C $(@D) install
> -	mkdir -p $(LINUX_FUSION_ETC_DIR)
> -	cp -dpf package/linux-fusion/40-fusion.rules $(LINUX_FUSION_ETC_DIR)
> +	$(INSTALL) -D -m 644 package/linux-fusion/40-fusion.rules \
> +		$(LINUX_FUSION_ETC_DIR)/40-fusion.rules

Here, you used a correct construct when calling isntall. ;-)

Regards,
Yann E. MORIN.

>  endef
>  
>  $(eval $(generic-package))
> -- 
> 2.1.3
> 

-- 
.-----------------.--------------------.------------------.--------------------.
|  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.  |
'------------------------------^-------^------------------^--------------------'

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [Buildroot] [PATCH v3 5/5] package: linux-fusion: use install instead of cp
  2014-11-17 17:19 ` [Buildroot] [PATCH v3 5/5] package: linux-fusion: " Guido Martínez
  2014-11-17 21:49   ` Yann E. MORIN
@ 2014-11-17 22:06   ` Arnout Vandecappelle
  2014-11-18 17:17     ` Guido Martínez
  2014-11-18 22:59   ` Thomas Petazzoni
  2 siblings, 1 reply; 24+ messages in thread
From: Arnout Vandecappelle @ 2014-11-17 22:06 UTC (permalink / raw)
  To: buildroot

On 17/11/14 18:19, Guido Mart?nez wrote:
> in order to not depend on the previous permissions of the file
> 
> Signed-off-by: Guido Mart?nez <guido@vanguardiasur.com.ar>

Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>

 There are a few more suspicious cp instances:

board/boundarydevices/nitrogen6x/post-build.sh
boot/grub/grub.mk
fs/iso9660/iso9660.mk


 Also, it would be good to document somewhere (e.g. in the intro mail) why the
rsync in the toolchain is OK. Or even better, replace it even though it's not
really necessary - it just feels more consistent and safe.


 Regards,
 Arnout

> ---
>  package/linux-fusion/linux-fusion.mk | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/package/linux-fusion/linux-fusion.mk b/package/linux-fusion/linux-fusion.mk
> index c5e7976..001388c 100644
> --- a/package/linux-fusion/linux-fusion.mk
> +++ b/package/linux-fusion/linux-fusion.mk
> @@ -36,8 +36,8 @@ define LINUX_FUSION_INSTALL_TARGET_CMDS
>  		$(LINUX_FUSION_MAKE_OPTS) \
>  		INSTALL_MOD_PATH=$(TARGET_DIR) \
>  		-C $(@D) install
> -	mkdir -p $(LINUX_FUSION_ETC_DIR)
> -	cp -dpf package/linux-fusion/40-fusion.rules $(LINUX_FUSION_ETC_DIR)
> +	$(INSTALL) -D -m 644 package/linux-fusion/40-fusion.rules \
> +		$(LINUX_FUSION_ETC_DIR)/40-fusion.rules
>  endef
>  
>  $(eval $(generic-package))
> 


-- 
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:  7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [Buildroot] [PATCH v3 1/5] Makefile: don't depend on the umask
  2014-11-17 21:11   ` Arnout Vandecappelle
@ 2014-11-18 17:01     ` Guido Martínez
  2014-11-18 19:22       ` Arnout Vandecappelle
  0 siblings, 1 reply; 24+ messages in thread
From: Guido Martínez @ 2014-11-18 17:01 UTC (permalink / raw)
  To: buildroot

Hi Arnout, all,

On Mon, Nov 17, 2014 at 10:11:50PM +0100, Arnout Vandecappelle wrote:
> On 17/11/14 18:19, Guido Mart?nez wrote:
> > Some packages and BR itself create files and directories on the target
> > with cp/mkdir/etc which depend on the umask at the time of building.
> > 
> > To fix this, use a trick inside the Makefile which wraps all rules when
> > the umask is not 0022. This sets the umask at the top level, and then
> > the building process continues as usual.
> > 
> > Signed-off-by: Guido Mart?nez <guido@vanguardiasur.com.ar>
> > ---
> >  Makefile | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> > 
> > diff --git a/Makefile b/Makefile
> > index 471edf9..5ad8235 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -24,6 +24,19 @@
> >  # You shouldn't need to mess with anything beyond this point...
> >  #--------------------------------------------------------------
> >  
> > +# Trick for always running with a fixed umask
> > +UMASK=0022
> > +ifneq ($(shell umask),$(UMASK))
> > +.PHONY:*
> 
>  What is this * supposed to do? It will expand to all existing files and
> directories in the buildroot directory... Don't you mean:
> 
> .PHONY: all $(MAKECMDGOALS)
> 
> ?
Right, it was supposed to expand to existing files so we always run the
rules, but your solution is better. In fact, I tried touching a file
named "clean" and the "phoniness" didn't actually work, since apparently
"%" isn't expanded for phony rules, so I used $(MAKECMDGOALS) for the
rule too.

With Yann's comment the first patch would then be:

"""
diff --git a/Makefile b/Makefile
index 471edf9..054a6be 100644
--- a/Makefile
+++ b/Makefile
@@ -24,6 +24,19 @@
 # You shouldn't need to mess with anything beyond this point...
 #--------------------------------------------------------------
 
+# Trick for always running with a fixed umask
+UMASK=0022
+ifneq ($(shell umask),$(UMASK))
+.PHONY: all $(MAKECMDGOALS)
+
+all:
+       @umask $(UMASK) && $(MAKE)
+
+$(MAKECMDGOALS):
+       @umask $(UMASK) && $(MAKE) $@
+
+else # umask
+
 # This is our default rule, so must come first
 all:
 
@@ -932,3 +945,5 @@ include docs/manual/manual.mk
 -include $(BR2_EXTERNAL)/docs/*/*.mk
 
 .PHONY: $(noconfig_targets)
+
+endif #umask
"""

Does that sound good?

>  Otherwise, looks good.
> 
>  BTW wouldn't it be nice to add the umask setting to the generated Makefile in
> the $(O) directory, to avoid another level of make?
Sure, will do.

Thanks!

-- 
Guido Mart?nez, VanguardiaSur
www.vanguardiasur.com.ar

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [Buildroot] [PATCH v3 3/5] pkg-generic.mk: don't depend on external package permissions
  2014-11-17 21:41   ` Yann E. MORIN
@ 2014-11-18 17:02     ` Guido Martínez
  0 siblings, 0 replies; 24+ messages in thread
From: Guido Martínez @ 2014-11-18 17:02 UTC (permalink / raw)
  To: buildroot

On Mon, Nov 17, 2014 at 10:41:06PM +0100, Yann E. MORIN wrote:
> Guido, All,
> 
> On 2014-11-17 14:19 -0300, Guido Mart?nez spake thusly:
> > Reset permissions for rsynced packages (when using OVERRIDE_SRCDIR) to
> > 755/644. We do this under the assumption that source files shouldn't
> > care about their permissions, except for possibly the exec bit.
> 
> Nit-picking:  except possibly for...
Oops :)! Fixed

Thanks!

-- 
Guido Mart?nez, VanguardiaSur
www.vanguardiasur.com.ar

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [Buildroot] [PATCH v3 4/5] package: matchbox-keyboard: use install instead of cp
  2014-11-17 21:44   ` Yann E. MORIN
@ 2014-11-18 17:06     ` Guido Martínez
  0 siblings, 0 replies; 24+ messages in thread
From: Guido Martínez @ 2014-11-18 17:06 UTC (permalink / raw)
  To: buildroot

Hi Yann, all,

On Mon, Nov 17, 2014 at 10:44:46PM +0100, Yann E. MORIN wrote:
> Guido, All,
> 
> On 2014-11-17 14:19 -0300, Guido Mart?nez spake thusly:
> > in order to not depend on the previous permissions of the file
> > 
> > Signed-off-by: Guido Mart?nez <guido@vanguardiasur.com.ar>
> > ---
> >  package/matchbox/matchbox-keyboard/matchbox-keyboard.mk | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/package/matchbox/matchbox-keyboard/matchbox-keyboard.mk b/package/matchbox/matchbox-keyboard/matchbox-keyboard.mk
> > index ebf23e4..48a50de 100644
> > --- a/package/matchbox/matchbox-keyboard/matchbox-keyboard.mk
> > +++ b/package/matchbox/matchbox-keyboard/matchbox-keyboard.mk
> > @@ -15,7 +15,7 @@ MATCHBOX_KEYBOARD_DEPENDENCIES = host-pkgconf matchbox-lib matchbox-fakekey expa
> >  MATCHBOX_KEYBOARD_CONF_ENV = expat=yes
> >  
> >  define MATCHBOX_KEYBOARD_POST_INSTALL_FIXES
> > -	cp -dpf ./package/matchbox/matchbox-keyboard/mb-applet-kbd-wrapper.sh $(TARGET_DIR)/usr/bin/
> > +	$(INSTALL) -m 0755 ./package/matchbox/matchbox-keyboard/mb-applet-kbd-wrapper.sh $(TARGET_DIR)/usr/bin/
> 
> This should be:
> 
>     $(INSTALL) -D -m 0755 package/matchbox/matchbox-keyboard/mb-applet-kbd-wrapper.sh \
>                           $(TARGET_DIR)/usr/bin/mb-applet-kbd-wrapper.sh
> 
> That is:
>   - full path to the destination file. Otherwise, if $(TARGET_DIR)/usr/bin
>     does not exist (in case of a custom skeleton, for example),
>     'install' will create a file named "$(TARGET_DIR)/usr/bin" instead
>     of creating a directory and copying into it;
>   - the leading ./ to the source is not needed.
Actually the trailing slash prevents the file being created as /usr/bin,
but the build would fail anyway. I changed it to your suggestion to be
skeleton-agnostic.

Thanks!

-- 
Guido Mart?nez, VanguardiaSur
www.vanguardiasur.com.ar

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [Buildroot] [PATCH v3 5/5] package: linux-fusion: use install instead of cp
  2014-11-17 22:06   ` Arnout Vandecappelle
@ 2014-11-18 17:17     ` Guido Martínez
  2014-11-18 19:23       ` Arnout Vandecappelle
  0 siblings, 1 reply; 24+ messages in thread
From: Guido Martínez @ 2014-11-18 17:17 UTC (permalink / raw)
  To: buildroot

Hi Arnout, all

On Mon, Nov 17, 2014 at 11:06:05PM +0100, Arnout Vandecappelle wrote:
> On 17/11/14 18:19, Guido Mart?nez wrote:
> > in order to not depend on the previous permissions of the file
> > 
> > Signed-off-by: Guido Mart?nez <guido@vanguardiasur.com.ar>
> 
> Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
> 
>  There are a few more suspicious cp instances:
> 
> board/boundarydevices/nitrogen6x/post-build.sh
> boot/grub/grub.mk
> fs/iso9660/iso9660.mk
I'll take a look at these, thanks.

>  Also, it would be good to document somewhere (e.g. in the intro mail) why the
> rsync in the toolchain is OK. Or even better, replace it even though it's not
> really necessary - it just feels more consistent and safe.
Yes, this sounds good. Maybe use --chmod on the toolchain and add a
comment there? This way we should be covered in the future if someone
uses 'rsync -a' from there.

Cheers!

-- 
Guido Mart?nez, VanguardiaSur
www.vanguardiasur.com.ar

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [Buildroot] [PATCH v3 1/5] Makefile: don't depend on the umask
  2014-11-18 17:01     ` Guido Martínez
@ 2014-11-18 19:22       ` Arnout Vandecappelle
  0 siblings, 0 replies; 24+ messages in thread
From: Arnout Vandecappelle @ 2014-11-18 19:22 UTC (permalink / raw)
  To: buildroot

On 18/11/14 18:01, Guido Mart?nez wrote:
> Hi Arnout, all,
> 
> On Mon, Nov 17, 2014 at 10:11:50PM +0100, Arnout Vandecappelle wrote:
>> On 17/11/14 18:19, Guido Mart?nez wrote:
>>> Some packages and BR itself create files and directories on the target
>>> with cp/mkdir/etc which depend on the umask at the time of building.
>>>
>>> To fix this, use a trick inside the Makefile which wraps all rules when
>>> the umask is not 0022. This sets the umask at the top level, and then
>>> the building process continues as usual.
>>>
>>> Signed-off-by: Guido Mart?nez <guido@vanguardiasur.com.ar>
>>> ---
>>>  Makefile | 15 +++++++++++++++
>>>  1 file changed, 15 insertions(+)
>>>
>>> diff --git a/Makefile b/Makefile
>>> index 471edf9..5ad8235 100644
>>> --- a/Makefile
>>> +++ b/Makefile
>>> @@ -24,6 +24,19 @@
>>>  # You shouldn't need to mess with anything beyond this point...
>>>  #--------------------------------------------------------------
>>>  
>>> +# Trick for always running with a fixed umask
>>> +UMASK=0022
>>> +ifneq ($(shell umask),$(UMASK))
>>> +.PHONY:*
>>
>>  What is this * supposed to do? It will expand to all existing files and
>> directories in the buildroot directory... Don't you mean:
>>
>> .PHONY: all $(MAKECMDGOALS)
>>
>> ?
> Right, it was supposed to expand to existing files so we always run the
> rules, but your solution is better. In fact, I tried touching a file
> named "clean" and the "phoniness" didn't actually work, since apparently
> "%" isn't expanded for phony rules, so I used $(MAKECMDGOALS) for the
> rule too.
> 
> With Yann's comment the first patch would then be:
> 
> """
> diff --git a/Makefile b/Makefile
> index 471edf9..054a6be 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -24,6 +24,19 @@
>  # You shouldn't need to mess with anything beyond this point...
>  #--------------------------------------------------------------
>  
> +# Trick for always running with a fixed umask
> +UMASK=0022
> +ifneq ($(shell umask),$(UMASK))
> +.PHONY: all $(MAKECMDGOALS)
> +
> +all:
> +       @umask $(UMASK) && $(MAKE)
> +
> +$(MAKECMDGOALS):
> +       @umask $(UMASK) && $(MAKE) $@
> +
> +else # umask
> +
>  # This is our default rule, so must come first
>  all:
>  
> @@ -932,3 +945,5 @@ include docs/manual/manual.mk
>  -include $(BR2_EXTERNAL)/docs/*/*.mk
>  
>  .PHONY: $(noconfig_targets)
> +
> +endif #umask
> """
> 
> Does that sound good?

 Yes, sounds good.


 Regards,
 Arnout

> 
>>  Otherwise, looks good.
>>
>>  BTW wouldn't it be nice to add the umask setting to the generated Makefile in
>> the $(O) directory, to avoid another level of make?
> Sure, will do.
> 
> Thanks!
> 


-- 
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:  7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [Buildroot] [PATCH v3 5/5] package: linux-fusion: use install instead of cp
  2014-11-18 17:17     ` Guido Martínez
@ 2014-11-18 19:23       ` Arnout Vandecappelle
  2014-11-18 19:40         ` Guido Martínez
  0 siblings, 1 reply; 24+ messages in thread
From: Arnout Vandecappelle @ 2014-11-18 19:23 UTC (permalink / raw)
  To: buildroot

On 18/11/14 18:17, Guido Mart?nez wrote:
> Hi Arnout, all
> 
> On Mon, Nov 17, 2014 at 11:06:05PM +0100, Arnout Vandecappelle wrote:
>> On 17/11/14 18:19, Guido Mart?nez wrote:
>>> in order to not depend on the previous permissions of the file
>>>
>>> Signed-off-by: Guido Mart?nez <guido@vanguardiasur.com.ar>
>>
>> Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
>>
>>  There are a few more suspicious cp instances:
>>
>> board/boundarydevices/nitrogen6x/post-build.sh
>> boot/grub/grub.mk
>> fs/iso9660/iso9660.mk
> I'll take a look at these, thanks.
> 
>>  Also, it would be good to document somewhere (e.g. in the intro mail) why the
>> rsync in the toolchain is OK. Or even better, replace it even though it's not
>> really necessary - it just feels more consistent and safe.
> Yes, this sounds good. Maybe use --chmod on the toolchain and add a
> comment there? This way we should be covered in the future if someone
> uses 'rsync -a' from there.

 Actually the toolchain already uses --chmod=Du+w.


 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:  7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [Buildroot] [PATCH v3 5/5] package: linux-fusion: use install instead of cp
  2014-11-18 19:23       ` Arnout Vandecappelle
@ 2014-11-18 19:40         ` Guido Martínez
  2014-11-18 20:58           ` Arnout Vandecappelle
  0 siblings, 1 reply; 24+ messages in thread
From: Guido Martínez @ 2014-11-18 19:40 UTC (permalink / raw)
  To: buildroot

On Tue, Nov 18, 2014 at 08:23:43PM +0100, Arnout Vandecappelle wrote:
> On 18/11/14 18:17, Guido Mart?nez wrote:
> > Hi Arnout, all
> > 
> > On Mon, Nov 17, 2014 at 11:06:05PM +0100, Arnout Vandecappelle wrote:
> >> On 17/11/14 18:19, Guido Mart?nez wrote:
> >>> in order to not depend on the previous permissions of the file
> >>>
> >>> Signed-off-by: Guido Mart?nez <guido@vanguardiasur.com.ar>
> >>
> >> Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
> >>
> >>  There are a few more suspicious cp instances:
> >>
> >> board/boundarydevices/nitrogen6x/post-build.sh
> >> boot/grub/grub.mk
> >> fs/iso9660/iso9660.mk
> > I'll take a look at these, thanks.
> > 
> >>  Also, it would be good to document somewhere (e.g. in the intro mail) why the
> >> rsync in the toolchain is OK. Or even better, replace it even though it's not
> >> really necessary - it just feels more consistent and safe.
> > Yes, this sounds good. Maybe use --chmod on the toolchain and add a
> > comment there? This way we should be covered in the future if someone
> > uses 'rsync -a' from there.
> 
>  Actually the toolchain already uses --chmod=Du+w.
Yes, but that doesn't have any effect on files, so files under
output/staging can have just about any mode, and more so when the
toolchain is preinstalled somewhere.

Changing it to --chmod=u=rwX,go=rX would eliminate that variablity
(again, we're assuming that there are no 'random' exec bits set, but
that's reasonable since no files are created with the exec bit set).

This way, at any place within BR we could do 'rsync -a
$(STAGING_DIR)/... $(TARGET_DIR)/...' and have well-defined modes on the
target.

That kind of rsync (or any other similar copy) don't exist as of now:
eveything is done via 'install'. So I don't have a strong opinion for
changing the rsync or not.

What do you guys think?
Thanks!

-- 
Guido Mart?nez, VanguardiaSur
www.vanguardiasur.com.ar

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [Buildroot] [PATCH v3 5/5] package: linux-fusion: use install instead of cp
  2014-11-18 19:40         ` Guido Martínez
@ 2014-11-18 20:58           ` Arnout Vandecappelle
  0 siblings, 0 replies; 24+ messages in thread
From: Arnout Vandecappelle @ 2014-11-18 20:58 UTC (permalink / raw)
  To: buildroot

On 18/11/14 20:40, Guido Mart?nez wrote:
> On Tue, Nov 18, 2014 at 08:23:43PM +0100, Arnout Vandecappelle wrote:
> > On 18/11/14 18:17, Guido Mart?nez wrote:
> >> Hi Arnout, all
> >>
> >> On Mon, Nov 17, 2014 at 11:06:05PM +0100, Arnout Vandecappelle wrote:
> >>> On 17/11/14 18:19, Guido Mart?nez wrote:
> >>>> in order to not depend on the previous permissions of the file
> >>>>
> >>>> Signed-off-by: Guido Mart?nez <guido@vanguardiasur.com.ar>
> >>>
> >>> Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
> >>>
> >>>  There are a few more suspicious cp instances:
> >>>
> >>> board/boundarydevices/nitrogen6x/post-build.sh
> >>> boot/grub/grub.mk
> >>> fs/iso9660/iso9660.mk
> >> I'll take a look at these, thanks.
> >>
> >>>  Also, it would be good to document somewhere (e.g. in the intro mail) why the
> >>> rsync in the toolchain is OK. Or even better, replace it even though it's not
> >>> really necessary - it just feels more consistent and safe.
> >> Yes, this sounds good. Maybe use --chmod on the toolchain and add a
> >> comment there? This way we should be covered in the future if someone
> >> uses 'rsync -a' from there.
> >
> >  Actually the toolchain already uses --chmod=Du+w.
> Yes, but that doesn't have any effect on files, so files under
> output/staging can have just about any mode, and more so when the
> toolchain is preinstalled somewhere.
>
> Changing it to --chmod=u=rwX,go=rX would eliminate that variablity
> (again, we're assuming that there are no 'random' exec bits set, but
> that's reasonable since no files are created with the exec bit set).
>
> This way, at any place within BR we could do 'rsync -a
> $(STAGING_DIR)/... $(TARGET_DIR)/...' and have well-defined modes on the
> target.
>
> That kind of rsync (or any other similar copy) don't exist as of now:
> eveything is done via 'install'. So I don't have a strong opinion for
> changing the rsync or not.

 Not true: there are plenty of instances of 'cp -a' or a variant.

 That's why I think it's safer to do the chmod for the toolchain's rsync
as well.


 Regards,
 Arnout


>
> What do you guys think?
> Thanks!
>


-- 
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: 7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [Buildroot] [PATCH v3 5/5] package: linux-fusion: use install instead of cp
  2014-11-17 17:19 ` [Buildroot] [PATCH v3 5/5] package: linux-fusion: " Guido Martínez
  2014-11-17 21:49   ` Yann E. MORIN
  2014-11-17 22:06   ` Arnout Vandecappelle
@ 2014-11-18 22:59   ` Thomas Petazzoni
  2 siblings, 0 replies; 24+ messages in thread
From: Thomas Petazzoni @ 2014-11-18 22:59 UTC (permalink / raw)
  To: buildroot

Dear Guido Mart?nez,

On Mon, 17 Nov 2014 14:19:10 -0300, Guido Mart?nez wrote:
> in order to not depend on the previous permissions of the file
> 
> Signed-off-by: Guido Mart?nez <guido@vanguardiasur.com.ar>
> ---
>  package/linux-fusion/linux-fusion.mk | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Applied to next, thanks.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

^ permalink raw reply	[flat|nested] 24+ messages in thread

end of thread, other threads:[~2014-11-18 22:59 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-11-17 17:19 [Buildroot] [PATCH v3 0/5] Target permissions Guido Martínez
2014-11-17 17:19 ` [Buildroot] [PATCH v3 1/5] Makefile: don't depend on the umask Guido Martínez
2014-11-17 21:11   ` Arnout Vandecappelle
2014-11-18 17:01     ` Guido Martínez
2014-11-18 19:22       ` Arnout Vandecappelle
2014-11-17 21:36   ` Yann E. MORIN
2014-11-17 21:41     ` Arnout Vandecappelle
2014-11-17 21:48       ` Yann E. MORIN
2014-11-17 17:19 ` [Buildroot] [PATCH v3 2/5] Makefile: don't depend on current skeleton/overlay permissions Guido Martínez
2014-11-17 21:39   ` Yann E. MORIN
2014-11-17 17:19 ` [Buildroot] [PATCH v3 3/5] pkg-generic.mk: don't depend on external package permissions Guido Martínez
2014-11-17 21:41   ` Yann E. MORIN
2014-11-18 17:02     ` Guido Martínez
2014-11-17 17:19 ` [Buildroot] [PATCH v3 4/5] package: matchbox-keyboard: use install instead of cp Guido Martínez
2014-11-17 21:44   ` Yann E. MORIN
2014-11-18 17:06     ` Guido Martínez
2014-11-17 17:19 ` [Buildroot] [PATCH v3 5/5] package: linux-fusion: " Guido Martínez
2014-11-17 21:49   ` Yann E. MORIN
2014-11-17 22:06   ` Arnout Vandecappelle
2014-11-18 17:17     ` Guido Martínez
2014-11-18 19:23       ` Arnout Vandecappelle
2014-11-18 19:40         ` Guido Martínez
2014-11-18 20:58           ` Arnout Vandecappelle
2014-11-18 22:59   ` Thomas Petazzoni

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.