[ptxdist] [PATCH 0/4] ptxd_make_world_inject: Spring cleanup and optional dest dir

[ptxdist] [PATCH 0/4] ptxd_make_world_inject: Spring cleanup and optional dest dir

[Alexander Dahl]

Hello,

some spring cleaning and a new feature for the firmware inject
mechanism.

When trying to re-build U-Boot as an oot build for the Karo QSBASE93
Evalkit (for QS93 Solder-Down System-on-Module) it became apparent
U-Boot wants some of those binary blobs in build folder instead of
source folder.  Supporting this usecase is nice to have, because you
won't clutter your source tree, especially when building from external
tree after something like `ptxdist local-src u-boot ~/src/u-boot` …

Maybe we should add some documentation for this inject mechanism?

Follow-up patches for u-boot package are not yet ready for submission.
Let me know what you thing of this first.

Greets
Alex

Alexander Dahl (4):
  ptxd_make_world_inject: Remove useless test
  ptxd_make_world_inject: Use <PKG>_DIR directly
  ptxd_make_world_inject: Escape inject path and files
  ptxd_make_world_inject: Introduce separate destination directory

 rules/post/ptxd_make_world_inject.make |  6 +++---
 scripts/lib/ptxd_make_world_inject.sh  | 25 ++++++++++++++++++++-----
 2 files changed, 23 insertions(+), 8 deletions(-)


base-commit: 549a207826f541515b26c8f8bdcc1b8df23fe3c5
-- 
2.39.2

[ptxdist] [PATCH 1/4] ptxd_make_world_inject: Remove useless test

[Alexander Dahl]

One line above $target is set to "${var}/$(subshell)" and even if $var
_and_ the result of the subshell call is empty, there's still the slash.
Thus target is never empty.

Signed-off-by: Alexander Dahl <ada@thorsis.com>
---
 scripts/lib/ptxd_make_world_inject.sh | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/scripts/lib/ptxd_make_world_inject.sh b/scripts/lib/ptxd_make_world_inject.sh
index fe4eb8363..5c2d0dc5f 100644
--- a/scripts/lib/ptxd_make_world_inject.sh
+++ b/scripts/lib/ptxd_make_world_inject.sh
@@ -11,9 +11,6 @@ ptxd_make_inject() {
 
     source="$(echo ${inject_file} | cut -d ":" -f 1)"
     target="${pkg_source}/$(echo ${inject_file} | cut -d ":" -f 2)"
-    if [ -z "${target}" ]; then
-	target="${source}"
-    fi
 
     if [[ "${source}" =~ ^/.* ]]; then
 	ptxd_bailout "'${source}' must not be an absolute path!" \
-- 
2.39.2

[ptxdist] [PATCH 2/4] ptxd_make_world_inject: Use <PKG>_DIR directly

[Alexander Dahl]

pkg_source was defined as "$($(1)_DIR)" which is the same as pkg_dir in
ptxd_make_world_common.  We can use pkg_dir directly.  Add a safe-guard
to bail out early if that var is empty.

Signed-off-by: Alexander Dahl <ada@thorsis.com>
---
 rules/post/ptxd_make_world_inject.make | 3 +--
 scripts/lib/ptxd_make_world_inject.sh  | 6 +++++-
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/rules/post/ptxd_make_world_inject.make b/rules/post/ptxd_make_world_inject.make
index b7d28e92f..3506ee114 100644
--- a/rules/post/ptxd_make_world_inject.make
+++ b/rules/post/ptxd_make_world_inject.make
@@ -9,8 +9,7 @@
 world/inject/env = \
 	$(call world/env, $(1)) \
 	pkg_inject_path="$($(1)_INJECT_PATH)" \
-	pkg_inject_files="$($(1)_INJECT_FILES)" \
-	pkg_source="$($(1)_DIR)"
+	pkg_inject_files="$($(1)_INJECT_FILES)"
 
 world/inject = \
 	$(call world/inject/env,$(strip $(1))) \
diff --git a/scripts/lib/ptxd_make_world_inject.sh b/scripts/lib/ptxd_make_world_inject.sh
index 5c2d0dc5f..b74e464c6 100644
--- a/scripts/lib/ptxd_make_world_inject.sh
+++ b/scripts/lib/ptxd_make_world_inject.sh
@@ -10,7 +10,7 @@ ptxd_make_inject() {
     local source target
 
     source="$(echo ${inject_file} | cut -d ":" -f 1)"
-    target="${pkg_source}/$(echo ${inject_file} | cut -d ":" -f 2)"
+    target="${pkg_dir}/$(echo ${inject_file} | cut -d ":" -f 2)"
 
     if [[ "${source}" =~ ^/.* ]]; then
 	ptxd_bailout "'${source}' must not be an absolute path!" \
@@ -32,6 +32,10 @@ export -f ptxd_make_inject
 ptxd_make_world_inject() {
     ptxd_make_world_init || return
 
+    if [ -z "${pkg_dir}" ]; then
+	ptxd_bailout "<PKG>_DIR empty, no destination to inject to."
+    fi
+
     for inject_file in ${pkg_inject_files}; do
 	ptxd_make_inject || return
     done
-- 
2.39.2

[ptxdist] [PATCH 3/4] ptxd_make_world_inject: Escape inject path and files

[Alexander Dahl]

Same as in ptxd_make_world_common for all the other variables set in
make and used in shell.

Signed-off-by: Alexander Dahl <ada@thorsis.com>
---
 rules/post/ptxd_make_world_inject.make | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/rules/post/ptxd_make_world_inject.make b/rules/post/ptxd_make_world_inject.make
index 3506ee114..eabcdd052 100644
--- a/rules/post/ptxd_make_world_inject.make
+++ b/rules/post/ptxd_make_world_inject.make
@@ -8,8 +8,8 @@
 
 world/inject/env = \
 	$(call world/env, $(1)) \
-	pkg_inject_path="$($(1)_INJECT_PATH)" \
-	pkg_inject_files="$($(1)_INJECT_FILES)"
+	pkg_inject_path="$(call ptx/escape,$($(1)_INJECT_PATH))" \
+	pkg_inject_files="$(call ptx/escape,$($(1)_INJECT_FILES))"
 
 world/inject = \
 	$(call world/inject/env,$(strip $(1))) \
-- 
2.39.2

[ptxdist] [PATCH 4/4] ptxd_make_world_inject: Introduce separate destination directory

[Alexander Dahl]

Setting the optional <PKG>_INJECT_DEST now allows to give a different
target for putting the binary blobs into.  When building out-of-tree
some bootloaders like U-Boot expect injected files in the build dir, not
in the source dir.  Backwards compatibility is ensured, the source dir
is still the default, the new inject dest is optional and in case set it
overwrites the default.

For using this in u-boot package, on top of the things already done to
use the inject mechanism in general, add this line to
'rules/u-boot.make' for example:

    U_BOOT_INJECT_DEST     := $(U_BOOT_BUILD_DIR)

Signed-off-by: Alexander Dahl <ada@thorsis.com>
---
 rules/post/ptxd_make_world_inject.make |  3 ++-
 scripts/lib/ptxd_make_world_inject.sh  | 22 ++++++++++++++++++----
 2 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/rules/post/ptxd_make_world_inject.make b/rules/post/ptxd_make_world_inject.make
index eabcdd052..509d480ba 100644
--- a/rules/post/ptxd_make_world_inject.make
+++ b/rules/post/ptxd_make_world_inject.make
@@ -9,7 +9,8 @@
 world/inject/env = \
 	$(call world/env, $(1)) \
 	pkg_inject_path="$(call ptx/escape,$($(1)_INJECT_PATH))" \
-	pkg_inject_files="$(call ptx/escape,$($(1)_INJECT_FILES))"
+	pkg_inject_files="$(call ptx/escape,$($(1)_INJECT_FILES))" \
+	pkg_inject_dest="$(call ptx/escape,$($(1)_INJECT_DEST))"
 
 world/inject = \
 	$(call world/inject/env,$(strip $(1))) \
diff --git a/scripts/lib/ptxd_make_world_inject.sh b/scripts/lib/ptxd_make_world_inject.sh
index b74e464c6..90bd8b684 100644
--- a/scripts/lib/ptxd_make_world_inject.sh
+++ b/scripts/lib/ptxd_make_world_inject.sh
@@ -7,10 +7,15 @@
 #
 
 ptxd_make_inject() {
-    local source target
+    local dest source target
+
+    dest="${pkg_dir}"
+    if [ -n "${pkg_inject_dest}" ]; then
+	dest="${pkg_inject_dest}"
+    fi
 
     source="$(echo ${inject_file} | cut -d ":" -f 1)"
-    target="${pkg_dir}/$(echo ${inject_file} | cut -d ":" -f 2)"
+    target="${dest}/$(echo ${inject_file} | cut -d ":" -f 2)"
 
     if [[ "${source}" =~ ^/.* ]]; then
 	ptxd_bailout "'${source}' must not be an absolute path!" \
@@ -32,8 +37,17 @@ export -f ptxd_make_inject
 ptxd_make_world_inject() {
     ptxd_make_world_init || return
 
-    if [ -z "${pkg_dir}" ]; then
-	ptxd_bailout "<PKG>_DIR empty, no destination to inject to."
+    if [ -z "${pkg_inject_dest}" ] && [ -z "${pkg_dir}" ]; then
+	ptxd_bailout "No destination to inject to." \
+	    "Set either <PKG>_DIR or <PKG>_INJECT_DEST to have a valid destination!"
+    fi
+
+    if [ -n "${pkg_inject_dest}" ]; then
+	if [ "${pkg_build_dir}" = "${pkg_inject_dest}" ] && [ "${pkg_build_oot}" = "YES" ]; then
+		ptxd_warning "<PKG>_INJECT_DEST is set to <PKG>_BUILD_DIR and <PKG>_BUILD_OOT is set to 'YES'." \
+		    "If you called world/inject before world/prepare your files will be removed after injecting."
+	fi
+	mkdir -p -- "${pkg_inject_dest}"
     fi
 
     for inject_file in ${pkg_inject_files}; do
-- 
2.39.2

Re: [ptxdist] [PATCH 4/4] ptxd_make_world_inject: Introduce separate destination directory

[Michael Olbrich]

On Wed, Apr 24, 2024 at 04:31:09PM +0200, Alexander Dahl wrote:
> Setting the optional <PKG>_INJECT_DEST now allows to give a different
> target for putting the binary blobs into.  When building out-of-tree
> some bootloaders like U-Boot expect injected files in the build dir, not
> in the source dir.  Backwards compatibility is ensured, the source dir
> is still the default, the new inject dest is optional and in case set it
> overwrites the default.
> 
> For using this in u-boot package, on top of the things already done to
> use the inject mechanism in general, add this line to
> 'rules/u-boot.make' for example:
> 
>     U_BOOT_INJECT_DEST     := $(U_BOOT_BUILD_DIR)

Can you add a patch for u-boot?

> 
> Signed-off-by: Alexander Dahl <ada@thorsis.com>
> ---
>  rules/post/ptxd_make_world_inject.make |  3 ++-
>  scripts/lib/ptxd_make_world_inject.sh  | 22 ++++++++++++++++++----
>  2 files changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/rules/post/ptxd_make_world_inject.make b/rules/post/ptxd_make_world_inject.make
> index eabcdd052..509d480ba 100644
> --- a/rules/post/ptxd_make_world_inject.make
> +++ b/rules/post/ptxd_make_world_inject.make
> @@ -9,7 +9,8 @@
>  world/inject/env = \
>  	$(call world/env, $(1)) \
>  	pkg_inject_path="$(call ptx/escape,$($(1)_INJECT_PATH))" \
> -	pkg_inject_files="$(call ptx/escape,$($(1)_INJECT_FILES))"
> +	pkg_inject_files="$(call ptx/escape,$($(1)_INJECT_FILES))" \
> +	pkg_inject_dest="$(call ptx/escape,$($(1)_INJECT_DEST))"
>  
>  world/inject = \
>  	$(call world/inject/env,$(strip $(1))) \
> diff --git a/scripts/lib/ptxd_make_world_inject.sh b/scripts/lib/ptxd_make_world_inject.sh
> index b74e464c6..90bd8b684 100644
> --- a/scripts/lib/ptxd_make_world_inject.sh
> +++ b/scripts/lib/ptxd_make_world_inject.sh
> @@ -7,10 +7,15 @@
>  #
>  
>  ptxd_make_inject() {
> -    local source target
> +    local dest source target
> +
> +    dest="${pkg_dir}"
> +    if [ -n "${pkg_inject_dest}" ]; then
> +	dest="${pkg_inject_dest}"
> +    fi

This should be in ptxd_make_world_inject(). It remains the same for all
files.

>  
>      source="$(echo ${inject_file} | cut -d ":" -f 1)"
> -    target="${pkg_dir}/$(echo ${inject_file} | cut -d ":" -f 2)"
> +    target="${dest}/$(echo ${inject_file} | cut -d ":" -f 2)"
>  
>      if [[ "${source}" =~ ^/.* ]]; then
>  	ptxd_bailout "'${source}' must not be an absolute path!" \
> @@ -32,8 +37,17 @@ export -f ptxd_make_inject
>  ptxd_make_world_inject() {
>      ptxd_make_world_init || return
>  
> -    if [ -z "${pkg_dir}" ]; then
> -	ptxd_bailout "<PKG>_DIR empty, no destination to inject to."
> +    if [ -z "${pkg_inject_dest}" ] && [ -z "${pkg_dir}" ]; then
> +	ptxd_bailout "No destination to inject to." \
> +	    "Set either <PKG>_DIR or <PKG>_INJECT_DEST to have a valid destination!"
> +    fi
> +
> +    if [ -n "${pkg_inject_dest}" ]; then
> +	if [ "${pkg_build_dir}" = "${pkg_inject_dest}" ] && [ "${pkg_build_oot}" = "YES" ]; then

This gives a warning even in the good case, right? I think we should
require that pkg_inject_dest already exists at this point and fail if it
does not.

Michael

> +		ptxd_warning "<PKG>_INJECT_DEST is set to <PKG>_BUILD_DIR and <PKG>_BUILD_OOT is set to 'YES'." \
> +		    "If you called world/inject before world/prepare your files will be removed after injecting."
> +	fi
> +	mkdir -p -- "${pkg_inject_dest}"
>      fi
>  
>      for inject_file in ${pkg_inject_files}; do
> -- 
> 2.39.2
> 
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [ptxdist] [PATCH 4/4] ptxd_make_world_inject: Introduce separate destination directory

[Alexander Dahl]

Hello Michael,

thanks for reviewing, answers below …

Am Fri, May 03, 2024 at 11:48:52AM +0200 schrieb Michael Olbrich:
> On Wed, Apr 24, 2024 at 04:31:09PM +0200, Alexander Dahl wrote:
> > Setting the optional <PKG>_INJECT_DEST now allows to give a different
> > target for putting the binary blobs into.  When building out-of-tree
> > some bootloaders like U-Boot expect injected files in the build dir, not
> > in the source dir.  Backwards compatibility is ensured, the source dir
> > is still the default, the new inject dest is optional and in case set it
> > overwrites the default.
> > 
> > For using this in u-boot package, on top of the things already done to
> > use the inject mechanism in general, add this line to
> > 'rules/u-boot.make' for example:
> > 
> >     U_BOOT_INJECT_DEST     := $(U_BOOT_BUILD_DIR)
> 
> Can you add a patch for u-boot?

I will, in an upcoming patch series with more changes to the u-boot
package.  Planned to this after lunch later today anyways. ^^

> > 
> > Signed-off-by: Alexander Dahl <ada@thorsis.com>
> > ---
> >  rules/post/ptxd_make_world_inject.make |  3 ++-
> >  scripts/lib/ptxd_make_world_inject.sh  | 22 ++++++++++++++++++----
> >  2 files changed, 20 insertions(+), 5 deletions(-)
> > 
> > diff --git a/rules/post/ptxd_make_world_inject.make b/rules/post/ptxd_make_world_inject.make
> > index eabcdd052..509d480ba 100644
> > --- a/rules/post/ptxd_make_world_inject.make
> > +++ b/rules/post/ptxd_make_world_inject.make
> > @@ -9,7 +9,8 @@
> >  world/inject/env = \
> >  	$(call world/env, $(1)) \
> >  	pkg_inject_path="$(call ptx/escape,$($(1)_INJECT_PATH))" \
> > -	pkg_inject_files="$(call ptx/escape,$($(1)_INJECT_FILES))"
> > +	pkg_inject_files="$(call ptx/escape,$($(1)_INJECT_FILES))" \
> > +	pkg_inject_dest="$(call ptx/escape,$($(1)_INJECT_DEST))"
> >  
> >  world/inject = \
> >  	$(call world/inject/env,$(strip $(1))) \
> > diff --git a/scripts/lib/ptxd_make_world_inject.sh b/scripts/lib/ptxd_make_world_inject.sh
> > index b74e464c6..90bd8b684 100644
> > --- a/scripts/lib/ptxd_make_world_inject.sh
> > +++ b/scripts/lib/ptxd_make_world_inject.sh
> > @@ -7,10 +7,15 @@
> >  #
> >  
> >  ptxd_make_inject() {
> > -    local source target
> > +    local dest source target
> > +
> > +    dest="${pkg_dir}"
> > +    if [ -n "${pkg_inject_dest}" ]; then
> > +	dest="${pkg_inject_dest}"
> > +    fi
> 
> This should be in ptxd_make_world_inject(). It remains the same for all
> files.

I thought about this when changing it, but decided to keep it at this
place.  Moving it to ptxd_make_world_inject() would require to
introduce another non-local variable and I did not want that.  But
sure, can be done.

> 
> >  
> >      source="$(echo ${inject_file} | cut -d ":" -f 1)"
> > -    target="${pkg_dir}/$(echo ${inject_file} | cut -d ":" -f 2)"
> > +    target="${dest}/$(echo ${inject_file} | cut -d ":" -f 2)"
> >  
> >      if [[ "${source}" =~ ^/.* ]]; then
> >  	ptxd_bailout "'${source}' must not be an absolute path!" \
> > @@ -32,8 +37,17 @@ export -f ptxd_make_inject
> >  ptxd_make_world_inject() {
> >      ptxd_make_world_init || return
> >  
> > -    if [ -z "${pkg_dir}" ]; then
> > -	ptxd_bailout "<PKG>_DIR empty, no destination to inject to."
> > +    if [ -z "${pkg_inject_dest}" ] && [ -z "${pkg_dir}" ]; then
> > +	ptxd_bailout "No destination to inject to." \
> > +	    "Set either <PKG>_DIR or <PKG>_INJECT_DEST to have a valid destination!"
> > +    fi
> > +
> > +    if [ -n "${pkg_inject_dest}" ]; then
> > +	if [ "${pkg_build_dir}" = "${pkg_inject_dest}" ] && [ "${pkg_build_oot}" = "YES" ]; then
> 
> This gives a warning even in the good case, right?

No it does not.  In the good case user has set either <PKG>_DIR or
<PKG>_INJECT_DEST to a not empty string and the bailout does not
happen.

The warning inside this more complicated if statement is just covering
this one edge case which I ran into when developing, and the text
should describe what happens, why the user would not find the files in
<PKG>_BUILD_DIR in that case.

The mkdir is only dependend on <PKG>_INJECT_DEST set, because there's
no need to create that dir if the feature is not used.

> I think we should require that pkg_inject_dest already exists at
> this point and fail if it does not.

This would move the responsibility to create that dir to the package
author.  I opted against that, because <PKG>_DIR and <PKG>_BUILD_DIR
are also implicitly created by ptxdist, no need to create those in the
make rules.  This is a question of policy, but it would make the
inject mechanism somewhat asymmetric if <PKG>_DIR (for when
_INJECT_DEST is not set) has not to be created by the package author
but <PKG>_INJECT_DEST has to.

Again: sure, can be done if desired.

Let me know if I should change the pieces or if my explanations made
sense and things can be kept like this?

Greets
Alex

> Michael
> 
> > +		ptxd_warning "<PKG>_INJECT_DEST is set to <PKG>_BUILD_DIR and <PKG>_BUILD_OOT is set to 'YES'." \
> > +		    "If you called world/inject before world/prepare your files will be removed after injecting."
> > +	fi
> > +	mkdir -p -- "${pkg_inject_dest}"
> >      fi
> >  
> >      for inject_file in ${pkg_inject_files}; do
> > -- 
> > 2.39.2
> > 
> > 
> > 
> 
> -- 
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> 

Re: [ptxdist] [PATCH 4/4] ptxd_make_world_inject: Introduce separate destination directory

[Michael Olbrich]

On Fri, May 03, 2024 at 12:08:34PM +0200, Alexander Dahl wrote:
> Am Fri, May 03, 2024 at 11:48:52AM +0200 schrieb Michael Olbrich:
> > On Wed, Apr 24, 2024 at 04:31:09PM +0200, Alexander Dahl wrote:
> > > Setting the optional <PKG>_INJECT_DEST now allows to give a different
> > > target for putting the binary blobs into.  When building out-of-tree
> > > some bootloaders like U-Boot expect injected files in the build dir, not
> > > in the source dir.  Backwards compatibility is ensured, the source dir
> > > is still the default, the new inject dest is optional and in case set it
> > > overwrites the default.
> > > 
> > > For using this in u-boot package, on top of the things already done to
> > > use the inject mechanism in general, add this line to
> > > 'rules/u-boot.make' for example:
> > > 
> > >     U_BOOT_INJECT_DEST     := $(U_BOOT_BUILD_DIR)
> > 
> > Can you add a patch for u-boot?
> 
> I will, in an upcoming patch series with more changes to the u-boot
> package.  Planned to this after lunch later today anyways. ^^
> 
> > > 
> > > Signed-off-by: Alexander Dahl <ada@thorsis.com>
> > > ---
> > >  rules/post/ptxd_make_world_inject.make |  3 ++-
> > >  scripts/lib/ptxd_make_world_inject.sh  | 22 ++++++++++++++++++----
> > >  2 files changed, 20 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/rules/post/ptxd_make_world_inject.make b/rules/post/ptxd_make_world_inject.make
> > > index eabcdd052..509d480ba 100644
> > > --- a/rules/post/ptxd_make_world_inject.make
> > > +++ b/rules/post/ptxd_make_world_inject.make
> > > @@ -9,7 +9,8 @@
> > >  world/inject/env = \
> > >  	$(call world/env, $(1)) \
> > >  	pkg_inject_path="$(call ptx/escape,$($(1)_INJECT_PATH))" \
> > > -	pkg_inject_files="$(call ptx/escape,$($(1)_INJECT_FILES))"
> > > +	pkg_inject_files="$(call ptx/escape,$($(1)_INJECT_FILES))" \
> > > +	pkg_inject_dest="$(call ptx/escape,$($(1)_INJECT_DEST))"
> > >  
> > >  world/inject = \
> > >  	$(call world/inject/env,$(strip $(1))) \
> > > diff --git a/scripts/lib/ptxd_make_world_inject.sh b/scripts/lib/ptxd_make_world_inject.sh
> > > index b74e464c6..90bd8b684 100644
> > > --- a/scripts/lib/ptxd_make_world_inject.sh
> > > +++ b/scripts/lib/ptxd_make_world_inject.sh
> > > @@ -7,10 +7,15 @@
> > >  #
> > >  
> > >  ptxd_make_inject() {
> > > -    local source target
> > > +    local dest source target
> > > +
> > > +    dest="${pkg_dir}"
> > > +    if [ -n "${pkg_inject_dest}" ]; then
> > > +	dest="${pkg_inject_dest}"
> > > +    fi
> > 
> > This should be in ptxd_make_world_inject(). It remains the same for all
> > files.
> 
> I thought about this when changing it, but decided to keep it at this
> place.  Moving it to ptxd_make_world_inject() would require to
> introduce another non-local variable and I did not want that.  But
> sure, can be done.

Just set pkg_inject_dest if it's empty. No need for a separate variable.

> > 
> > >  
> > >      source="$(echo ${inject_file} | cut -d ":" -f 1)"
> > > -    target="${pkg_dir}/$(echo ${inject_file} | cut -d ":" -f 2)"
> > > +    target="${dest}/$(echo ${inject_file} | cut -d ":" -f 2)"
> > >  
> > >      if [[ "${source}" =~ ^/.* ]]; then
> > >  	ptxd_bailout "'${source}' must not be an absolute path!" \
> > > @@ -32,8 +37,17 @@ export -f ptxd_make_inject
> > >  ptxd_make_world_inject() {
> > >      ptxd_make_world_init || return
> > >  
> > > -    if [ -z "${pkg_dir}" ]; then
> > > -	ptxd_bailout "<PKG>_DIR empty, no destination to inject to."
> > > +    if [ -z "${pkg_inject_dest}" ] && [ -z "${pkg_dir}" ]; then
> > > +	ptxd_bailout "No destination to inject to." \
> > > +	    "Set either <PKG>_DIR or <PKG>_INJECT_DEST to have a valid destination!"
> > > +    fi
> > > +
> > > +    if [ -n "${pkg_inject_dest}" ]; then
> > > +	if [ "${pkg_build_dir}" = "${pkg_inject_dest}" ] && [ "${pkg_build_oot}" = "YES" ]; then
> > 
> > This gives a warning even in the good case, right?
> 
> No it does not.  In the good case user has set either <PKG>_DIR or
> <PKG>_INJECT_DEST to a not empty string and the bailout does not
> happen.
> 
> The warning inside this more complicated if statement is just covering
> this one edge case which I ran into when developing, and the text
> should describe what happens, why the user would not find the files in
> <PKG>_BUILD_DIR in that case.

Still, for u-boot the warning will always be visible right? That's not
nice.

> The mkdir is only dependend on <PKG>_INJECT_DEST set, because there's
> no need to create that dir if the feature is not used.

Setting <PKG>_INJECT_DEST to anything other than pkg_dir or pkg_build_dir
does not really make sense. The whole point of this is, to copy files
there.
Hmmm, maybe we should make that explicit:

<PKG>_INJECT_OOT := YES

Or something else that indicates that the pkg_build_dir should be used. So
don't make it more flexible than needed.

And then you can definitely check for the directory existence to determine
if the order was correct.

> > I think we should require that pkg_inject_dest already exists at
> > this point and fail if it does not.
> 
> This would move the responsibility to create that dir to the package
> author.  I opted against that, because <PKG>_DIR and <PKG>_BUILD_DIR
> are also implicitly created by ptxdist, no need to create those in the
> make rules.  This is a question of policy, but it would make the
> inject mechanism somewhat asymmetric if <PKG>_DIR (for when
> _INJECT_DEST is not set) has not to be created by the package author
> but <PKG>_INJECT_DEST has to.

For pkg_build_dir there are clear rules, when it's created: during
world/prepare. I think it's fine to require that. Or world/execute for a
custom prepare command. u-boot should actually use that.

> Again: sure, can be done if desired.
> 
> Let me know if I should change the pieces or if my explanations made
> sense and things can be kept like this?

Michael

> > > +		ptxd_warning "<PKG>_INJECT_DEST is set to <PKG>_BUILD_DIR and <PKG>_BUILD_OOT is set to 'YES'." \
> > > +		    "If you called world/inject before world/prepare your files will be removed after injecting."
> > > +	fi
> > > +	mkdir -p -- "${pkg_inject_dest}"
> > >      fi
> > >  
> > >      for inject_file in ${pkg_inject_files}; do
> > > -- 
> > > 2.39.2
> > > 
> > > 
> > > 
> > 
> > -- 
> > Pengutronix e.K.                           |                             |
> > Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> > 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> > Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> > 
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [ptxdist] [PATCH 4/4] ptxd_make_world_inject: Introduce separate destination directory

[Alexander Dahl]

Hello Michael,

Am Fri, May 03, 2024 at 05:31:29PM +0200 schrieb Michael Olbrich:
> On Fri, May 03, 2024 at 12:08:34PM +0200, Alexander Dahl wrote:
> > Am Fri, May 03, 2024 at 11:48:52AM +0200 schrieb Michael Olbrich:
> > > On Wed, Apr 24, 2024 at 04:31:09PM +0200, Alexander Dahl wrote:
> > > > Setting the optional <PKG>_INJECT_DEST now allows to give a different
> > > > target for putting the binary blobs into.  When building out-of-tree
> > > > some bootloaders like U-Boot expect injected files in the build dir, not
> > > > in the source dir.  Backwards compatibility is ensured, the source dir
> > > > is still the default, the new inject dest is optional and in case set it
> > > > overwrites the default.
> > > > 
> > > > For using this in u-boot package, on top of the things already done to
> > > > use the inject mechanism in general, add this line to
> > > > 'rules/u-boot.make' for example:
> > > > 
> > > >     U_BOOT_INJECT_DEST     := $(U_BOOT_BUILD_DIR)
> > > 
> > > Can you add a patch for u-boot?
> > 
> > I will, in an upcoming patch series with more changes to the u-boot
> > package.  Planned to this after lunch later today anyways. ^^
> > 
> > > > 
> > > > Signed-off-by: Alexander Dahl <ada@thorsis.com>
> > > > ---
> > > >  rules/post/ptxd_make_world_inject.make |  3 ++-
> > > >  scripts/lib/ptxd_make_world_inject.sh  | 22 ++++++++++++++++++----
> > > >  2 files changed, 20 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/rules/post/ptxd_make_world_inject.make b/rules/post/ptxd_make_world_inject.make
> > > > index eabcdd052..509d480ba 100644
> > > > --- a/rules/post/ptxd_make_world_inject.make
> > > > +++ b/rules/post/ptxd_make_world_inject.make
> > > > @@ -9,7 +9,8 @@
> > > >  world/inject/env = \
> > > >  	$(call world/env, $(1)) \
> > > >  	pkg_inject_path="$(call ptx/escape,$($(1)_INJECT_PATH))" \
> > > > -	pkg_inject_files="$(call ptx/escape,$($(1)_INJECT_FILES))"
> > > > +	pkg_inject_files="$(call ptx/escape,$($(1)_INJECT_FILES))" \
> > > > +	pkg_inject_dest="$(call ptx/escape,$($(1)_INJECT_DEST))"
> > > >  
> > > >  world/inject = \
> > > >  	$(call world/inject/env,$(strip $(1))) \
> > > > diff --git a/scripts/lib/ptxd_make_world_inject.sh b/scripts/lib/ptxd_make_world_inject.sh
> > > > index b74e464c6..90bd8b684 100644
> > > > --- a/scripts/lib/ptxd_make_world_inject.sh
> > > > +++ b/scripts/lib/ptxd_make_world_inject.sh
> > > > @@ -7,10 +7,15 @@
> > > >  #
> > > >  
> > > >  ptxd_make_inject() {
> > > > -    local source target
> > > > +    local dest source target
> > > > +
> > > > +    dest="${pkg_dir}"
> > > > +    if [ -n "${pkg_inject_dest}" ]; then
> > > > +	dest="${pkg_inject_dest}"
> > > > +    fi
> > > 
> > > This should be in ptxd_make_world_inject(). It remains the same for all
> > > files.
> > 
> > I thought about this when changing it, but decided to keep it at this
> > place.  Moving it to ptxd_make_world_inject() would require to
> > introduce another non-local variable and I did not want that.  But
> > sure, can be done.
> 
> Just set pkg_inject_dest if it's empty. No need for a separate variable.

Ack.

> > > >      source="$(echo ${inject_file} | cut -d ":" -f 1)"
> > > > -    target="${pkg_dir}/$(echo ${inject_file} | cut -d ":" -f 2)"
> > > > +    target="${dest}/$(echo ${inject_file} | cut -d ":" -f 2)"
> > > >  
> > > >      if [[ "${source}" =~ ^/.* ]]; then
> > > >  	ptxd_bailout "'${source}' must not be an absolute path!" \
> > > > @@ -32,8 +37,17 @@ export -f ptxd_make_inject
> > > >  ptxd_make_world_inject() {
> > > >      ptxd_make_world_init || return
> > > >  
> > > > -    if [ -z "${pkg_dir}" ]; then
> > > > -	ptxd_bailout "<PKG>_DIR empty, no destination to inject to."
> > > > +    if [ -z "${pkg_inject_dest}" ] && [ -z "${pkg_dir}" ]; then
> > > > +	ptxd_bailout "No destination to inject to." \
> > > > +	    "Set either <PKG>_DIR or <PKG>_INJECT_DEST to have a valid destination!"
> > > > +    fi
> > > > +
> > > > +    if [ -n "${pkg_inject_dest}" ]; then
> > > > +	if [ "${pkg_build_dir}" = "${pkg_inject_dest}" ] && [ "${pkg_build_oot}" = "YES" ]; then
> > > 
> > > This gives a warning even in the good case, right?
> > 
> > No it does not.  In the good case user has set either <PKG>_DIR or
> > <PKG>_INJECT_DEST to a not empty string and the bailout does not
> > happen.
> > 
> > The warning inside this more complicated if statement is just covering
> > this one edge case which I ran into when developing, and the text
> > should describe what happens, why the user would not find the files in
> > <PKG>_BUILD_DIR in that case.
> 
> Still, for u-boot the warning will always be visible right? That's not
> nice.

No, it is not always visible.  Warning is only visible if
<PKG>_BUILD_OOT is set to 'YES'.  For u-boot this is actually never
the case, because there it's either 'KEEP' or 'NO'.  But the code is
generic, thus the warning for this edge case.

When testing this I had exactly that case: inject destination set to
build dir and build_oot set to 'YES' and then I wondered where the
files are.  They get deleted by some other ptxdist script magic
because the whole build_dir is removed before it is recreated again.
Not sure in what stage though, but this is a thing 'KEEP' is
preventing.

> > The mkdir is only dependend on <PKG>_INJECT_DEST set, because there's
> > no need to create that dir if the feature is not used.
> 
> Setting <PKG>_INJECT_DEST to anything other than pkg_dir or pkg_build_dir
> does not really make sense. The whole point of this is, to copy files
> there.
> Hmmm, maybe we should make that explicit:
> 
> <PKG>_INJECT_OOT := YES

This is nice, I really like that.

> Or something else that indicates that the pkg_build_dir should be used. So
> don't make it more flexible than needed.
> 
> And then you can definitely check for the directory existence to determine
> if the order was correct.

Will check the implications of that next week.

> > > I think we should require that pkg_inject_dest already exists at
> > > this point and fail if it does not.
> > 
> > This would move the responsibility to create that dir to the package
> > author.  I opted against that, because <PKG>_DIR and <PKG>_BUILD_DIR
> > are also implicitly created by ptxdist, no need to create those in the
> > make rules.  This is a question of policy, but it would make the
> > inject mechanism somewhat asymmetric if <PKG>_DIR (for when
> > _INJECT_DEST is not set) has not to be created by the package author
> > but <PKG>_INJECT_DEST has to.
> 
> For pkg_build_dir there are clear rules, when it's created: during
> world/prepare. I think it's fine to require that. Or world/execute for a
> custom prepare command. u-boot should actually use that.

Well, I'll look into that again after reworking the destination folder
thing, next week.

Thanks for your input and have a nice weekend.

Greets
Alex

> 
> > Again: sure, can be done if desired.
> > 
> > Let me know if I should change the pieces or if my explanations made
> > sense and things can be kept like this?
> 
> Michael
> 
> > > > +		ptxd_warning "<PKG>_INJECT_DEST is set to <PKG>_BUILD_DIR and <PKG>_BUILD_OOT is set to 'YES'." \
> > > > +		    "If you called world/inject before world/prepare your files will be removed after injecting."
> > > > +	fi
> > > > +	mkdir -p -- "${pkg_inject_dest}"
> > > >      fi
> > > >  
> > > >      for inject_file in ${pkg_inject_files}; do
> > > > -- 
> > > > 2.39.2
> > > > 
> > > > 
> > > > 
> > > 
> > > -- 
> > > Pengutronix e.K.                           |                             |
> > > Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> > > 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> > > Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> > > 
> > 
> > 
> 
> -- 
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
>