* [PATCH v2 0/1] Makefile: rework u-boot-initial-env target
@ 2022-10-28 16:18 Max Krummenacher
2022-10-28 16:18 ` [PATCH v2 1/1] " Max Krummenacher
0 siblings, 1 reply; 9+ messages in thread
From: Max Krummenacher @ 2022-10-28 16:18 UTC (permalink / raw)
To: u-boot
Cc: Tom Rini, U-Boot STM32, Pali Rohár, Adam Ford,
Patrick DELAUNAY, Patrice CHOTARD, Max Krummenacher,
Heinrich Schuchardt, Marek Behún, Philippe Reynes,
Quentin Schulz, Simon Glass
From: Max Krummenacher <max.krummenacher@toradex.com>
With CONFIG_LTO enabled the current way of extracting the
configured environment no longer works, i.e. the object file
content changes due to LTO.
Build a host tool which prints the configured environment instead
of using objcopy and friends to achive the same.
The code and Makefile changes were mostly stolen from tools/env/
i.e. the target userspace tools to access the environment.
Changes in v2:
- reworked to build a host tool which prints the configured
environment as proposed by Pali Rohár
https://lore.kernel.org/u-boot/20221018174827.1393211-1-max.oss.09@gmail.com/
- renamed patch, v1 used "Makefile: fix u-boot-initial-env target if lto is enabled"
Max Krummenacher (1):
Makefile: rework u-boot-initial-env target
Makefile | 7 ++++---
scripts/.gitignore | 1 +
scripts/Makefile | 5 +++++
scripts/printinitialenv.c | 44 +++++++++++++++++++++++++++++++++++++++
4 files changed, 54 insertions(+), 3 deletions(-)
create mode 100644 scripts/printinitialenv.c
--
2.35.3
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 1/1] Makefile: rework u-boot-initial-env target
2022-10-28 16:18 [PATCH v2 0/1] Makefile: rework u-boot-initial-env target Max Krummenacher
@ 2022-10-28 16:18 ` Max Krummenacher
2022-10-28 16:40 ` Pali Rohár
0 siblings, 1 reply; 9+ messages in thread
From: Max Krummenacher @ 2022-10-28 16:18 UTC (permalink / raw)
To: u-boot
Cc: Tom Rini, U-Boot STM32, Pali Rohár, Adam Ford,
Patrick DELAUNAY, Patrice CHOTARD, Max Krummenacher,
Heinrich Schuchardt, Marek Behún, Philippe Reynes,
Quentin Schulz, Simon Glass
From: Max Krummenacher <max.krummenacher@toradex.com>
With LTO enabled the U-Boot initial environment is no longer stored
in an easy accessible section in env/common.o. I.e. the section name
changes from build to build, its content maybe compressed and it is
annotated with additional data.
Drop trying to read the initial env with elf tools from the compiler
specific object file in favour of adding and using a host tool with
the only functionality of printing the initial env to stdout.
See also:
https://lore.kernel.org/all/927b122e-1f62-e790-f5ca-30bae4332c77@foss.st.com/
Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com>
---
Changes in v2:
- reworked to build a host tool which prints the configured
environment as proposed by Pali Rohár
https://lore.kernel.org/u-boot/20221018174827.1393211-1-max.oss.09@gmail.com/
- renamed patch, v1 used "Makefile: fix u-boot-initial-env target if lto is enabled"
Makefile | 7 ++++---
scripts/.gitignore | 1 +
scripts/Makefile | 5 +++++
scripts/printinitialenv.c | 44 +++++++++++++++++++++++++++++++++++++++
4 files changed, 54 insertions(+), 3 deletions(-)
create mode 100644 scripts/printinitialenv.c
diff --git a/Makefile b/Makefile
index 0f1174718f7..2f97d63c398 100644
--- a/Makefile
+++ b/Makefile
@@ -2442,9 +2442,10 @@ endif
$(Q)$(MAKE) -f $(srctree)/scripts/Makefile.modpost
quiet_cmd_genenv = GENENV $@
-cmd_genenv = $(OBJCOPY) --dump-section .rodata.default_environment=$@ env/common.o; \
- sed --in-place -e 's/\x00/\x0A/g' $@; sed --in-place -e '/^\s*$$/d' $@; \
- sort --field-separator== -k1,1 --stable $@ -o $@
+cmd_genenv = \
+ scripts/printinitialenv | \
+ sed -e 's/\x00/\x0A/g' -e '/^\s*$$/d' | \
+ sort --field-separator== -k1,1 --stable -o $@
u-boot-initial-env: u-boot.bin
$(call if_changed,genenv)
diff --git a/scripts/.gitignore b/scripts/.gitignore
index 08cc824bac3..6068724a0d4 100644
--- a/scripts/.gitignore
+++ b/scripts/.gitignore
@@ -2,3 +2,4 @@
# Generated files
#
bin2c
+printinitialenv
diff --git a/scripts/Makefile b/scripts/Makefile
index 8731e6cecd7..ba993820571 100644
--- a/scripts/Makefile
+++ b/scripts/Makefile
@@ -5,8 +5,13 @@
# ---------------------------------------------------------------------------
hostprogs-$(CONFIG_BUILD_BIN2C) += bin2c
+hostprogs-y += printinitialenv
always := $(hostprogs-y)
+HOSTCFLAGS_printinitialenv.o += \
+ $(patsubst -I%,-idirafter%, $(filter -I%, $(UBOOTINCLUDE))) \
+ -DUSE_HOSTCC
+
# Let clean descend into subdirs
subdir- += basic kconfig dtc
diff --git a/scripts/printinitialenv.c b/scripts/printinitialenv.c
new file mode 100644
index 00000000000..c58b234d679
--- /dev/null
+++ b/scripts/printinitialenv.c
@@ -0,0 +1,44 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * (C) Copyright 2022
+ * Max Krummenacher, Toradex
+ *
+ * Snippets taken from tools/env/fw_env.c
+ *
+ * This prints the list of default environment variables as currently
+ * configured.
+ *
+ */
+
+#include <stdio.h>
+
+/* Pull in the current config to define the default environment */
+#include <linux/kconfig.h>
+
+#ifndef __ASSEMBLY__
+#define __ASSEMBLY__ /* get only #defines from config.h */
+#include <config.h>
+#undef __ASSEMBLY__
+#else
+#include <config.h>
+#endif
+
+#define DEFAULT_ENV_INSTANCE_STATIC
+#include <generated/environment.h>
+#include <env_default.h>
+
+int main(void)
+{
+ char *env, *nxt;
+
+ for (env = default_environment; *env; env = nxt + 1) {
+ for (nxt = env; *nxt; ++nxt) {
+ if (nxt >= &default_environment[sizeof(default_environment)]) {
+ fprintf(stderr, "## Error: environment not terminated\n");
+ return -1;
+ }
+ }
+ printf("%s\n", env);
+ }
+ return 0;
+}
--
2.35.3
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/1] Makefile: rework u-boot-initial-env target
2022-10-28 16:18 ` [PATCH v2 1/1] " Max Krummenacher
@ 2022-10-28 16:40 ` Pali Rohár
2022-10-31 10:51 ` Max Krummenacher
0 siblings, 1 reply; 9+ messages in thread
From: Pali Rohár @ 2022-10-28 16:40 UTC (permalink / raw)
To: Max Krummenacher
Cc: u-boot, Tom Rini, U-Boot STM32, Adam Ford, Patrick DELAUNAY,
Patrice CHOTARD, Max Krummenacher, Heinrich Schuchardt,
Marek Behún, Philippe Reynes, Quentin Schulz, Simon Glass
Hello! This is really much better solution! Few comments are below.
On Friday 28 October 2022 18:18:49 Max Krummenacher wrote:
> From: Max Krummenacher <max.krummenacher@toradex.com>
>
> With LTO enabled the U-Boot initial environment is no longer stored
> in an easy accessible section in env/common.o. I.e. the section name
> changes from build to build, its content maybe compressed and it is
> annotated with additional data.
>
> Drop trying to read the initial env with elf tools from the compiler
> specific object file in favour of adding and using a host tool with
> the only functionality of printing the initial env to stdout.
>
> See also:
> https://lore.kernel.org/all/927b122e-1f62-e790-f5ca-30bae4332c77@foss.st.com/
>
> Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com>
>
> ---
>
> Changes in v2:
> - reworked to build a host tool which prints the configured
> environment as proposed by Pali Rohár
> https://lore.kernel.org/u-boot/20221018174827.1393211-1-max.oss.09@gmail.com/
> - renamed patch, v1 used "Makefile: fix u-boot-initial-env target if lto is enabled"
>
> Makefile | 7 ++++---
> scripts/.gitignore | 1 +
> scripts/Makefile | 5 +++++
> scripts/printinitialenv.c | 44 +++++++++++++++++++++++++++++++++++++++
> 4 files changed, 54 insertions(+), 3 deletions(-)
> create mode 100644 scripts/printinitialenv.c
>
> diff --git a/Makefile b/Makefile
> index 0f1174718f7..2f97d63c398 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2442,9 +2442,10 @@ endif
> $(Q)$(MAKE) -f $(srctree)/scripts/Makefile.modpost
>
> quiet_cmd_genenv = GENENV $@
> -cmd_genenv = $(OBJCOPY) --dump-section .rodata.default_environment=$@ env/common.o; \
> - sed --in-place -e 's/\x00/\x0A/g' $@; sed --in-place -e '/^\s*$$/d' $@; \
> - sort --field-separator== -k1,1 --stable $@ -o $@
> +cmd_genenv = \
> + scripts/printinitialenv | \
> + sed -e 's/\x00/\x0A/g' -e '/^\s*$$/d' | \
I think that you do not need this sed anymore as you print newline in
host tool.
> + sort --field-separator== -k1,1 --stable -o $@
>
> u-boot-initial-env: u-boot.bin
It is needed to update dependencies for u-boot-initial-env target. Now
it does not depend on u-boot.bin but rather on printinitialenv tool.
> $(call if_changed,genenv)
> diff --git a/scripts/.gitignore b/scripts/.gitignore
> index 08cc824bac3..6068724a0d4 100644
> --- a/scripts/.gitignore
> +++ b/scripts/.gitignore
> @@ -2,3 +2,4 @@
> # Generated files
> #
> bin2c
> +printinitialenv
> diff --git a/scripts/Makefile b/scripts/Makefile
> index 8731e6cecd7..ba993820571 100644
> --- a/scripts/Makefile
> +++ b/scripts/Makefile
> @@ -5,8 +5,13 @@
> # ---------------------------------------------------------------------------
>
> hostprogs-$(CONFIG_BUILD_BIN2C) += bin2c
> +hostprogs-y += printinitialenv
I'm not sure if the scripts/ is the correct directory for the this tool.
Maybe it should be in tools? Lets wait what maintainers or Tom say.
>
> always := $(hostprogs-y)
>
> +HOSTCFLAGS_printinitialenv.o += \
> + $(patsubst -I%,-idirafter%, $(filter -I%, $(UBOOTINCLUDE))) \
> + -DUSE_HOSTCC
> +
> # Let clean descend into subdirs
> subdir- += basic kconfig dtc
> diff --git a/scripts/printinitialenv.c b/scripts/printinitialenv.c
> new file mode 100644
> index 00000000000..c58b234d679
> --- /dev/null
> +++ b/scripts/printinitialenv.c
> @@ -0,0 +1,44 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * (C) Copyright 2022
> + * Max Krummenacher, Toradex
> + *
> + * Snippets taken from tools/env/fw_env.c
> + *
> + * This prints the list of default environment variables as currently
> + * configured.
> + *
> + */
> +
> +#include <stdio.h>
> +
> +/* Pull in the current config to define the default environment */
> +#include <linux/kconfig.h>
> +
> +#ifndef __ASSEMBLY__
> +#define __ASSEMBLY__ /* get only #defines from config.h */
> +#include <config.h>
> +#undef __ASSEMBLY__
> +#else
> +#include <config.h>
> +#endif
> +
> +#define DEFAULT_ENV_INSTANCE_STATIC
> +#include <generated/environment.h>
> +#include <env_default.h>
> +
> +int main(void)
> +{
> + char *env, *nxt;
> +
> + for (env = default_environment; *env; env = nxt + 1) {
> + for (nxt = env; *nxt; ++nxt) {
> + if (nxt >= &default_environment[sizeof(default_environment)]) {
> + fprintf(stderr, "## Error: environment not terminated\n");
> + return -1;
> + }
> + }
> + printf("%s\n", env);
> + }
> + return 0;
> +}
> --
> 2.35.3
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/1] Makefile: rework u-boot-initial-env target
2022-10-28 16:40 ` Pali Rohár
@ 2022-10-31 10:51 ` Max Krummenacher
2022-11-01 14:07 ` Tom Rini
2022-11-01 17:35 ` Pali Rohár
0 siblings, 2 replies; 9+ messages in thread
From: Max Krummenacher @ 2022-10-31 10:51 UTC (permalink / raw)
To: Pali Rohár
Cc: u-boot, Tom Rini, U-Boot STM32, Adam Ford, Patrick DELAUNAY,
Patrice CHOTARD, Max Krummenacher, Heinrich Schuchardt,
Marek Behún, Philippe Reynes, Quentin Schulz, Simon Glass
Hi
On Fri, Oct 28, 2022 at 6:40 PM Pali Rohár <pali@kernel.org> wrote:
>
> Hello! This is really much better solution! Few comments are below.
>
> On Friday 28 October 2022 18:18:49 Max Krummenacher wrote:
> > From: Max Krummenacher <max.krummenacher@toradex.com>
> >
> > With LTO enabled the U-Boot initial environment is no longer stored
> > in an easy accessible section in env/common.o. I.e. the section name
> > changes from build to build, its content maybe compressed and it is
> > annotated with additional data.
> >
> > Drop trying to read the initial env with elf tools from the compiler
> > specific object file in favour of adding and using a host tool with
> > the only functionality of printing the initial env to stdout.
> >
> > See also:
> > https://lore.kernel.org/all/927b122e-1f62-e790-f5ca-30bae4332c77@foss.st.com/
> >
> > Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com>
> >
> > ---
> >
> > Changes in v2:
> > - reworked to build a host tool which prints the configured
> > environment as proposed by Pali Rohár
> > https://lore.kernel.org/u-boot/20221018174827.1393211-1-max.oss.09@gmail.com/
> > - renamed patch, v1 used "Makefile: fix u-boot-initial-env target if lto is enabled"
> >
> > Makefile | 7 ++++---
> > scripts/.gitignore | 1 +
> > scripts/Makefile | 5 +++++
> > scripts/printinitialenv.c | 44 +++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 54 insertions(+), 3 deletions(-)
> > create mode 100644 scripts/printinitialenv.c
> >
> > diff --git a/Makefile b/Makefile
> > index 0f1174718f7..2f97d63c398 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -2442,9 +2442,10 @@ endif
> > $(Q)$(MAKE) -f $(srctree)/scripts/Makefile.modpost
> >
> > quiet_cmd_genenv = GENENV $@
> > -cmd_genenv = $(OBJCOPY) --dump-section .rodata.default_environment=$@ env/common.o; \
> > - sed --in-place -e 's/\x00/\x0A/g' $@; sed --in-place -e '/^\s*$$/d' $@; \
> > - sort --field-separator== -k1,1 --stable $@ -o $@
> > +cmd_genenv = \
> > + scripts/printinitialenv | \
> > + sed -e 's/\x00/\x0A/g' -e '/^\s*$$/d' | \
>
> I think that you do not need this sed anymore as you print newline in
> host tool.
Missed that one, will change in a V3.
>
> > + sort --field-separator== -k1,1 --stable -o $@
> >
> > u-boot-initial-env: u-boot.bin
>
> It is needed to update dependencies for u-boot-initial-env target. Now
> it does not depend on u-boot.bin but rather on printinitialenv tool.
I'm unsure if that is the best way forward. The initial solution would
also not need to depend on u-boot.bin but rather on env/common.o.
I guess that the intention was that the U-Boot binary and the
u-boot-initial-env file should not be out of sync.
Opinions?
>
> > $(call if_changed,genenv)
> > diff --git a/scripts/.gitignore b/scripts/.gitignore
> > index 08cc824bac3..6068724a0d4 100644
> > --- a/scripts/.gitignore
> > +++ b/scripts/.gitignore
> > @@ -2,3 +2,4 @@
> > # Generated files
> > #
> > bin2c
> > +printinitialenv
> > diff --git a/scripts/Makefile b/scripts/Makefile
> > index 8731e6cecd7..ba993820571 100644
> > --- a/scripts/Makefile
> > +++ b/scripts/Makefile
> > @@ -5,8 +5,13 @@
> > # ---------------------------------------------------------------------------
> >
> > hostprogs-$(CONFIG_BUILD_BIN2C) += bin2c
> > +hostprogs-y += printinitialenv
>
> I'm not sure if the scripts/ is the correct directory for the this tool.
> Maybe it should be in tools? Lets wait what maintainers or Tom say.
According to tools/Makefile tools should be used for tools which
are not dependent on any boards. This here is a helper during
the build of a particular U-Boot configuration. That is why I put it
into scripts/.
Max
>
> >
> > always := $(hostprogs-y)
> >
> > +HOSTCFLAGS_printinitialenv.o += \
> > + $(patsubst -I%,-idirafter%, $(filter -I%, $(UBOOTINCLUDE))) \
> > + -DUSE_HOSTCC
> > +
> > # Let clean descend into subdirs
> > subdir- += basic kconfig dtc
> > diff --git a/scripts/printinitialenv.c b/scripts/printinitialenv.c
> > new file mode 100644
> > index 00000000000..c58b234d679
> > --- /dev/null
> > +++ b/scripts/printinitialenv.c
> > @@ -0,0 +1,44 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * (C) Copyright 2022
> > + * Max Krummenacher, Toradex
> > + *
> > + * Snippets taken from tools/env/fw_env.c
> > + *
> > + * This prints the list of default environment variables as currently
> > + * configured.
> > + *
> > + */
> > +
> > +#include <stdio.h>
> > +
> > +/* Pull in the current config to define the default environment */
> > +#include <linux/kconfig.h>
> > +
> > +#ifndef __ASSEMBLY__
> > +#define __ASSEMBLY__ /* get only #defines from config.h */
> > +#include <config.h>
> > +#undef __ASSEMBLY__
> > +#else
> > +#include <config.h>
> > +#endif
> > +
> > +#define DEFAULT_ENV_INSTANCE_STATIC
> > +#include <generated/environment.h>
> > +#include <env_default.h>
> > +
> > +int main(void)
> > +{
> > + char *env, *nxt;
> > +
> > + for (env = default_environment; *env; env = nxt + 1) {
> > + for (nxt = env; *nxt; ++nxt) {
> > + if (nxt >= &default_environment[sizeof(default_environment)]) {
> > + fprintf(stderr, "## Error: environment not terminated\n");
> > + return -1;
> > + }
> > + }
> > + printf("%s\n", env);
> > + }
> > + return 0;
> > +}
> > --
> > 2.35.3
> >
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/1] Makefile: rework u-boot-initial-env target
2022-10-31 10:51 ` Max Krummenacher
@ 2022-11-01 14:07 ` Tom Rini
2022-11-01 17:35 ` Pali Rohár
1 sibling, 0 replies; 9+ messages in thread
From: Tom Rini @ 2022-11-01 14:07 UTC (permalink / raw)
To: Max Krummenacher
Cc: Pali Rohár, u-boot, U-Boot STM32, Adam Ford,
Patrick DELAUNAY, Patrice CHOTARD, Max Krummenacher,
Heinrich Schuchardt, Marek Behún, Philippe Reynes,
Quentin Schulz, Simon Glass
[-- Attachment #1: Type: text/plain, Size: 4272 bytes --]
On Mon, Oct 31, 2022 at 11:51:45AM +0100, Max Krummenacher wrote:
> Hi
>
> On Fri, Oct 28, 2022 at 6:40 PM Pali Rohár <pali@kernel.org> wrote:
> >
> > Hello! This is really much better solution! Few comments are below.
> >
> > On Friday 28 October 2022 18:18:49 Max Krummenacher wrote:
> > > From: Max Krummenacher <max.krummenacher@toradex.com>
> > >
> > > With LTO enabled the U-Boot initial environment is no longer stored
> > > in an easy accessible section in env/common.o. I.e. the section name
> > > changes from build to build, its content maybe compressed and it is
> > > annotated with additional data.
> > >
> > > Drop trying to read the initial env with elf tools from the compiler
> > > specific object file in favour of adding and using a host tool with
> > > the only functionality of printing the initial env to stdout.
> > >
> > > See also:
> > > https://lore.kernel.org/all/927b122e-1f62-e790-f5ca-30bae4332c77@foss.st.com/
> > >
> > > Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com>
> > >
> > > ---
> > >
> > > Changes in v2:
> > > - reworked to build a host tool which prints the configured
> > > environment as proposed by Pali Rohár
> > > https://lore.kernel.org/u-boot/20221018174827.1393211-1-max.oss.09@gmail.com/
> > > - renamed patch, v1 used "Makefile: fix u-boot-initial-env target if lto is enabled"
> > >
> > > Makefile | 7 ++++---
> > > scripts/.gitignore | 1 +
> > > scripts/Makefile | 5 +++++
> > > scripts/printinitialenv.c | 44 +++++++++++++++++++++++++++++++++++++++
> > > 4 files changed, 54 insertions(+), 3 deletions(-)
> > > create mode 100644 scripts/printinitialenv.c
> > >
> > > diff --git a/Makefile b/Makefile
> > > index 0f1174718f7..2f97d63c398 100644
> > > --- a/Makefile
> > > +++ b/Makefile
> > > @@ -2442,9 +2442,10 @@ endif
> > > $(Q)$(MAKE) -f $(srctree)/scripts/Makefile.modpost
> > >
> > > quiet_cmd_genenv = GENENV $@
> > > -cmd_genenv = $(OBJCOPY) --dump-section .rodata.default_environment=$@ env/common.o; \
> > > - sed --in-place -e 's/\x00/\x0A/g' $@; sed --in-place -e '/^\s*$$/d' $@; \
> > > - sort --field-separator== -k1,1 --stable $@ -o $@
> > > +cmd_genenv = \
> > > + scripts/printinitialenv | \
> > > + sed -e 's/\x00/\x0A/g' -e '/^\s*$$/d' | \
> >
> > I think that you do not need this sed anymore as you print newline in
> > host tool.
>
> Missed that one, will change in a V3.
>
> >
> > > + sort --field-separator== -k1,1 --stable -o $@
> > >
> > > u-boot-initial-env: u-boot.bin
> >
> > It is needed to update dependencies for u-boot-initial-env target. Now
> > it does not depend on u-boot.bin but rather on printinitialenv tool.
>
> I'm unsure if that is the best way forward. The initial solution would
> also not need to depend on u-boot.bin but rather on env/common.o.
>
> I guess that the intention was that the U-Boot binary and the
> u-boot-initial-env file should not be out of sync.
>
> Opinions?
>
> >
> > > $(call if_changed,genenv)
> > > diff --git a/scripts/.gitignore b/scripts/.gitignore
> > > index 08cc824bac3..6068724a0d4 100644
> > > --- a/scripts/.gitignore
> > > +++ b/scripts/.gitignore
> > > @@ -2,3 +2,4 @@
> > > # Generated files
> > > #
> > > bin2c
> > > +printinitialenv
> > > diff --git a/scripts/Makefile b/scripts/Makefile
> > > index 8731e6cecd7..ba993820571 100644
> > > --- a/scripts/Makefile
> > > +++ b/scripts/Makefile
> > > @@ -5,8 +5,13 @@
> > > # ---------------------------------------------------------------------------
> > >
> > > hostprogs-$(CONFIG_BUILD_BIN2C) += bin2c
> > > +hostprogs-y += printinitialenv
> >
> > I'm not sure if the scripts/ is the correct directory for the this tool.
> > Maybe it should be in tools? Lets wait what maintainers or Tom say.
>
> According to tools/Makefile tools should be used for tools which
> are not dependent on any boards. This here is a helper during
> the build of a particular U-Boot configuration. That is why I put it
> into scripts/.
I think it should go to tools/ as the comment there isn't an absolute
(other env things are there too).
--
Tom
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/1] Makefile: rework u-boot-initial-env target
2022-10-31 10:51 ` Max Krummenacher
2022-11-01 14:07 ` Tom Rini
@ 2022-11-01 17:35 ` Pali Rohár
2022-11-01 19:48 ` Tom Rini
1 sibling, 1 reply; 9+ messages in thread
From: Pali Rohár @ 2022-11-01 17:35 UTC (permalink / raw)
To: Max Krummenacher
Cc: u-boot, Tom Rini, U-Boot STM32, Adam Ford, Patrick DELAUNAY,
Patrice CHOTARD, Max Krummenacher, Heinrich Schuchardt,
Marek Behún, Philippe Reynes, Quentin Schulz, Simon Glass
On Monday 31 October 2022 11:51:45 Max Krummenacher wrote:
> On Fri, Oct 28, 2022 at 6:40 PM Pali Rohár <pali@kernel.org> wrote:
> > On Friday 28 October 2022 18:18:49 Max Krummenacher wrote:
> > > quiet_cmd_genenv = GENENV $@
> > > -cmd_genenv = $(OBJCOPY) --dump-section .rodata.default_environment=$@ env/common.o; \
> > > - sed --in-place -e 's/\x00/\x0A/g' $@; sed --in-place -e '/^\s*$$/d' $@; \
> > > - sort --field-separator== -k1,1 --stable $@ -o $@
> > > +cmd_genenv = \
> > > + scripts/printinitialenv | \
> > > + sed -e 's/\x00/\x0A/g' -e '/^\s*$$/d' | \
> >
> > I think that you do not need this sed anymore as you print newline in
> > host tool.
>
> Missed that one, will change in a V3.
>
> >
> > > + sort --field-separator== -k1,1 --stable -o $@
> > >
> > > u-boot-initial-env: u-boot.bin
> >
> > It is needed to update dependencies for u-boot-initial-env target. Now
> > it does not depend on u-boot.bin but rather on printinitialenv tool.
>
> I'm unsure if that is the best way forward. The initial solution would
> also not need to depend on u-boot.bin but rather on env/common.o.
Yea, it was wrong too.
> I guess that the intention was that the U-Boot binary and the
> u-boot-initial-env file should not be out of sync.
This is not possible to achieve with Makefile build system and is
probably also against the logic of make to achieve the best parallelism
during build of the project.
In _my_ opinion, Makefile targets should have correct dependencies, on
which they depend. This will prevent circular dependences and also
faster building as make tool can run in parallel as much jobs as
possible -- and do not have to wait until main u-boot.bin is built and
then call other jobs.
Tom, or do you think something different?
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/1] Makefile: rework u-boot-initial-env target
2022-11-01 17:35 ` Pali Rohár
@ 2022-11-01 19:48 ` Tom Rini
2022-11-01 19:55 ` Pali Rohár
0 siblings, 1 reply; 9+ messages in thread
From: Tom Rini @ 2022-11-01 19:48 UTC (permalink / raw)
To: Pali Rohár
Cc: Max Krummenacher, u-boot, U-Boot STM32, Adam Ford,
Patrick DELAUNAY, Patrice CHOTARD, Max Krummenacher,
Heinrich Schuchardt, Marek Behún, Philippe Reynes,
Quentin Schulz, Simon Glass
[-- Attachment #1: Type: text/plain, Size: 2109 bytes --]
On Tue, Nov 01, 2022 at 06:35:04PM +0100, Pali Rohár wrote:
> On Monday 31 October 2022 11:51:45 Max Krummenacher wrote:
> > On Fri, Oct 28, 2022 at 6:40 PM Pali Rohár <pali@kernel.org> wrote:
> > > On Friday 28 October 2022 18:18:49 Max Krummenacher wrote:
> > > > quiet_cmd_genenv = GENENV $@
> > > > -cmd_genenv = $(OBJCOPY) --dump-section .rodata.default_environment=$@ env/common.o; \
> > > > - sed --in-place -e 's/\x00/\x0A/g' $@; sed --in-place -e '/^\s*$$/d' $@; \
> > > > - sort --field-separator== -k1,1 --stable $@ -o $@
> > > > +cmd_genenv = \
> > > > + scripts/printinitialenv | \
> > > > + sed -e 's/\x00/\x0A/g' -e '/^\s*$$/d' | \
> > >
> > > I think that you do not need this sed anymore as you print newline in
> > > host tool.
> >
> > Missed that one, will change in a V3.
> >
> > >
> > > > + sort --field-separator== -k1,1 --stable -o $@
> > > >
> > > > u-boot-initial-env: u-boot.bin
> > >
> > > It is needed to update dependencies for u-boot-initial-env target. Now
> > > it does not depend on u-boot.bin but rather on printinitialenv tool.
> >
> > I'm unsure if that is the best way forward. The initial solution would
> > also not need to depend on u-boot.bin but rather on env/common.o.
>
> Yea, it was wrong too.
>
> > I guess that the intention was that the U-Boot binary and the
> > u-boot-initial-env file should not be out of sync.
>
> This is not possible to achieve with Makefile build system and is
> probably also against the logic of make to achieve the best parallelism
> during build of the project.
>
> In _my_ opinion, Makefile targets should have correct dependencies, on
> which they depend. This will prevent circular dependences and also
> faster building as make tool can run in parallel as much jobs as
> possible -- and do not have to wait until main u-boot.bin is built and
> then call other jobs.
>
> Tom, or do you think something different?
Well, this tool has the same environment requirements as tools/env does,
so we should be able to use the same dependency info?
--
Tom
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/1] Makefile: rework u-boot-initial-env target
2022-11-01 19:48 ` Tom Rini
@ 2022-11-01 19:55 ` Pali Rohár
2022-11-01 20:00 ` Tom Rini
0 siblings, 1 reply; 9+ messages in thread
From: Pali Rohár @ 2022-11-01 19:55 UTC (permalink / raw)
To: Tom Rini
Cc: Max Krummenacher, u-boot, U-Boot STM32, Adam Ford,
Patrick DELAUNAY, Patrice CHOTARD, Max Krummenacher,
Heinrich Schuchardt, Marek Behún, Philippe Reynes,
Quentin Schulz, Simon Glass
On Tuesday 01 November 2022 15:48:34 Tom Rini wrote:
> On Tue, Nov 01, 2022 at 06:35:04PM +0100, Pali Rohár wrote:
> > On Monday 31 October 2022 11:51:45 Max Krummenacher wrote:
> > > On Fri, Oct 28, 2022 at 6:40 PM Pali Rohár <pali@kernel.org> wrote:
> > > > On Friday 28 October 2022 18:18:49 Max Krummenacher wrote:
> > > > > quiet_cmd_genenv = GENENV $@
> > > > > -cmd_genenv = $(OBJCOPY) --dump-section .rodata.default_environment=$@ env/common.o; \
> > > > > - sed --in-place -e 's/\x00/\x0A/g' $@; sed --in-place -e '/^\s*$$/d' $@; \
> > > > > - sort --field-separator== -k1,1 --stable $@ -o $@
> > > > > +cmd_genenv = \
> > > > > + scripts/printinitialenv | \
> > > > > + sed -e 's/\x00/\x0A/g' -e '/^\s*$$/d' | \
> > > >
> > > > I think that you do not need this sed anymore as you print newline in
> > > > host tool.
> > >
> > > Missed that one, will change in a V3.
> > >
> > > >
> > > > > + sort --field-separator== -k1,1 --stable -o $@
> > > > >
> > > > > u-boot-initial-env: u-boot.bin
> > > >
> > > > It is needed to update dependencies for u-boot-initial-env target. Now
> > > > it does not depend on u-boot.bin but rather on printinitialenv tool.
> > >
> > > I'm unsure if that is the best way forward. The initial solution would
> > > also not need to depend on u-boot.bin but rather on env/common.o.
> >
> > Yea, it was wrong too.
> >
> > > I guess that the intention was that the U-Boot binary and the
> > > u-boot-initial-env file should not be out of sync.
> >
> > This is not possible to achieve with Makefile build system and is
> > probably also against the logic of make to achieve the best parallelism
> > during build of the project.
> >
> > In _my_ opinion, Makefile targets should have correct dependencies, on
> > which they depend. This will prevent circular dependences and also
> > faster building as make tool can run in parallel as much jobs as
> > possible -- and do not have to wait until main u-boot.bin is built and
> > then call other jobs.
> >
> > Tom, or do you think something different?
>
> Well, this tool has the same environment requirements as tools/env does,
> so we should be able to use the same dependency info?
Yes, build tool is _able_ to use it. (I think we all agree on this
ability of make)
But the question is if it _should_ be used. Because Max wrote some
arguments why not to use this env dependency info.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/1] Makefile: rework u-boot-initial-env target
2022-11-01 19:55 ` Pali Rohár
@ 2022-11-01 20:00 ` Tom Rini
0 siblings, 0 replies; 9+ messages in thread
From: Tom Rini @ 2022-11-01 20:00 UTC (permalink / raw)
To: Pali Rohár
Cc: Max Krummenacher, u-boot, U-Boot STM32, Adam Ford,
Patrick DELAUNAY, Patrice CHOTARD, Max Krummenacher,
Heinrich Schuchardt, Marek Behún, Philippe Reynes,
Quentin Schulz, Simon Glass
[-- Attachment #1: Type: text/plain, Size: 2916 bytes --]
On Tue, Nov 01, 2022 at 08:55:09PM +0100, Pali Rohár wrote:
> On Tuesday 01 November 2022 15:48:34 Tom Rini wrote:
> > On Tue, Nov 01, 2022 at 06:35:04PM +0100, Pali Rohár wrote:
> > > On Monday 31 October 2022 11:51:45 Max Krummenacher wrote:
> > > > On Fri, Oct 28, 2022 at 6:40 PM Pali Rohár <pali@kernel.org> wrote:
> > > > > On Friday 28 October 2022 18:18:49 Max Krummenacher wrote:
> > > > > > quiet_cmd_genenv = GENENV $@
> > > > > > -cmd_genenv = $(OBJCOPY) --dump-section .rodata.default_environment=$@ env/common.o; \
> > > > > > - sed --in-place -e 's/\x00/\x0A/g' $@; sed --in-place -e '/^\s*$$/d' $@; \
> > > > > > - sort --field-separator== -k1,1 --stable $@ -o $@
> > > > > > +cmd_genenv = \
> > > > > > + scripts/printinitialenv | \
> > > > > > + sed -e 's/\x00/\x0A/g' -e '/^\s*$$/d' | \
> > > > >
> > > > > I think that you do not need this sed anymore as you print newline in
> > > > > host tool.
> > > >
> > > > Missed that one, will change in a V3.
> > > >
> > > > >
> > > > > > + sort --field-separator== -k1,1 --stable -o $@
> > > > > >
> > > > > > u-boot-initial-env: u-boot.bin
> > > > >
> > > > > It is needed to update dependencies for u-boot-initial-env target. Now
> > > > > it does not depend on u-boot.bin but rather on printinitialenv tool.
> > > >
> > > > I'm unsure if that is the best way forward. The initial solution would
> > > > also not need to depend on u-boot.bin but rather on env/common.o.
> > >
> > > Yea, it was wrong too.
> > >
> > > > I guess that the intention was that the U-Boot binary and the
> > > > u-boot-initial-env file should not be out of sync.
> > >
> > > This is not possible to achieve with Makefile build system and is
> > > probably also against the logic of make to achieve the best parallelism
> > > during build of the project.
> > >
> > > In _my_ opinion, Makefile targets should have correct dependencies, on
> > > which they depend. This will prevent circular dependences and also
> > > faster building as make tool can run in parallel as much jobs as
> > > possible -- and do not have to wait until main u-boot.bin is built and
> > > then call other jobs.
> > >
> > > Tom, or do you think something different?
> >
> > Well, this tool has the same environment requirements as tools/env does,
> > so we should be able to use the same dependency info?
>
> Yes, build tool is _able_ to use it. (I think we all agree on this
> ability of make)
>
> But the question is if it _should_ be used. Because Max wrote some
> arguments why not to use this env dependency info.
Er, I'm lost I guess, sorry. Yes, it should not depend on env/common.o
nor u-boot.bin. I guess looking again, we can't borrow just the envtools
rules as we also would need to update the section that adds
$(defaultenv_h) to envtools would also need to be done for this tool.
--
Tom
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-11-01 20:00 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-10-28 16:18 [PATCH v2 0/1] Makefile: rework u-boot-initial-env target Max Krummenacher
2022-10-28 16:18 ` [PATCH v2 1/1] " Max Krummenacher
2022-10-28 16:40 ` Pali Rohár
2022-10-31 10:51 ` Max Krummenacher
2022-11-01 14:07 ` Tom Rini
2022-11-01 17:35 ` Pali Rohár
2022-11-01 19:48 ` Tom Rini
2022-11-01 19:55 ` Pali Rohár
2022-11-01 20:00 ` Tom Rini
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox