| Message ID | 1416244750-24256-6-git-send-email-guido@vanguardiasur.com.ar |
|---|---|
| State | Accepted |
| Headers | show |
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 >
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)) >
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!
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
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!
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! >
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
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))
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(-)