| Message ID | 20180124095559.28317-1-rasmus.villemoes@prevas.dk |
|---|---|
| State | RFC |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [U-Boot,RFC] Allow providing default environment from file | expand |
Hi Rasmus, > It is sometimes useful to be able to define the entire default > environment in an external file. There is already available script for extracting the environment. Please look into: ./scripts/get_default_envs.sh Maybe you can reuse it in this patch? > This implements a Kconfig option for > allowing that. > > It is somewhat annoying to have two visible Kconfig options; it would > probably be more user-friendly to just have the string option (with > empty string obviously meaning not to use this feature). But then we'd > also need a hidden CONFIG that we can use in the #ifdef in > env_default.h, and I don't think one can set a def_bool based on > whether a string-valued config is empty or not. > > I've tried to make the accepted format the same as the one the > mkenvimage tool accepts. I have no idea how portable the sed script > implementing the "allow embedded newlines in values" is. Nor do I know > if one can expect xxd to be available. > > Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> > --- > Makefile | 16 ++++++++++++++++ > env/Kconfig | 18 ++++++++++++++++++ > include/env_default.h | 4 ++++ > 3 files changed, 38 insertions(+) > > diff --git a/Makefile b/Makefile > index 4981a2ed6f..e5ba5213fd 100644 > --- a/Makefile > +++ b/Makefile > @@ -423,6 +423,7 @@ endif > > version_h := include/generated/version_autogenerated.h > timestamp_h := include/generated/timestamp_autogenerated.h > +defaultenv_h := include/generated/defaultenv_autogenerated.h > > no-dot-config-targets := clean clobber mrproper distclean \ > help %docs check% coccicheck \ > @@ -1366,6 +1367,10 @@ ifeq ($(wildcard $(LDSCRIPT)),) > @/bin/false > endif > > +ifeq ($(CONFIG_DEFAULT_ENV_FROM_FILE),y) > +prepare1: $(defaultenv_h) > +endif > + > archprepare: prepare1 scripts_basic > > prepare0: archprepare FORCE > @@ -1413,12 +1418,23 @@ define filechk_timestamp.h > fi) > endef > > +define filechk_defaultenv.h > + (grep -v '^#' | \ > + grep -v '^$$' | \ > + tr '\n' '\0' | \ > + sed -e 's/\\\x0/\n/' | \ > + xxd -i ; echo ", 0x00" ; ) > +endef > + > $(version_h): include/config/uboot.release FORCE > $(call filechk,version.h) > > $(timestamp_h): $(srctree)/Makefile FORCE > $(call filechk,timestamp.h) > > +$(defaultenv_h): $(CONFIG_DEFAULT_ENV_FILE:"%"=%) FORCE > + $(call filechk,defaultenv.h) > + > # > --------------------------------------------------------------------------- > quiet_cmd_cpp_lds = LDS $@ cmd_cpp_lds = $(CPP) > -Wp,-MD,$(depfile) $(cpp_flags) $(LDPPFLAGS) \ diff --git > a/env/Kconfig b/env/Kconfig index a24370786b..1baebd743b 100644 > --- a/env/Kconfig > +++ b/env/Kconfig > @@ -482,4 +482,22 @@ config ENV_SIZE > > endif > > +config DEFAULT_ENV_FROM_FILE > + bool "Create default environment from file" > + help > + Normally, the default environment is automatically > generated > + based on the settings of various CONFIG_* options, as well > + as the CONFIG_EXTRA_ENV_SETTINGS. By selecting this option, > + you can instead define the entire default environment in an > + external file. > + > +config DEFAULT_ENV_FILE > + string "Path to default environment file" > + depends on DEFAULT_ENV_FROM_FILE > + help > + The path containing the default environment. The format is > + the same as accepted by the mkenvimage tool: lines > + containing key=value pairs, blank lines and lines beginning > + with # are ignored. > + > endmenu > diff --git a/include/env_default.h b/include/env_default.h > index b574345af2..656d202cc7 100644 > --- a/include/env_default.h > +++ b/include/env_default.h > @@ -22,6 +22,7 @@ static char default_environment[] = { > #else > const uchar default_environment[] = { > #endif > +#ifndef CONFIG_DEFAULT_ENV_FROM_FILE > #ifdef CONFIG_ENV_CALLBACK_LIST_DEFAULT > ENV_CALLBACK_VAR "=" CONFIG_ENV_CALLBACK_LIST_DEFAULT "\0" > #endif > @@ -108,6 +109,9 @@ const uchar default_environment[] = { > CONFIG_EXTRA_ENV_SETTINGS > #endif > "\0" > +#else /* CONFIG_DEFAULT_ENV_FROM_FILE */ > +#include "generated/defaultenv_autogenerated.h" > +#endif > #ifdef DEFAULT_ENV_INSTANCE_EMBEDDED > } > #endif Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
On 2018-01-25 10:30, Lukasz Majewski wrote: > Hi Rasmus, > >> It is sometimes useful to be able to define the entire default >> environment in an external file. > > There is already available script for extracting the environment. > > Please look into: > ./scripts/get_default_envs.sh > > Maybe you can reuse it in this patch? I'm sorry, but I don't see what I could use that for. It seems to do the opposite of what I want, namely extract the default environment and store it in a plain-text file. I want to provide a plain-text file to define the default environment. It's quite likely that that script can be useful for generating a sketch for the external file (i.e., build a U-boot as "usual" with default environment built from various config options etc. etc., then hand-edit that file to remove redundant stuff and add the things one needs). The thing is, having the default environment in an external file makes it much easier to put it under version control than having to maintain a branch inside the U-boot repo just to tweak CONFIG_EXTRA_ENV_SETTINGS. Rasmus
Hi Rasmus, > On 2018-01-25 10:30, Lukasz Majewski wrote: > > Hi Rasmus, > > > >> It is sometimes useful to be able to define the entire default > >> environment in an external file. > > > > There is already available script for extracting the environment. > > > > Please look into: > > ./scripts/get_default_envs.sh > > > > Maybe you can reuse it in this patch? > > I'm sorry, but I don't see what I could use that for. It seems to do > the opposite of what I want, Sorry. I have had misunderstood the patch description. > namely extract the default environment > and store it in a plain-text file. I want to provide a plain-text > file to define the default environment. Ok. Then your patch seems perfectly valid. > > It's quite likely that that script can be useful for generating a > sketch for the external file (i.e., build a U-boot as "usual" with > default environment built from various config options etc. etc., then > hand-edit that file to remove redundant stuff and add the things one > needs). The thing is, having the default environment in an external > file makes it much easier to put it under version control than having > to maintain a branch inside the U-boot repo just to tweak > CONFIG_EXTRA_ENV_SETTINGS. > > Rasmus Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
On 24-01-2018 10:55, Rasmus Villemoes wrote: > It is sometimes useful to be able to define the entire default > environment in an external file. This implements a Kconfig option for > allowing that. > > It is somewhat annoying to have two visible Kconfig options; it would > probably be more user-friendly to just have the string option (with > empty string obviously meaning not to use this feature). But then we'd > also need a hidden CONFIG that we can use in the #ifdef in > env_default.h, and I don't think one can set a def_bool based on > whether a string-valued config is empty or not. > > I've tried to make the accepted format the same as the one the > mkenvimage tool accepts. I have no idea how portable the sed script > implementing the "allow embedded newlines in values" is. Nor do I know > if one can expect xxd to be available. > > Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> Tested-by: Sean Nyekjaer <sean.nyekjaer@prevas.dk> > --- > Makefile | 16 ++++++++++++++++ > env/Kconfig | 18 ++++++++++++++++++ > include/env_default.h | 4 ++++ > 3 files changed, 38 insertions(+) > > diff --git a/Makefile b/Makefile > index 4981a2ed6f..e5ba5213fd 100644 > --- a/Makefile > +++ b/Makefile > @@ -423,6 +423,7 @@ endif > > version_h := include/generated/version_autogenerated.h > timestamp_h := include/generated/timestamp_autogenerated.h > +defaultenv_h := include/generated/defaultenv_autogenerated.h > > no-dot-config-targets := clean clobber mrproper distclean \ > help %docs check% coccicheck \ > @@ -1366,6 +1367,10 @@ ifeq ($(wildcard $(LDSCRIPT)),) > @/bin/false > endif > > +ifeq ($(CONFIG_DEFAULT_ENV_FROM_FILE),y) > +prepare1: $(defaultenv_h) > +endif > + > archprepare: prepare1 scripts_basic > > prepare0: archprepare FORCE > @@ -1413,12 +1418,23 @@ define filechk_timestamp.h > fi) > endef > > +define filechk_defaultenv.h > + (grep -v '^#' | \ > + grep -v '^$$' | \ > + tr '\n' '\0' | \ > + sed -e 's/\\\x0/\n/' | \ > + xxd -i ; echo ", 0x00" ; ) > +endef > + > $(version_h): include/config/uboot.release FORCE > $(call filechk,version.h) > > $(timestamp_h): $(srctree)/Makefile FORCE > $(call filechk,timestamp.h) > > +$(defaultenv_h): $(CONFIG_DEFAULT_ENV_FILE:"%"=%) FORCE > + $(call filechk,defaultenv.h) > + > # --------------------------------------------------------------------------- > quiet_cmd_cpp_lds = LDS $@ > cmd_cpp_lds = $(CPP) -Wp,-MD,$(depfile) $(cpp_flags) $(LDPPFLAGS) \ > diff --git a/env/Kconfig b/env/Kconfig > index a24370786b..1baebd743b 100644 > --- a/env/Kconfig > +++ b/env/Kconfig > @@ -482,4 +482,22 @@ config ENV_SIZE > > endif > > +config DEFAULT_ENV_FROM_FILE > + bool "Create default environment from file" > + help > + Normally, the default environment is automatically generated > + based on the settings of various CONFIG_* options, as well > + as the CONFIG_EXTRA_ENV_SETTINGS. By selecting this option, > + you can instead define the entire default environment in an > + external file. > + > +config DEFAULT_ENV_FILE > + string "Path to default environment file" > + depends on DEFAULT_ENV_FROM_FILE > + help > + The path containing the default environment. The format is > + the same as accepted by the mkenvimage tool: lines > + containing key=value pairs, blank lines and lines beginning > + with # are ignored. > + > endmenu > diff --git a/include/env_default.h b/include/env_default.h > index b574345af2..656d202cc7 100644 > --- a/include/env_default.h > +++ b/include/env_default.h > @@ -22,6 +22,7 @@ static char default_environment[] = { > #else > const uchar default_environment[] = { > #endif > +#ifndef CONFIG_DEFAULT_ENV_FROM_FILE > #ifdef CONFIG_ENV_CALLBACK_LIST_DEFAULT > ENV_CALLBACK_VAR "=" CONFIG_ENV_CALLBACK_LIST_DEFAULT "\0" > #endif > @@ -108,6 +109,9 @@ const uchar default_environment[] = { > CONFIG_EXTRA_ENV_SETTINGS > #endif > "\0" > +#else /* CONFIG_DEFAULT_ENV_FROM_FILE */ > +#include "generated/defaultenv_autogenerated.h" > +#endif > #ifdef DEFAULT_ENV_INSTANCE_EMBEDDED > } > #endif
On 2018-01-24 10:55, Rasmus Villemoes wrote: > It is sometimes useful to be able to define the entire default > environment in an external file. This implements a Kconfig option for > allowing that. ping
On 2018-02-02 21:15, Rasmus Villemoes wrote: > On 2018-01-24 10:55, Rasmus Villemoes wrote: >> It is sometimes useful to be able to define the entire default >> environment in an external file. This implements a Kconfig option for >> allowing that. > ping Hi Tom Will you please take a look a this? /Sean
On Wed, Jan 24, 2018 at 10:55:59AM +0100, Rasmus Villemoes wrote: > It is sometimes useful to be able to define the entire default > environment in an external file. This implements a Kconfig option for > allowing that. > > It is somewhat annoying to have two visible Kconfig options; it would > probably be more user-friendly to just have the string option (with > empty string obviously meaning not to use this feature). But then we'd > also need a hidden CONFIG that we can use in the #ifdef in > env_default.h, and I don't think one can set a def_bool based on > whether a string-valued config is empty or not. > > I've tried to make the accepted format the same as the one the > mkenvimage tool accepts. I have no idea how portable the sed script > implementing the "allow embedded newlines in values" is. Nor do I know > if one can expect xxd to be available. > > Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> > --- > Makefile | 16 ++++++++++++++++ > env/Kconfig | 18 ++++++++++++++++++ > include/env_default.h | 4 ++++ > 3 files changed, 38 insertions(+) Conceptually, this is fine. But can you please re-word the commit message and put some of the commentary below the --- ? Also, in general we do a pair of CONFIG_USE_xxx and CONFIG_xxx, so you might need to re-word the rest of the option name a bit too to match that. Thanks!
diff --git a/Makefile b/Makefile index 4981a2ed6f..e5ba5213fd 100644 --- a/Makefile +++ b/Makefile @@ -423,6 +423,7 @@ endif version_h := include/generated/version_autogenerated.h timestamp_h := include/generated/timestamp_autogenerated.h +defaultenv_h := include/generated/defaultenv_autogenerated.h no-dot-config-targets := clean clobber mrproper distclean \ help %docs check% coccicheck \ @@ -1366,6 +1367,10 @@ ifeq ($(wildcard $(LDSCRIPT)),) @/bin/false endif +ifeq ($(CONFIG_DEFAULT_ENV_FROM_FILE),y) +prepare1: $(defaultenv_h) +endif + archprepare: prepare1 scripts_basic prepare0: archprepare FORCE @@ -1413,12 +1418,23 @@ define filechk_timestamp.h fi) endef +define filechk_defaultenv.h + (grep -v '^#' | \ + grep -v '^$$' | \ + tr '\n' '\0' | \ + sed -e 's/\\\x0/\n/' | \ + xxd -i ; echo ", 0x00" ; ) +endef + $(version_h): include/config/uboot.release FORCE $(call filechk,version.h) $(timestamp_h): $(srctree)/Makefile FORCE $(call filechk,timestamp.h) +$(defaultenv_h): $(CONFIG_DEFAULT_ENV_FILE:"%"=%) FORCE + $(call filechk,defaultenv.h) + # --------------------------------------------------------------------------- quiet_cmd_cpp_lds = LDS $@ cmd_cpp_lds = $(CPP) -Wp,-MD,$(depfile) $(cpp_flags) $(LDPPFLAGS) \ diff --git a/env/Kconfig b/env/Kconfig index a24370786b..1baebd743b 100644 --- a/env/Kconfig +++ b/env/Kconfig @@ -482,4 +482,22 @@ config ENV_SIZE endif +config DEFAULT_ENV_FROM_FILE + bool "Create default environment from file" + help + Normally, the default environment is automatically generated + based on the settings of various CONFIG_* options, as well + as the CONFIG_EXTRA_ENV_SETTINGS. By selecting this option, + you can instead define the entire default environment in an + external file. + +config DEFAULT_ENV_FILE + string "Path to default environment file" + depends on DEFAULT_ENV_FROM_FILE + help + The path containing the default environment. The format is + the same as accepted by the mkenvimage tool: lines + containing key=value pairs, blank lines and lines beginning + with # are ignored. + endmenu diff --git a/include/env_default.h b/include/env_default.h index b574345af2..656d202cc7 100644 --- a/include/env_default.h +++ b/include/env_default.h @@ -22,6 +22,7 @@ static char default_environment[] = { #else const uchar default_environment[] = { #endif +#ifndef CONFIG_DEFAULT_ENV_FROM_FILE #ifdef CONFIG_ENV_CALLBACK_LIST_DEFAULT ENV_CALLBACK_VAR "=" CONFIG_ENV_CALLBACK_LIST_DEFAULT "\0" #endif @@ -108,6 +109,9 @@ const uchar default_environment[] = { CONFIG_EXTRA_ENV_SETTINGS #endif "\0" +#else /* CONFIG_DEFAULT_ENV_FROM_FILE */ +#include "generated/defaultenv_autogenerated.h" +#endif #ifdef DEFAULT_ENV_INSTANCE_EMBEDDED } #endif
It is sometimes useful to be able to define the entire default environment in an external file. This implements a Kconfig option for allowing that. It is somewhat annoying to have two visible Kconfig options; it would probably be more user-friendly to just have the string option (with empty string obviously meaning not to use this feature). But then we'd also need a hidden CONFIG that we can use in the #ifdef in env_default.h, and I don't think one can set a def_bool based on whether a string-valued config is empty or not. I've tried to make the accepted format the same as the one the mkenvimage tool accepts. I have no idea how portable the sed script implementing the "allow embedded newlines in values" is. Nor do I know if one can expect xxd to be available. Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> --- Makefile | 16 ++++++++++++++++ env/Kconfig | 18 ++++++++++++++++++ include/env_default.h | 4 ++++ 3 files changed, 38 insertions(+)