Target permissions
v3 cover Target permissions
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(-)
Signature · 1 line
2.1.3
PATCH v3 1/5 Makefile: don't depend on the umask
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.
--- Makefile | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
Makefile +15 -0
@@ -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
Signature · 1 line
2.1.3
PATCH v3 1/5 Makefile: don't depend on the umask
Quoted 17 lines · Guido Mart?nez
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
Makefilewhich 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 <[email protected]>--- Makefile | 15 +++++++++++++++ 1 file changed, 15 insertions(+)Makefile +0
-0
Makefile line 30
Quoted 7 lines
Makefile +4
-0@@ -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
Quoted 19 lines
Makefile +11
-0+ +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
Signature · 6 lines
Arnout Vandecappelle arnout at mind be Senior Embedded Software Architect +32-16-286500 Essensium/Mind http://www.mind.be G.Geenslaan 9, 3001 Leuven, Belgium BE 872 984 063 RPR Leuven LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle GPG fingerprint: 7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F
PATCH v3 1/5 Makefile: don't depend on the umask
Hi Arnout, all,
Quoted 29 lines · Arnout Vandecappelle
Quoted 17 lines · Guido Mart?nez
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
Makefilewhich 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 <[email protected]>--- Makefile | 15 +++++++++++++++ 1 file changed, 15 insertions(+)Makefile +0
-0Makefile line 30
Quoted 7 lines
Makefile +4
-0@@ -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:
Quoted 3 lines
.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:
"""
Makefile +15 -0
@@ -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?
Quoted 4 lines
Otherwise, looks good.
BTW wouldn't it be nice to add the umask setting to the generated
Makefilein
the $(O) directory, to avoid another level of make?
Sure, will do.
Thanks!
Signature · 2 lines
Guido Mart?nez, VanguardiaSur www.vanguardiasur.com.ar
PATCH v3 1/5 Makefile: don't depend on the umask
Quoted 76 lines · Guido Mart?nez
Hi Arnout, all,
Quoted 29 lines · Arnout Vandecappelle
Quoted 17 lines · Guido Mart?nez
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
Makefilewhich 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 <[email protected]>--- Makefile | 15 +++++++++++++++ 1 file changed, 15 insertions(+)Makefile +0
-0Makefile line 30
Quoted 7 lines
Makefile +4
-0@@ -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:Quoted 3 lines
.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:
"""
Makefile +15
-0@@ -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"""
Quoted 1 line
Does that sound good?
Yes, sounds good.
Regards,
Arnout
Quoted 9 lines
Quoted 5 lines
Otherwise, looks good.
BTW wouldn't it be nice to add the umask setting to the generated
Makefilein
the $(O) directory, to avoid another level of make?Sure, will do.
Thanks!
Signature · 6 lines
Arnout Vandecappelle arnout at mind be Senior Embedded Software Architect +32-16-286500 Essensium/Mind http://www.mind.be G.Geenslaan 9, 3001 Leuven, Belgium BE 872 984 063 RPR Leuven LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle GPG fingerprint: 7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F
PATCH v3 1/5 Makefile: don't depend on the umask
In reply to Guido MartínezGuido, All,
On 2014-11-17 14:19 -0300, Guido Mart?nez spake thusly:
Quoted 17 lines
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
Makefilewhich 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 <[email protected]>--- Makefile | 15 +++++++++++++++ 1 file changed, 15 insertions(+)Makefile +0
-0
Makefile line 33
Quoted 10 lines
Makefile +7
-0@@ -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.
Makefile line 36
Quoted 3 lines
Makefile +3
-0+ +%: + @umask $(UMASK) && make --no-print-directory $@
Ditto.
Regards,
Yann E. MORIN.
Quoted 14 lines
Makefile +4
-0+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 #umaskSignature · 1 line
2.1.3
Signature · 6 lines
.-----------------.--------------------.------------------.--------------------. | Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: | | +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ | | +33 223 225 172 `------------.-------: X AGAINST | \e/ There is no | | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. | '------------------------------^-------^------------------^--------------------'
PATCH v3 1/5 Makefile: don't depend on the umask
Quoted 33 lines · Yann E. MORIN
Guido, All,
On 2014-11-17 14:19 -0300, Guido Mart?nez spake thusly:
Quoted 17 lines
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
Makefilewhich 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 <[email protected]>--- Makefile | 15 +++++++++++++++ 1 file changed, 15 insertions(+)Makefile +0
-0Makefile line 33
Quoted 10 lines
Makefile +7
-0@@ -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-directoryYou should use $(MAKE) instead of just 'make'.
Quoted 4 lines
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
Quoted 29 lines
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.Makefile line 36
Quoted 3 lines
Makefile +3
-0+ +%: + @umask $(UMASK) && make --no-print-directory $@Ditto.
Regards,
Yann E. MORIN.Quoted 14 lines
Makefile +4
-0+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 #umaskSignature · 1 line
2.1.3
Signature · 6 lines
Arnout Vandecappelle arnout at mind be Senior Embedded Software Architect +32-16-286500 Essensium/Mind http://www.mind.be G.Geenslaan 9, 3001 Leuven, Belgium BE 872 984 063 RPR Leuven LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle GPG fingerprint: 7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F
PATCH v3 1/5 Makefile: don't depend on the umask
Arnout, All,
On 2014-11-17 22:41 +0100, Arnout Vandecappelle spake thusly:
Quoted 2 lines · Yann E. MORIN
Quoted 1 line
On 2014-11-17 14:19 -0300, Guido Mart?nez spake thusly:
[--SNIP--]
Quoted 2 lines
Quoted 2 lines
Makefile line 33
Quoted 2 lines
Makefile +2
-0+all: + @umask $(UMASK) && make --no-print-directory
[--SNIP--]
Quoted 6 lines
Quoted 4 lines
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
Makefilein $(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.
Signature · 6 lines
.-----------------.--------------------.------------------.--------------------. | Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: | | +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ | | +33 223 225 172 `------------.-------: X AGAINST | \e/ There is no | | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. | '------------------------------^-------^------------------^--------------------'
PATCH v3 2/5 Makefile: don't depend on current skeleton/overlay permissions
In reply to Guido MartínezWe 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.
--- Makefile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Makefile +2 -2
@@ -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)), \
Signature · 1 line
2.1.3
PATCH v3 2/5 Makefile: don't depend on current skeleton/overlay permissions
Guido, All,
On 2014-11-17 14:19 -0300, Guido Mart?nez spake thusly:
Quoted 11 lines
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 <[email protected]>
Regards,
Yann E. MORIN.
Quoted 29 lines
--- Makefile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)Makefile +2
-2@@ -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)), \Signature · 1 line
2.1.3
Signature · 6 lines
.-----------------.--------------------.------------------.--------------------. | Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: | | +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ | | +33 223 225 172 `------------.-------: X AGAINST | \e/ There is no | | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. | '------------------------------^-------^------------------^--------------------'
PATCH v3 3/5 pkg-generic.mk: don't depend on external package permissions
In reply to Guido MartínezReset 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.
--- package/pkg-generic.mk | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
package/pkg-generic.mk +1 -1
@@ -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 $@
Signature · 1 line
2.1.3
PATCH v3 3/5 pkg-generic.mk: don't depend on external package permissions
Guido, All,
On 2014-11-17 14:19 -0300, Guido Mart?nez spake thusly:
Quoted 3 lines
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...
Quoted 5 lines
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 <[email protected]>
Regards,
Yann E. MORIN.
Quoted 20 lines
--- package/pkg-generic.mk | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)package/pkg-generic.mk +1
-1@@ -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 $@Signature · 1 line
2.1.3
Signature · 6 lines
.-----------------.--------------------.------------------.--------------------. | Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: | | +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ | | +33 223 225 172 `------------.-------: X AGAINST | \e/ There is no | | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. | '------------------------------^-------^------------------^--------------------'
PATCH v3 3/5 pkg-generic.mk: don't depend on external package permissions
Quoted 8 lines · Yann E. MORIN
Guido, All,
On 2014-11-17 14:19 -0300, Guido Mart?nez spake thusly:
Quoted 3 lines
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!
Signature · 2 lines
Guido Mart?nez, VanguardiaSur www.vanguardiasur.com.ar
PATCH v3 4/5 package: matchbox-keyboard: use install instead of cp
In reply to Guido Martínezin order to not depend on the previous permissions of the file
--- package/matchbox/matchbox-keyboard/matchbox-keyboard.mk | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
package/matchbox/matchbox-keyboard/matchbox-keyboard.mk +1 -1
@@ -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
Signature · 1 line
2.1.3
PATCH v3 4/5 package: matchbox-keyboard: use install instead of cp
Guido, All,
On 2014-11-17 14:19 -0300, Guido Mart?nez spake thusly:
Quoted 12 lines
in order to not depend on the previous permissions of the file
Signed-off-by Guido Mart?nez <[email protected]>--- package/matchbox/matchbox-keyboard/matchbox-keyboard.mk | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)package/matchbox/matchbox-keyboard/matchbox-keyboard.mk +0
-0
package/matchbox/matchbox-keyboard/matchbox-keyboard.mk MATCHBOX_KEYBOARD_DEPENDENCIES = host-pkgconf matchbox-lib matchbox-fakekey expa · line 18
Quoted 5 lines
package/matchbox/matchbox-keyboard/matchbox-keyboard.mk +1
-1@@ -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.shThat 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.
Quoted 6 lines
package/matchbox/matchbox-keyboard/matchbox-keyboard.mk +0
-0endef MATCHBOX_KEYBOARD_POST_INSTALL_TARGET_HOOKS += MATCHBOX_KEYBOARD_POST_INSTALL_FIXESSignature · 1 line
2.1.3
Signature · 6 lines
.-----------------.--------------------.------------------.--------------------. | Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: | | +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ | | +33 223 225 172 `------------.-------: X AGAINST | \e/ There is no | | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. | '------------------------------^-------^------------------^--------------------'
PATCH v3 4/5 package: matchbox-keyboard: use install instead of cp
Hi Yann, all,
Quoted 26 lines · Yann E. MORIN
Guido, All,
On 2014-11-17 14:19 -0300, Guido Mart?nez spake thusly:
Quoted 12 lines
in order to not depend on the previous permissions of the file
Signed-off-by Guido Mart?nez <[email protected]>--- package/matchbox/matchbox-keyboard/matchbox-keyboard.mk | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)package/matchbox/matchbox-keyboard/matchbox-keyboard.mk +0
-0package/matchbox/matchbox-keyboard/matchbox-keyboard.mk MATCHBOX_KEYBOARD_DEPENDENCIES = host-pkgconf matchbox-lib matchbox-fakekey expa · line 18
Quoted 5 lines
package/matchbox/matchbox-keyboard/matchbox-keyboard.mk +1
-1@@ -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
Quoted 6 lines
That is:
- full path to the destination file. Otherwise, if $(TARGET_DIR)/usr/bindoes 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!
Signature · 2 lines
Guido Mart?nez, VanguardiaSur www.vanguardiasur.com.ar
PATCH v3 5/5 package: linux-fusion: use install instead of cp
In reply to Guido Martínezin order to not depend on the previous permissions of the file
--- package/linux-fusion/linux-fusion.mk | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
package/linux-fusion/linux-fusion.mk +2 -2
@@ -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))
Signature · 1 line
2.1.3
PATCH v3 5/5 package: linux-fusion: use install instead of cp
Guido, All,
On 2014-11-17 14:19 -0300, Guido Mart?nez spake thusly:
Quoted 3 lines
in order to not depend on the previous permissions of the file
Signed-off-by Guido Mart?nez <[email protected]>
But see a comment below...
Quoted 9 lines
--- package/linux-fusion/linux-fusion.mk | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)package/linux-fusion/linux-fusion.mk +0
-0
package/linux-fusion/linux-fusion.mk define LINUX_FUSION_INSTALL_TARGET_CMDS · line 40
Quoted 7 lines
package/linux-fusion/linux-fusion.mk +2
-2@@ -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.
Quoted 6 lines
package/linux-fusion/linux-fusion.mk +0
-0endef $(eval $(generic-package))Signature · 1 line
2.1.3
Signature · 6 lines
.-----------------.--------------------.------------------.--------------------. | Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: | | +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ | | +33 223 225 172 `------------.-------: X AGAINST | \e/ There is no | | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. | '------------------------------^-------^------------------^--------------------'
PATCH v3 5/5 package: linux-fusion: use install instead of cp
In reply to Guido MartínezQuoted 3 lines · Guido Mart?nez
in order to not depend on the previous permissions of the file
Signed-off-by Guido Mart?nez <[email protected]>
There are a few more suspicious cp instances:
board/boundarydevices/nitrogen6x/post-build.shboot/grub/grub.mkfs/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
Quoted 20 lines
--- package/linux-fusion/linux-fusion.mk | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)package/linux-fusion/linux-fusion.mk +2
-2@@ -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))
Signature · 6 lines
Arnout Vandecappelle arnout at mind be Senior Embedded Software Architect +32-16-286500 Essensium/Mind http://www.mind.be G.Geenslaan 9, 3001 Leuven, Belgium BE 872 984 063 RPR Leuven LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle GPG fingerprint: 7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F
PATCH v3 5/5 package: linux-fusion: use install instead of cp
Hi Arnout, all
Quoted 5 lines · Arnout Vandecappelle
Quoted 3 lines · Guido Mart?nez
in order to not depend on the previous permissions of the file
Signed-off-by Guido Mart?nez <[email protected]>
Quoted 7 lines
There are a few more suspicious cp instances:
board/boundarydevices/nitrogen6x/post-build.shboot/grub/grub.mkfs/iso9660/iso9660.mk
I'll take a look at these, thanks.
Quoted 3 lines
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!
Signature · 2 lines
Guido Mart?nez, VanguardiaSur www.vanguardiasur.com.ar
PATCH v3 5/5 package: linux-fusion: use install instead of cp
Quoted 15 lines · Guido Mart?nez
Hi Arnout, all
Quoted 5 lines · Arnout Vandecappelle
Quoted 3 lines · Guido Mart?nez
in order to not depend on the previous permissions of the file
Signed-off-by Guido Mart?nez <[email protected]>Quoted 7 lines
There are a few more suspicious cp instances:
board/boundarydevices/nitrogen6x/post-build.shboot/grub/grub.mkfs/iso9660/iso9660.mk
Quoted 8 lines
I'll take a look at these, thanks.
Quoted 3 lines
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
Signature · 6 lines
Arnout Vandecappelle arnout at mind be Senior Embedded Software Architect +32-16-286500 Essensium/Mind http://www.mind.be G.Geenslaan 9, 3001 Leuven, Belgium BE 872 984 063 RPR Leuven LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle GPG fingerprint: 7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F
PATCH v3 5/5 package: linux-fusion: use install instead of cp
Quoted 18 lines · Arnout Vandecappelle
Quoted 17 lines · Guido Mart?nez
Hi Arnout, all
Quoted 5 lines · Arnout Vandecappelle
Quoted 3 lines · Guido Mart?nez
in order to not depend on the previous permissions of the file
Signed-off-by Guido Mart?nez <[email protected]>Quoted 7 lines
There are a few more suspicious cp instances:
board/boundarydevices/nitrogen6x/post-build.shboot/grub/grub.mkfs/iso9660/iso9660.mkI'll take a look at these, thanks.
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
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!
Signature · 2 lines
Guido Mart?nez, VanguardiaSur www.vanguardiasur.com.ar
PATCH v3 5/5 package: linux-fusion: use install instead of cp
Quoted 35 lines · Guido Mart?nez
Quoted 18 lines · Arnout Vandecappelle
Quoted 17 lines · Guido Mart?nez
Hi Arnout, all
Quoted 5 lines · Arnout Vandecappelle
Quoted 3 lines · Guido Mart?nez
in order to not depend on the previous permissions of the file
Signed-off-by Guido Mart?nez <[email protected]>Quoted 7 lines
There are a few more suspicious cp instances:
board/boundarydevices/nitrogen6x/post-build.shboot/grub/grub.mkfs/iso9660/iso9660.mkI'll take a look at these, thanks.
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
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).
Quoted 7 lines
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
Quoted 4 lines
What do you guys think?
Thanks!
Signature · 6 lines
Arnout Vandecappelle arnout at mind be Senior Embedded Software Architect +32-16-286500 Essensium/Mind http://www.mind.be G.Geenslaan 9, 3001 Leuven, Belgium BE 872 984 063 RPR Leuven LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle GPG fingerprint: 7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F
Thomas Petazzoni
AppliedPATCH v3 5/5 package: linux-fusion: use install instead of cp
In reply to Guido MartínezDear Guido Mart?nez,
Quoted 6 lines · Guido Mart?nez
in order to not depend on the previous permissions of the file
Signed-off-by Guido Mart?nez <[email protected]>--- package/linux-fusion/linux-fusion.mk | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Applied to next, thanks.
Thomas
Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com