* [Buildroot] [PATCH v1 1/1] package/rcw: Optional RCW Generation
2019-02-19 20:31 [Buildroot] [PATCH v1 1/1] package/rcw: Optional RCW Generation jared.bents at rockwellcollins.com
@ 2019-02-23 15:54 ` Thomas Petazzoni
0 siblings, 0 replies; 2+ messages in thread
From: Thomas Petazzoni @ 2019-02-23 15:54 UTC (permalink / raw)
To: buildroot
Hello,
On Tue, 19 Feb 2019 14:31:57 -0600
jared.bents at rockwellcollins.com wrote:
> +if BR2_PACKAGE_HOST_RCW
> +
> +config BR2_PACKAGE_HOST_RCW_CUSTOM_PATH
> +
Could you please drop this empty line ?
> + string "RCW Source file paths"
> + help
> + Path to custom RCW source files. You can provide a
> + list of rcw paths to copy and build, separated by spaces.
> + NOTE: If you want to build one common rcw file
> + (e.g. "X.rcwi") with main rcw file (e.g. "Y.rcw") then
> + include common rcw file X.rcwi in main rcw file Y.rcw
> + using "#include"
I am not sure this working is really relevant, people using this stuff
are supposed to know the syntax of .rcw files and that they can use
#include to include .rcwi files.
Instead the working should perhaps be:
Space-separated list of .rcw and .rcwi files, that will be
used to generate a RCW binary. The entire list of .rcwi files
used by the .rcw file must be specified. There must be a
single .rcw file in the list.
> diff --git a/package/rcw/rcw.mk b/package/rcw/rcw.mk
> index f4570b9bde..d33d655e6d 100644
> --- a/package/rcw/rcw.mk
> +++ b/package/rcw/rcw.mk
> @@ -9,7 +9,19 @@ RCW_SITE = https://source.codeaurora.org/external/qoriq/qoriq-components/rcw
> RCW_SITE_METHOD = git
> RCW_LICENSE = BSD-3-Clause
> RCW_LICENSE_FILES = LICENSE
> +HOST_RCW_DEPENDENCIES = host-python
Do we really need to depend on Python ? Buildroot guarantees that at
least Python 2.7 is available on the build machine, regardless of
whether host-python is built or not.
> +# The name of the file to be deliveried in the BINARIES_DIR
> +RCW_DELIVERY_FILE = PBL.bin
> +
> +# Use the host python
> +HOST_PYTHON=$(HOST_DIR)/bin/python$(PYTHON_VERSION_MAJOR)
All variables declared by a package are global, so by using
"HOST_PYTHON" as a variable name, you are "polluting" the global
namespace of variables, potentially causing a conflict. That is why we
enforce the policy that all variables defined by a package must be
prefixed by the package name.
Also, spaces around =.
> +# Get the name of the custom rcw file from the custom list
> +RCW_PROJECT=$(filter %.rcw,$(notdir $(call qstrip,$(BR2_PACKAGE_HOST_RCW_CUSTOM_PATH))))
So, this means that only a single .rcw should be in the list ? If so,
perhaps:
ifeq ($(BR2_BUILDING),y)
ifneq ($(RCW_PROJECT),)
ifneq ($(words $(RCW_PROJECT)),1)
$(error BR2_PACKAGE_HOST_RCW_CUSTOM_PATH)
endif
endif
endif
(totally untested)
> +
> +
Only one empty line here. Please run "make check-package".
> +ifeq ($(BR2_PACKAGE_HOST_RCW_CUSTOM_PATH),"")
I am not sure why the default installation becomes conditional, as you
also have to do the same logic when a RCW file is being generated.
> # Copy source files and script into $(HOST_DIR)/share/rcw/ so a developer
> # could use a post image or SDK to build/install PBL files.
> define HOST_RCW_INSTALL_CMDS
> @@ -17,4 +29,23 @@ define HOST_RCW_INSTALL_CMDS
> cp -a $(@D)/* $(HOST_DIR)/share/rcw
> endef
>
> +else
> +# Generate rcw using the specified source files from within the build directory.
> +# Copy source files and script into $(HOST_DIR)/share/rcw/ so a developer
> +# could use a SDK to rebuild/install PBL files.
> +define HOST_RCW_INSTALL_CMDS
> + mkdir -p $(@D)/custom_board/rcw
> + cp -f $(call qstrip,$(BR2_PACKAGE_HOST_RCW_CUSTOM_PATH)) $(@D)/custom_board/rcw
> + for rcwi in $(filter %.rcwi,$(notdir $(call qstrip,$(BR2_PACKAGE_HOST_RCW_CUSTOM_PATH)))); do \
> + cd $(@D)/custom_board; ln -sf rcw/$${rcwi} $${rcwi} ; \
> + done
I don't really understand this dance of symlinks. Why do you have this ?
Please use a make $(foreach ...) loop.
> + $(HOST_PYTHON) $(@D)/rcw.py -i $(@D)/custom_board/rcw/${RCW_PROJECT} -I $(@D)/custom_board/ -o $(@D)/$(RCW_DELIVERY_FILE)
Please use $(...) when referring to make variables.
> + $(INSTALL) -D -m 0644 $(@D)/$(RCW_DELIVERY_FILE) $(BINARIES_DIR)/$(RCW_DELIVERY_FILE)
> + mkdir -p $(HOST_DIR)/share/rcw
> + cp -a $(@D)/* $(HOST_DIR)/share/rcw
> + find $(HOST_DIR)/share/rcw -name "*.bin" -delete
> +endef
> +
> +endif
I'd prefer to see it implementing like this:
ifneq ($(RCW_PROJECT),)
define HOST_RCW_ADD_CUSTOM_RCW_FILES
... copying of .rcw files ...
endef
HOST_RCW_POST_PATCH_HOOKS += HOST_RCW_ADD_CUSTOM_RCW_FILES
define HOST_RCW_BUILD_CMDS
python $(@D)/rcw.py -i $(@D)/custom_board/rcw/$(RCW_PROJECT) -I $(@D)/custom_board/ -o $(@D)/$(RCW_DELIVERY_FILE)
endef
define HOST_RCW_INSTALL_DELIVERY_FILE
$(INSTALL) -D -m 0644 $(@D)/$(RCW_DELIVERY_FILE) $(BINARIES_DIR)/$(RCW_DELIVERY_FILE)
endef
endif
define HOST_RCW_INSTALL_CMDS
mkdir -p $(HOST_DIR)/share/rcw
cp -a $(@D)/* $(HOST_DIR)/share/rcw
$(HOST_RCW_INSTALL_DELIVERY_FILE)
find $(HOST_DIR)/share/rcw -name "*.bin" -delete
endef
or something along those lines.
Best regards,
Thomas Petazzoni
--
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 2+ messages in thread