Patchwork [03/51] package/dtc: add option to install programs

login
register
mail settings
Submitter Yann E. MORIN
Date Nov. 28, 2012, 11:54 p.m.
Message ID <1354146890-27380-4-git-send-email-yann.morin.1998@free.fr>
Download mbox | patch
Permalink /patch/202576/
State Changes Requested
Headers show

Comments

Yann E. MORIN - Nov. 28, 2012, 11:54 p.m.
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(-)
Thomas Petazzoni - Nov. 29, 2012, 8:40 a.m.
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
Arnout Vandecappelle - Nov. 29, 2012, 3 p.m.
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
Yann E. MORIN - Nov. 30, 2012, 4:38 p.m.
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.
Yann E. MORIN - Dec. 3, 2012, 6:10 p.m.
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.
Arnout Vandecappelle - Dec. 4, 2012, 9:28 p.m.
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
Yann E. MORIN - Dec. 4, 2012, 10:10 p.m.
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.
Arnout Vandecappelle - Dec. 5, 2012, 6:30 a.m.
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

Patch

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