| Message ID | 1354146890-27380-4-git-send-email-yann.morin.1998@free.fr |
|---|---|
| State | Changes Requested |
| Headers | show |
Dear Yann E. MORIN, On Thu, 29 Nov 2012 00:54:02 +0100, Yann E. MORIN wrote: > There is some (minor?) issues wrt the licensing terms. > The libfdt library is dual-licensed GPLv2+/BSD-2c, and the > executables are licensed GPLv2+. > > There is no way in BR to properly convey this information. > So I decided to add some explanatory comments in the .mk > file, in retaliation. ;-) As per the discussion during the Developers Days, there is now a way to convey this information. From the report: multiple licenses: we don't want to make things very complex. It doesn't have to be machine-readable (we don't need to support machine analysis of the license field), so we don't have to formally decide how the license text must be written. It does make sense to have some convention, though. Proposal: "GPLv2+ or BSD-2c for the library, GPLv2+ for the dtc executable". Thanks, Thomas
On 29/11/12 00:54, Yann E. MORIN wrote: > By default, we only install the libfdt library. > > As suggested by Arnout, add an option that also > installs the few dtc programs. > > Cc: Arnout Vandecappelle<arnout@mind.be> > Signed-off-by: "Yann E. MORIN"<yann.morin.1998@free.fr> [snip] > +# libfdt_install is our own install rule added by our patch > +DTC_BUILD_RULE=$(if $(BR2_PACKAGE_DTC_BINARY),,libfdt) > +DTC_INSTALL_RULE=$(if $(BR2_PACKAGE_DTC_BINARY),install,libfdt_install) > + I (and I think Peter as well) prefer the more verbose ifeq ($(BR2_PACKAGE_DTC_BINARY),y) DTC_INSTALL_RULE = install else DTC_BUILD_RULE = libfdt DTC_INSTALL_RULE = libfdt_install endif I would also call it _TARGET instead of _RULE. > define DTC_BUILD_CMDS > $(TARGET_CONFIGURE_OPTS) \ > CFLAGS="$(TARGET_CFLAGS)" \ > - $(MAKE) -C $(@D) PREFIX=/usr libfdt > + $(MAKE) -C $(@D) PREFIX=/usr $(DTC_BUILD_RULE) > endef > > -# libfdt_install is our own install rule added by our patch > +# For staging, only the library is needed > define DTC_INSTALL_STAGING_CMDS > $(MAKE) -C $(@D) DESTDIR=$(STAGING_DIR) PREFIX=/usr libfdt_install > endef > > define DTC_INSTALL_TARGET_CMDS > - $(MAKE) -C $(@D) DESTDIR=$(TARGET_DIR) PREFIX=/usr libfdt_install > + $(MAKE) -C $(@D) DESTDIR=$(TARGET_DIR) PREFIX=/usr $(DTC_INSTALL_RULE) > endef I would prefer the same rule for staging and target install: - it doesn't hurt to have the executable in staging; - it's easier to read if it's the same; - when we finally get around to creating $(make-package), the default install commands will be $(MAKE) -C $(@D) DESTDIR=... $($(PKG)_INSTALL_TARGET) (where _INSTALL_TARGET is the same for target and staging and defaults to 'install') Regards, Arnout > > define DTC_CLEAN_CMDS
Arnout, All, On Thursday 29 November 2012 Arnout Vandecappelle wrote: > I (and I think Peter as well) prefer the more verbose > > ifeq ($(BR2_PACKAGE_DTC_BINARY),y) > DTC_INSTALL_RULE = install > else > DTC_BUILD_RULE = libfdt > DTC_INSTALL_RULE = libfdt_install > endif Yes, it's easier to read. > I would also call it _TARGET instead of _RULE. Why? DTC_INSTALL_TARGET already exists, and is meant to specify if the package is to be installed in tharget. OTOH, DTC_INSTALL_RULE (or DTC_INSTALL_MAKERULE, but it's ugly) is more explicit: it is the make rule to install the package. > I would prefer the same rule for staging and target install: > - it doesn't hurt to have the executable in staging; > - it's easier to read if it's the same; Well, not too fond of it: staging serves a different purpose than target, so I prefer separating the rules. Peter: arbitration, please! ;-) > - when we finally get around to creating $(make-package), the default install > commands will be > $(MAKE) -C $(@D) DESTDIR=... $($(PKG)_INSTALL_TARGET) > (where _INSTALL_TARGET is the same for target and staging and defaults to > 'install') I was not aware of the make-package upcoming infra. However: - as already said above: $(PKG)_INSTALL_TARGET is already used. - I'd suggest using _RULE (or _MAKERULE) as the suffix - hopefully, we'll have a way to specify different rules for straging and target. Regards, Yann E. MORIN.
Thomas, All, On Thursday 29 November 2012 Thomas Petazzoni wrote: > On Thu, 29 Nov 2012 00:54:02 +0100, Yann E. MORIN wrote: > > There is some (minor?) issues wrt the licensing terms. > > The libfdt library is dual-licensed GPLv2+/BSD-2c, and the > > executables are licensed GPLv2+. > > > > There is no way in BR to properly convey this information. > > So I decided to add some explanatory comments in the .mk > > file, in retaliation. ;-) > > As per the discussion during the Developers Days, there is now a way to > convey this information. From the report: > > multiple licenses: we don't want to make things very complex. It > doesn't have to be machine-readable (we don't need to support > machine analysis of the license field), so we don't have to > formally decide how the license text must be written. It does make > sense to have some convention, though. Proposal: "GPLv2+ or BSD-2c > for the library, GPLv2+ for the dtc executable". Right-o-right, I forgot that. Thanks for reminding me. :-) Regards, Yann E. MORIN.
On 30/11/12 17:38, Yann E. MORIN wrote: > Arnout, All, > > On Thursday 29 November 2012 Arnout Vandecappelle wrote: >> I (and I think Peter as well) prefer the more verbose >> >> ifeq ($(BR2_PACKAGE_DTC_BINARY),y) >> DTC_INSTALL_RULE = install >> else >> DTC_BUILD_RULE = libfdt >> DTC_INSTALL_RULE = libfdt_install >> endif > > Yes, it's easier to read. > >> I would also call it _TARGET instead of _RULE. > > Why? Because it's not a rule. A rule is a line with a colon in the Makefile that is followed by a command. But actually target is also not correct - the GNU make manual calls it a 'goal'. (Never mind that goal and target are synonyms :-) That's why the variable is called MAKECMDGOALS. So my suggestion to use _TARGET was in fact wrong as well. > DTC_INSTALL_TARGET already exists, and is meant to specify if the > package is to be installed in tharget. Whaa, overloading! There's actually a DTC_TARGET_INSTALL_TARGET as well, which contains the name of the make goal to install to the target directory... Given that in buildroot the make goals are called targets all the time, calling it _GOAL here would solve the duplication but reduces consistency. Unless we change all the current uses of 'target' to 'goal' (which would resolve some confusion with the target directory, so that would be a good thing...). I realize that this is nitpicking, but if we choose a name now we'll probably be stuck with it for years, so we need to have this discussion at some point. > > OTOH, DTC_INSTALL_RULE (or DTC_INSTALL_MAKERULE, but it's ugly) is more > explicit: it is the make rule to install the packag [snip] Regards, Arnout
Arnout, All, On Tuesday 04 December 2012 Arnout Vandecappelle wrote: > On 30/11/12 17:38, Yann E. MORIN wrote: > > On Thursday 29 November 2012 Arnout Vandecappelle wrote: > >> I would also call it _TARGET instead of _RULE. > > Why? > Because it's not a rule. A rule is a line with a colon in the Makefile > that is followed by a command. Right. > But actually target is also not correct - the GNU make manual calls it > a 'goal'. (Never mind that goal and target are synonyms :-) That's why > the variable is called MAKECMDGOALS. Right again. [--SNIP--] > Given that in buildroot the make goals are called targets all the time, > calling it _GOAL here would solve the duplication but reduces consistency. > Unless we change all the current uses of 'target' to 'goal' (which would > resolve some confusion with the target directory, so that would be a good > thing...). > > I realize that this is nitpicking, but if we choose a name now we'll > probably be stuck with it for years, so we need to have this discussion > at some point. OK, so let's first list what variables we need. I can think of at least these (prefix by "$(PKG)_", of coure): Variable | Default | Notes --------------------+-----------+---------------------------------- MAKE_CONFIG_GOAL | (empty) | Not used if empty (or not set?) MAKE_BUILD_GOAL | (empty) | Will call the default target of the Makefile MAKE_INSTALL_GOAL | install | MAKE_UNINSTALL_GOAL | uninstall | MAKE_CLEAN_GOAL | clean | I think the names are explicit enough, save for the first one. Some packages may require a step before the acutal build: for the Linux kernel, we need to make oldconfig first. Now, let's settle for a naming convention. See my proposal above. Anyone wants to suggest another naming scheme? Note however that I really want to get rid of this qemu series of mine (ie. have it applied upstream asap). ;-) I'll rework the series to move this to the end, so we can have more time to think about the naming scheme. Regards, Yann E. MORIN.
On 04/12/12 23:10, Yann E. MORIN wrote: > OK, so let's first list what variables we need. I can think of at least > these (prefix by "$(PKG)_", of coure): > > Variable | Default | Notes > --------------------+-----------+---------------------------------- > MAKE_CONFIG_GOAL | (empty) | Not used if empty (or not set?) > MAKE_BUILD_GOAL | (empty) | Will call the default target of the Makefile > MAKE_INSTALL_GOAL | install | > MAKE_UNINSTALL_GOAL | uninstall | > MAKE_CLEAN_GOAL | clean | > > I think the names are explicit enough, save for the first one. Some packages > may require a step before the acutal build: for the Linux kernel, we need > to make oldconfig first. > > Now, let's settle for a naming convention. See my proposal above. > Anyone wants to suggest another naming scheme? Your proposal looks good to me! Regards, Arnout
diff --git a/package/dtc/Config.in b/package/dtc/Config.in index 96225e3..04edab2 100644 --- a/package/dtc/Config.in +++ b/package/dtc/Config.in @@ -1,9 +1,25 @@ config BR2_PACKAGE_DTC - bool "dtc" + bool "dtc (libfdt)" help The Device Tree Compiler, dtc, takes as input a device-tree in a given format and outputs a device-tree in another format. - Note that only the library is installed for now. + Note that only the library is installed. + If you want the programs, say 'y' here, and to "dtc programs", below. http://git.jdl.com/gitweb/?p=dtc.git (no home page) + +if BR2_PACKAGE_DTC + +config BR2_PACKAGE_DTC_BINARY + bool "dtc programs" + help + Say 'y' here if you also want the programs on the target: + - convert-dtsv0 convert from version 0 to version 1 + - dtc the device tree compiler + - dtdiff compare two device trees (needs bash) + - fdtdump print a readable version of a flat device tree + - fdtget read values from device tree + - fdtput write a property value to a device tree + +endif diff --git a/package/dtc/dtc.mk b/package/dtc/dtc.mk index a0e1e35..1ffd341 100644 --- a/package/dtc/dtc.mk +++ b/package/dtc/dtc.mk @@ -6,25 +6,29 @@ DTC_VERSION = e4b497f367a3b2ae99cc52089a14a221b13a76ef DTC_SITE = git://git.jdl.com/software/dtc.git -DTC_LICENSE = GPLv2+/BSD-2c +DTC_LICENSE = GPLv2+/BSD-2c GPLv2+ DTC_LICENSE_FILES = README.license GPL # Note: the dual-license only applies to the library. -# The DT compiler (dtc) is GPLv2+, but we do not install it. +# The DT compiler (dtc) is GPLv2+. DTC_INSTALL_STAGING = YES +# libfdt_install is our own install rule added by our patch +DTC_BUILD_RULE=$(if $(BR2_PACKAGE_DTC_BINARY),,libfdt) +DTC_INSTALL_RULE=$(if $(BR2_PACKAGE_DTC_BINARY),install,libfdt_install) + define DTC_BUILD_CMDS $(TARGET_CONFIGURE_OPTS) \ CFLAGS="$(TARGET_CFLAGS)" \ - $(MAKE) -C $(@D) PREFIX=/usr libfdt + $(MAKE) -C $(@D) PREFIX=/usr $(DTC_BUILD_RULE) endef -# libfdt_install is our own install rule added by our patch +# For staging, only the library is needed define DTC_INSTALL_STAGING_CMDS $(MAKE) -C $(@D) DESTDIR=$(STAGING_DIR) PREFIX=/usr libfdt_install endef define DTC_INSTALL_TARGET_CMDS - $(MAKE) -C $(@D) DESTDIR=$(TARGET_DIR) PREFIX=/usr libfdt_install + $(MAKE) -C $(@D) DESTDIR=$(TARGET_DIR) PREFIX=/usr $(DTC_INSTALL_RULE) endef define DTC_CLEAN_CMDS
By default, we only install the libfdt library. As suggested by Arnout, add an option that also installs the few dtc programs. Cc: Arnout Vandecappelle <arnout@mind.be> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr> --- There is some (minor?) issues wrt the licensing terms. The libfdt library is dual-licensed GPLv2+/BSD-2c, and the executables are licensed GPLv2+. There is no way in BR to properly convey this information. So I decided to add some explanatory comments in the .mk file, in retaliation. ;-) --- package/dtc/Config.in | 20 ++++++++++++++++++-- package/dtc/dtc.mk | 14 +++++++++----- 2 files changed, 27 insertions(+), 7 deletions(-)