| Message ID | 1413580605-57289-1-git-send-email-kaszak@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
Hi Karoly, all On Fri, Oct 17, 2014 at 11:16 PM, Karoly Kasza <kaszak@gmail.com> wrote: > The "iso image" rootfs target is a special one, as it does not use the > ROOTFS_TARGET infrastructure. The absence of ROOTFS_*_DEPENDENCIES > variable makes "make source" to skip this target's dependencies > (namely host-cdrkit and it's children) obstructing an offline build. > > Signed-off-by: Karoly Kasza <kaszak@gmail.com> > --- > fs/iso9660/iso9660.mk | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/fs/iso9660/iso9660.mk b/fs/iso9660/iso9660.mk > index b0e755d..5137026 100644 > --- a/fs/iso9660/iso9660.mk > +++ b/fs/iso9660/iso9660.mk > @@ -10,6 +10,8 @@ > ISO9660_TARGET_DIR = $(BUILD_DIR)/iso9660 > ISO9660_BOOT_MENU := $(call qstrip,$(BR2_TARGET_ROOTFS_ISO9660_BOOT_MENU)) > > +ROOTFS_ISO9660_DEPENDENCIES = host-cdrkit host-fakeroot linux rootfs-cpio grub > + > $(BINARIES_DIR)/rootfs.iso9660: host-cdrkit host-fakeroot linux rootfs-cpio grub > @$(call MESSAGE,"Generating root filesystem image rootfs.iso9660") > mkdir -p $(ISO9660_TARGET_DIR) > -- > 1.7.10.4 How about putting the dependencies variables on the top ? Also, please keep the dependencies alphabetically sorted. I know they are not ordered the line below, but it would be nice. > > _______________________________________________ > buildroot mailing list > buildroot@busybox.net > http://lists.busybox.net/mailman/listinfo/buildroot
On 18/10/14 12:31, Maxime Hadjinlian wrote: > Hi Karoly, all > > On Fri, Oct 17, 2014 at 11:16 PM, Karoly Kasza <kaszak@gmail.com> wrote: >> The "iso image" rootfs target is a special one, as it does not use the >> ROOTFS_TARGET infrastructure. The absence of ROOTFS_*_DEPENDENCIES >> variable makes "make source" to skip this target's dependencies >> (namely host-cdrkit and it's children) obstructing an offline build. Good catch! >> >> Signed-off-by: Karoly Kasza <kaszak@gmail.com> >> --- >> fs/iso9660/iso9660.mk | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/fs/iso9660/iso9660.mk b/fs/iso9660/iso9660.mk >> index b0e755d..5137026 100644 >> --- a/fs/iso9660/iso9660.mk >> +++ b/fs/iso9660/iso9660.mk >> @@ -10,6 +10,8 @@ >> ISO9660_TARGET_DIR = $(BUILD_DIR)/iso9660 >> ISO9660_BOOT_MENU := $(call qstrip,$(BR2_TARGET_ROOTFS_ISO9660_BOOT_MENU)) >> >> +ROOTFS_ISO9660_DEPENDENCIES = host-cdrkit host-fakeroot linux rootfs-cpio grub >> + >> $(BINARIES_DIR)/rootfs.iso9660: host-cdrkit host-fakeroot linux rootfs-cpio grub You should use $(ROOTFS_ISO9660_DEPENDENCIES) here. >> @$(call MESSAGE,"Generating root filesystem image rootfs.iso9660") >> mkdir -p $(ISO9660_TARGET_DIR) >> -- >> 1.7.10.4 > How about putting the dependencies variables on the top ? > Also, please keep the dependencies alphabetically sorted. > I know they are not ordered the line below, but it would be nice. Well, now they're ordered according to how they appear in the makefiles, i.e. first the real packages, then the kernel, then the rootfs, then the bootloader. Hm, actually, the bootloader should come before the kernel in that reasoning... So alphabetically is probably better :-) Regards, Arnout >> >> _______________________________________________ >> buildroot mailing list >> buildroot@busybox.net >> http://lists.busybox.net/mailman/listinfo/buildroot > _______________________________________________ > buildroot mailing list > buildroot@busybox.net > http://lists.busybox.net/mailman/listinfo/buildroot >
Hi, >> > >> +ROOTFS_ISO9660_DEPENDENCIES = host-cdrkit host-fakeroot linux > rootfs-cpio grub > >> + > >> $(BINARIES_DIR)/rootfs.iso9660: host-cdrkit host-fakeroot linux > rootfs-cpio grub > > You should use $(ROOTFS_ISO9660_DEPENDENCIES) here. > > Why? Non of the other rootfs target makefiles use that (fs/*/*.mk). > > How about putting the dependencies variables on the top ? > > Also, please keep the dependencies alphabetically sorted. > > I know they are not ordered the line below, but it would be nice. > > Well, now they're ordered according to how they appear in the makefiles, > i.e. > first the real packages, then the kernel, then the rootfs, then the > bootloader. > > Hm, actually, the bootloader should come before the kernel in that > reasoning... > > So alphabetically is probably better :-) > Technically, the dependencies variable is already at the top, before the first make target. Also, it's not on the exact top of the file in any other rootfs target makefiles. Regarding the order: I don't really think that matters. Regards, Karoly
Dear Károly Kasza, On Sat, 18 Oct 2014 16:17:46 +0200, Károly Kasza wrote: > > You should use $(ROOTFS_ISO9660_DEPENDENCIES) here. > > > Why? Non of the other rootfs target makefiles use that (fs/*/*.mk). All the other rootfs target makefiles use <foo>_DEPENDENCIES. Except that it's taken into account directly by the common filesystem logic in fs/common.mk. However, iso9660 is special, and therefore we handle the dependencies manually. But still, there's no need to repeat them twice. So, in your patch, change: ROOTFS_ISO9660_DEPENDENCIES = host-cdrkit host-fakeroot linux rootfs-cpio grub $(BINARIES_DIR)/rootfs.iso9660: host-cdrkit host-fakeroot linux rootfs-cpio grub To: ROOTFS_ISO9660_DEPENDENCIES = host-cdrkit host-fakeroot linux rootfs-cpio grub $(BINARIES_DIR)/rootfs.iso9660: $(ROOTFS_ISO9660_DEPENDENCIES) > Technically, the dependencies variable is already at the top, before the > first make target. Agreed. > Regarding the order: I don't really think that matters. Well, it's indeed just a subjective thing, like most coding style things. Still, when a core Buildroot contributor such as Arnout suggests coding style changes, it's usually a good idea to follow suit to get the patch merged quickly :) That being said, I agree that we don't enforce any particular order for the dependencies in the other packages. Best regards, Thomas
> > > > > You should use $(ROOTFS_ISO9660_DEPENDENCIES) here. > > > > > Why? Non of the other rootfs target makefiles use that (fs/*/*.mk). > > All the other rootfs target makefiles use <foo>_DEPENDENCIES. Except > that it's taken into account directly by the common filesystem logic in > fs/common.mk. However, iso9660 is special, and therefore we handle the > dependencies manually. But still, there's no need to repeat them twice. > So, in your patch, change: > > ROOTFS_ISO9660_DEPENDENCIES = host-cdrkit host-fakeroot linux rootfs-cpio > grub > > $(BINARIES_DIR)/rootfs.iso9660: host-cdrkit host-fakeroot linux > rootfs-cpio grub > > To: > > ROOTFS_ISO9660_DEPENDENCIES = host-cdrkit host-fakeroot linux rootfs-cpio > grub > > $(BINARIES_DIR)/rootfs.iso9660: $(ROOTFS_ISO9660_DEPENDENCIES) > > Oh, that's correct. I misunderstood the first proposal of Arnout. > Regarding the order: I don't really think that matters. > > Well, it's indeed just a subjective thing, like most coding style > things. Still, when a core Buildroot contributor such as Arnout > suggests coding style changes, it's usually a good idea to follow suit > to get the patch merged quickly :) > > That being said, I agree that we don't enforce any particular order for > the dependencies in the other packages. > OK, but this means the changing of the order of dependencies is not at all connected with the original purpose of this patch, also everywhere else it should be changed I guess. I'll send a V2. Best regards, Karoly
Dear Károly Kasza, On Sat, 18 Oct 2014 19:13:11 +0200, Károly Kasza wrote: > OK, but this means the changing of the order of dependencies is not at all > connected with the original purpose of this patch, also everywhere > else it should be changed I guess. I agree the order of the dependencies is a separate thing. And it's also a bit strange to enforce an ordering for this case, while we do not have such a rule for all other packages. Thomas
diff --git a/fs/iso9660/iso9660.mk b/fs/iso9660/iso9660.mk index b0e755d..5137026 100644 --- a/fs/iso9660/iso9660.mk +++ b/fs/iso9660/iso9660.mk @@ -10,6 +10,8 @@ ISO9660_TARGET_DIR = $(BUILD_DIR)/iso9660 ISO9660_BOOT_MENU := $(call qstrip,$(BR2_TARGET_ROOTFS_ISO9660_BOOT_MENU)) +ROOTFS_ISO9660_DEPENDENCIES = host-cdrkit host-fakeroot linux rootfs-cpio grub + $(BINARIES_DIR)/rootfs.iso9660: host-cdrkit host-fakeroot linux rootfs-cpio grub @$(call MESSAGE,"Generating root filesystem image rootfs.iso9660") mkdir -p $(ISO9660_TARGET_DIR)
The "iso image" rootfs target is a special one, as it does not use the ROOTFS_TARGET infrastructure. The absence of ROOTFS_*_DEPENDENCIES variable makes "make source" to skip this target's dependencies (namely host-cdrkit and it's children) obstructing an offline build. Signed-off-by: Karoly Kasza <kaszak@gmail.com> --- fs/iso9660/iso9660.mk | 2 ++ 1 file changed, 2 insertions(+)