Hi Wolfgang,

On Mon, Oct 28, 2013 at 3:16 PM, Wolfgang Denk wd@denx.de wrote:

Dear Simon,

In message <CAPnjgZ0+35Zd6_o=G0=

G-YL_mR-GiTW+MeYqK7BSxU_nVkvv1w@mail.gmail.com> you wrote:

...

...

I can see why you like such a "beautified" text format, but I don;t think you are doing anybody a favour here. Can we not rather use _exactly_ the same text format as U-Boot uses with it's import / export commands?

That would be nice, but how to we handle newlines? Some scripts are quite long. Do we need to put \ at the end of every line? That feels a bit painful to me.

Agreed. But that's what "env export -t" creates anyway (and I cannot think of a much better presentation of multiline variable content if you don't want to run in ambiguities).

I believe the ambiguities are pretty rare. And people tend to indent multi-line scripts anyway, right?

...

Also how do we handle #define? Without it I don't think this feature is useful, since the existing build system often sticks CONFIG variables into the environment.

I don't understand this question here. I agree that we should be able to run the file through the preprocessor such that it can resolve such macro definitions. But this should be independent of the actual text format, or am I missing something?

Yes we can, agreed. I was thinking you would not want the preprocessor since 'env export -t' doesn't understand it.

...

I believe that is already possible - you should be able to take the output of 'env export -t' and put it in the .env file. I have not tried it though.

Hm... I really think we should agree on a common format her e- one that is easy to work with on both sides.

Agreed.

...

...

Example (using notation as exported by U-Boot using "env export -t"):

    foo=setenv xxx\
    one=1;setenv yyy\
    two=2;setenv zzz\
    three=3

...

• # Print out all the variables
  

• for (var in vars) {
  

•         print var "=" vars[var] "\\0";
  

• }
  

I think it should not be difficult to find examples that would result incorrect output.

I will take a look at this - here it is the \ at the end of line which needs to be handled.

Well, I can't see how you avoid such ambiguities in your proposed format. You need a clear termination for the value of a variable. If it is spread across multiple lines, you have to find a way to mark continuation lines. The "accepted standard" way to do so is by using a backslash at the end of the line. It appears you want to use indentation instead - this prevents users from using a free text format, and quickly becomes messy. And it is incompatible to (current) U-Boot code.

Indentation is pretty normal in code, so I don't feel embarrassed about asking for it. I think the primary problem with my feature is that it is different from 'env export -t', and thus potentially introduces another format. My argument against that interpretation is that I am in fact replacing a C header file definition with a text file, so perhaps I'm not really increasing the number of formats?

...

...

I guess this needs more work - but then - why define a new format at all? Why not use what U-Boot uses itself?

See above, would be good to resolve this issue before going any further.

"Above" I cannot see any explanation why you define a new format except for the fact that the backslash as marker for continuation lines is "painful". I'm open on discussing a new format for text representation (which then also should be used by "env print" and "env export -t" ?). But I'd like to see a description of that format first (and not just a few examples I can guess from).

From my commit message:

At present U-Boot environment variables, and thus scripts, are defined

by CONFIG_EXTRA_ENV_SETTINGS. It is painful to add large amounts of text to this file and dealing with quoting and newlines is harder than it should be. It would be better if we could just type the script into a text file and have it included by U-Boot.

Well yes I could adjust 'env export -t' to use the same format - is that a good idea, or not? I can see down-sides. We can of course convert existing files brought in by 'env import -t' (by making the import flexible) but I worry that there might be external tools that users have which expect the format to be a certain way.

I agree creating a new format is not ideal - it's just that the existing C header format is so painful...

Regards, Simon

Hello,

On Sat, Oct 26, 2013 at 3:01 AM, Simon Glass sjg@chromium.org wrote:

At present U-Boot environment variables, and thus scripts, are defined by CONFIG_EXTRA_ENV_SETTINGS. It is painful to add large amounts of text to this file and dealing with quoting and newlines is harder than it should be. It would be better if we could just type the script into a text file and have it included by U-Boot.

Add a feature that brings in a .env file associated with the board config, if present. To use it, create a file in a board/<vendor>/env directory called <board>.env (or common.env if you want the same environment for all boards).

The environment variables should be of the form "var=value". Values can extend to multiple lines. See the README under 'Environment Variables:' for more information and an example.

Comments are not permitted in the environment with this commit.

Signed-off-by: Simon Glass sjg@chromium.org

I think people (or myself) are misunderstanding what this patch does.

My understand is:

- it allow moving environment setting to a .env file - it needs to include the environment at /build/ time

This does improve a lot the current status (and I really appreciate it); one missing thing (or I missed it completely) is a way to:

- build u-boot (binary) - generate a binary version of the environment - glue both together in a way the new environment /replaces/ the default environment originally written inside u-boot (binary)

Am I missing something?

On 10/25/2013 11:01 PM, Simon Glass wrote:

This seems more intuitive that the current #define way of doing things. The resulting code is shorter, avoids the quoting and line continuation pain, and also improves the clumsy way that stdio variables are created:

#ifdef CONFIG_VIDEO_TEGRA #define STDOUT_LCD ",lcd" #else #define STDOUT_LCD "" #endif

... #define TEGRA_DEVICE_SETTINGS \ "stdout=serial" STDOUT_LCD "\0" \ ...

The MEM_LAYOUT_ENV_SETTINGS variable is left in the header files, since it depends on the SOC type and we probably don't want to add .emv files for each board at this stage.

I guess I'm fine with this as long as e.g. board/compulab/env/trimslice.env can contain:

#include "../../../board/nvidia/env/common.env"

... or perhaps the include path is set up to include board/nvidia/env already, so it could just contain:

#include <common-nvidia.env>

diff --git a/board/nvidia/env/common.env b/board/nvidia/env/common.env

+bootcmd_mmc0=setenv devnum 0; run mmc_boot +bootcmd_mmc1=setenv devnum 1; run mmc_booxt +boot_targets+= mmc1 mmc0

I still don't see why = needs no space before/after, but += needs no space before, but a space after. That simply looks like a typo to me, and I'd be inclined to fix it were I editing this file. If a sed script can't handle more flexible white-space, perhaps use Python or perhaps Perl instead?

On 10/28/2013 02:34 PM, Simon Glass wrote:

Hi Stephen,

On Mon, Oct 28, 2013 at 1:59 PM, Stephen Warren swarren@wwwdotorg.org wrote:

...

On 10/25/2013 11:01 PM, Simon Glass wrote:

...

This seems more intuitive that the current #define way of doing things. The resulting code is shorter, avoids the quoting and line continuation pain, and also improves the clumsy way that stdio variables are created:

#ifdef CONFIG_VIDEO_TEGRA #define STDOUT_LCD ",lcd" #else #define STDOUT_LCD "" #endif

... #define TEGRA_DEVICE_SETTINGS \ "stdout=serial" STDOUT_LCD "\0" \ ...

The MEM_LAYOUT_ENV_SETTINGS variable is left in the header files, since it depends on the SOC type and we probably don't want to add .emv files for each board at this stage.

I guess I'm fine with this as long as e.g. board/compulab/env/trimslice.env can contain:

#include "../../../board/nvidia/env/common.env"

... or perhaps the include path is set up to include board/nvidia/env already, so it could just contain:

#include <common-nvidia.env>

...

diff --git a/board/nvidia/env/common.env b/board/nvidia/env/common.env

...

+bootcmd_mmc0=setenv devnum 0; run mmc_boot +bootcmd_mmc1=setenv devnum 1; run mmc_booxt +boot_targets+= mmc1 mmc0

I still don't see why = needs no space before/after, but += needs no space before, but a space after. That simply looks like a typo to me, and I'd be inclined to fix it were I editing this file. If a sed script can't handle more flexible white-space, perhaps use Python or perhaps Perl instead?

The old code was similar, in that it had a space after the quote.

We need the string to contain "mmc0 mmc1 usb0 dhcp" or perhaps "mmc0 mmc1". I chose to add a space at the start of each string, but certainly we need a space somewhere, or we get "mmc0mmc1usb0dhcp".

Oh, I see. I thought the space was part of the += syntax, not the value. Perhaps to make that more obvious, you could allow:

# No space added to value var+=value var+= value var +=value var += value

var+=value1 value2 var+= value1 value2 var +=value1 value2 var += value1 value2

var+="value1 value2" var+= "value1 value2" var +="value1 value2" var += "value1 value2"

# One space included at start of addition to value var+=" value1 value2" var+= " value1 value2" var +=" value1 value2" var += " value1 value2"

On 10/28/2013 02:50 PM, Simon Glass wrote:

Hi Stephen,

On Mon, Oct 28, 2013 at 2:41 PM, Stephen Warren swarren@wwwdotorg.org wrote:

...

On 10/28/2013 02:34 PM, Simon Glass wrote:

...

Hi Stephen,

On Mon, Oct 28, 2013 at 1:59 PM, Stephen Warren swarren@wwwdotorg.org wrote:

...

On 10/25/2013 11:01 PM, Simon Glass wrote:

...

This seems more intuitive that the current #define way of doing things. The resulting code is shorter, avoids the quoting and line continuation pain, and also improves the clumsy way that stdio variables are created:

...

...

...

...

diff --git a/board/nvidia/env/common.env b/board/nvidia/env/common.env

...

+bootcmd_mmc0=setenv devnum 0; run mmc_boot +bootcmd_mmc1=setenv devnum 1; run mmc_booxt +boot_targets+= mmc1 mmc0

I still don't see why = needs no space before/after, but += needs no space before, but a space after. That simply looks like a typo to me, and I'd be inclined to fix it were I editing this file. If a sed script can't handle more flexible white-space, perhaps use Python or perhaps Perl instead?

The old code was similar, in that it had a space after the quote.

We need the string to contain "mmc0 mmc1 usb0 dhcp" or perhaps "mmc0 mmc1". I chose to add a space at the start of each string, but certainly we need a space somewhere, or we get "mmc0mmc1usb0dhcp".

Oh, I see. I thought the space was part of the += syntax, not the value. Perhaps to make that more obvious, you could allow:

# No space added to value var+=value

...

...

var += "value1 value2"

# One space included at start of addition to value var+=" value1 value2" var+= " value1 value2" var +=" value1 value2" var += " value1 value2"

I was deliberately trying to avoid using quotes, since then it is really hard when you actually mean 'quote'.

Hmm. On the other hand, quoting is standard syntax in any scripting language.

For example at present you can put this in an env script at present, but how would you do it if quotes are special?

Just escape it; " goes around the string and " or "" within the string. This seems pretty common...

On 10/28/2013 04:15 PM, Simon Glass wrote:

Hi Stephen,

On Mon, Oct 28, 2013 at 3:20 PM, Stephen Warren swarren@wwwdotorg.org wrote:

...

On 10/28/2013 02:50 PM, Simon Glass wrote:

...

Hi Stephen,

On Mon, Oct 28, 2013 at 2:41 PM, Stephen Warren swarren@wwwdotorg.org wrote:

...

On 10/28/2013 02:34 PM, Simon Glass wrote:

...

Hi Stephen,

On Mon, Oct 28, 2013 at 1:59 PM, Stephen Warren swarren@wwwdotorg.org wrote:

...

On 10/25/2013 11:01 PM, Simon Glass wrote: > This seems more intuitive that the current #define way of doing things. > The resulting code is shorter, avoids the quoting and line continuation > pain, and also improves the clumsy way that stdio variables are created:

...

...

...

...

> diff --git a/board/nvidia/env/common.env b/board/nvidia/env/common.env

> +bootcmd_mmc0=setenv devnum 0; run mmc_boot > +bootcmd_mmc1=setenv devnum 1; run mmc_booxt > +boot_targets+= mmc1 mmc0

I still don't see why = needs no space before/after, but += needs no space before, but a space after. That simply looks like a typo to me, and I'd be inclined to fix it were I editing this file. If a sed script can't handle more flexible white-space, perhaps use Python or perhaps Perl instead?

The old code was similar, in that it had a space after the quote.

We need the string to contain "mmc0 mmc1 usb0 dhcp" or perhaps "mmc0 mmc1". I chose to add a space at the start of each string, but certainly we need a space somewhere, or we get "mmc0mmc1usb0dhcp".

Oh, I see. I thought the space was part of the += syntax, not the value. Perhaps to make that more obvious, you could allow:

# No space added to value var+=value

...

...

...

var += "value1 value2"

# One space included at start of addition to value var+=" value1 value2" var+= " value1 value2" var +=" value1 value2" var += " value1 value2"

I was deliberately trying to avoid using quotes, since then it is really hard when you actually mean 'quote'.

Hmm. On the other hand, quoting is standard syntax in any scripting language.

...

For example at present you can put this in an env script at present, but how would you do it if quotes are special?

Just escape it; " goes around the string and " or "" within the string. This seems pretty common...

Quoting quotes is currently needed for the header file. So how would my feature actually improve things?

Between this and Wolfgang's \ at newline I am wondering if this feature will actually improve anything? It we are really going to insist on making the .env file like a C string then I'm not sure what we gain.

I guess I don't really see much wrong with the current header file approach. The benefits this patch provide are the ability to use += and omit the \0. Maintaining a more typical " and \ based quoting system seem fine though. I guess you could avoid the need for \ by using either shell-style here documents:

var += <<<ENDOFHERE data more data ENDOFHERE

or Python-style triple-quotes:

var += """ data more data"""

either of which would degenerate to allowing the following when multi-line and leading/trailing spaces weren't required:

var += value

... although I like the idea of requiring quotes, thus making that:

var += "value"