* [ptxdist] [PATCH 0/4] ptxd_make_world_inject: Spring cleanup and optional dest dir
@ 2024-04-24 14:31 Alexander Dahl
2024-04-24 14:31 ` [ptxdist] [PATCH 1/4] ptxd_make_world_inject: Remove useless test Alexander Dahl
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: Alexander Dahl @ 2024-04-24 14:31 UTC (permalink / raw)
To: ptxdist; +Cc: Michael Riesch
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
^ permalink raw reply [flat|nested] 9+ messages in thread* [ptxdist] [PATCH 1/4] ptxd_make_world_inject: Remove useless test 2024-04-24 14:31 [ptxdist] [PATCH 0/4] ptxd_make_world_inject: Spring cleanup and optional dest dir Alexander Dahl @ 2024-04-24 14:31 ` Alexander Dahl 2024-04-24 14:31 ` [ptxdist] [PATCH 2/4] ptxd_make_world_inject: Use <PKG>_DIR directly Alexander Dahl ` (2 subsequent siblings) 3 siblings, 0 replies; 9+ messages in thread From: Alexander Dahl @ 2024-04-24 14:31 UTC (permalink / raw) To: ptxdist; +Cc: Michael Riesch 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [ptxdist] [PATCH 2/4] ptxd_make_world_inject: Use <PKG>_DIR directly 2024-04-24 14:31 [ptxdist] [PATCH 0/4] ptxd_make_world_inject: Spring cleanup and optional dest dir Alexander Dahl 2024-04-24 14:31 ` [ptxdist] [PATCH 1/4] ptxd_make_world_inject: Remove useless test Alexander Dahl @ 2024-04-24 14:31 ` Alexander Dahl 2024-04-24 14:31 ` [ptxdist] [PATCH 3/4] ptxd_make_world_inject: Escape inject path and files Alexander Dahl 2024-04-24 14:31 ` [ptxdist] [PATCH 4/4] ptxd_make_world_inject: Introduce separate destination directory Alexander Dahl 3 siblings, 0 replies; 9+ messages in thread From: Alexander Dahl @ 2024-04-24 14:31 UTC (permalink / raw) To: ptxdist; +Cc: Michael Riesch 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [ptxdist] [PATCH 3/4] ptxd_make_world_inject: Escape inject path and files 2024-04-24 14:31 [ptxdist] [PATCH 0/4] ptxd_make_world_inject: Spring cleanup and optional dest dir Alexander Dahl 2024-04-24 14:31 ` [ptxdist] [PATCH 1/4] ptxd_make_world_inject: Remove useless test Alexander Dahl 2024-04-24 14:31 ` [ptxdist] [PATCH 2/4] ptxd_make_world_inject: Use <PKG>_DIR directly Alexander Dahl @ 2024-04-24 14:31 ` Alexander Dahl 2024-04-24 14:31 ` [ptxdist] [PATCH 4/4] ptxd_make_world_inject: Introduce separate destination directory Alexander Dahl 3 siblings, 0 replies; 9+ messages in thread From: Alexander Dahl @ 2024-04-24 14:31 UTC (permalink / raw) To: ptxdist; +Cc: Michael Riesch 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [ptxdist] [PATCH 4/4] ptxd_make_world_inject: Introduce separate destination directory 2024-04-24 14:31 [ptxdist] [PATCH 0/4] ptxd_make_world_inject: Spring cleanup and optional dest dir Alexander Dahl ` (2 preceding siblings ...) 2024-04-24 14:31 ` [ptxdist] [PATCH 3/4] ptxd_make_world_inject: Escape inject path and files Alexander Dahl @ 2024-04-24 14:31 ` Alexander Dahl 2024-05-03 9:48 ` Michael Olbrich 3 siblings, 1 reply; 9+ messages in thread From: Alexander Dahl @ 2024-04-24 14:31 UTC (permalink / raw) To: ptxdist; +Cc: Michael Riesch 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [ptxdist] [PATCH 4/4] ptxd_make_world_inject: Introduce separate destination directory 2024-04-24 14:31 ` [ptxdist] [PATCH 4/4] ptxd_make_world_inject: Introduce separate destination directory Alexander Dahl @ 2024-05-03 9:48 ` Michael Olbrich 2024-05-03 10:08 ` Alexander Dahl 0 siblings, 1 reply; 9+ messages in thread From: Michael Olbrich @ 2024-05-03 9:48 UTC (permalink / raw) To: Alexander Dahl; +Cc: ptxdist, Michael Riesch 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 | ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [ptxdist] [PATCH 4/4] ptxd_make_world_inject: Introduce separate destination directory 2024-05-03 9:48 ` Michael Olbrich @ 2024-05-03 10:08 ` Alexander Dahl 2024-05-03 15:31 ` Michael Olbrich 0 siblings, 1 reply; 9+ messages in thread From: Alexander Dahl @ 2024-05-03 10:08 UTC (permalink / raw) To: Alexander Dahl, ptxdist, Michael Riesch 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 | > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [ptxdist] [PATCH 4/4] ptxd_make_world_inject: Introduce separate destination directory 2024-05-03 10:08 ` Alexander Dahl @ 2024-05-03 15:31 ` Michael Olbrich 2024-05-03 16:05 ` Alexander Dahl 0 siblings, 1 reply; 9+ messages in thread From: Michael Olbrich @ 2024-05-03 15:31 UTC (permalink / raw) To: ptxdist, Michael Riesch 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 | ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [ptxdist] [PATCH 4/4] ptxd_make_world_inject: Introduce separate destination directory 2024-05-03 15:31 ` Michael Olbrich @ 2024-05-03 16:05 ` Alexander Dahl 0 siblings, 0 replies; 9+ messages in thread From: Alexander Dahl @ 2024-05-03 16:05 UTC (permalink / raw) To: ptxdist, Michael Riesch 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 | > ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2024-05-03 16:05 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-04-24 14:31 [ptxdist] [PATCH 0/4] ptxd_make_world_inject: Spring cleanup and optional dest dir Alexander Dahl 2024-04-24 14:31 ` [ptxdist] [PATCH 1/4] ptxd_make_world_inject: Remove useless test Alexander Dahl 2024-04-24 14:31 ` [ptxdist] [PATCH 2/4] ptxd_make_world_inject: Use <PKG>_DIR directly Alexander Dahl 2024-04-24 14:31 ` [ptxdist] [PATCH 3/4] ptxd_make_world_inject: Escape inject path and files Alexander Dahl 2024-04-24 14:31 ` [ptxdist] [PATCH 4/4] ptxd_make_world_inject: Introduce separate destination directory Alexander Dahl 2024-05-03 9:48 ` Michael Olbrich 2024-05-03 10:08 ` Alexander Dahl 2024-05-03 15:31 ` Michael Olbrich 2024-05-03 16:05 ` Alexander Dahl
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox