[Buildroot] [PATCH 0/3] package/environment-setup: minor improvements to

[Buildroot] [PATCH 0/3] package/environment-setup: minor improvements to

[Konrad Schwarz]

First patch is to the manual; the wrong file name is used here.

Other fixes:
a) make use of host/environment-setup idempotent;
i.e., PATH is extended only when necessary.
b) Slim the shell code down a little bit and make the
generated file host/environment-setup easier to read.

Konrad Schwarz (3):
  package/environment-setup: fix spelling of the script file in the
    manual.
  package/environment-setup: make script idempotent
  package/environment-setup: improve legibility

 docs/manual/using-buildroot-toolchain.txt     |  4 +-
 .../environment-setup/environment-setup.mk    | 63 ++++++++++---------
 2 files changed, 35 insertions(+), 32 deletions(-)

-- 
2.25.4

[Buildroot] [PATCH 1/3] package/environment-setup: fix spelling of the script file in the manual.

[Konrad Schwarz]

The manual incorrectly refers to the script file as `setup-environment';
it is actually called `environment-setup'.

Signed-off-by: Konrad Schwarz <konrad.schwarz@siemens.com>
---
 docs/manual/using-buildroot-toolchain.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/docs/manual/using-buildroot-toolchain.txt b/docs/manual/using-buildroot-toolchain.txt
index 110be5939f..e202c7d09b 100644
--- a/docs/manual/using-buildroot-toolchain.txt
+++ b/docs/manual/using-buildroot-toolchain.txt
@@ -35,9 +35,9 @@ generating a tarball.
 
 For your convenience, by selecting the option
 +BR2_PACKAGE_HOST_ENVIRONMENT_SETUP+, you can get a
-+setup-environment+ script installed in +output/host/+ and therefore
++environment-setup+ script installed in +output/host/+ and therefore
 in your SDK.  This script can be sourced with
-+. your/sdk/path/environment-setup+ to export a number of environment
++. your/sdk/path/setup-environment+ to export a number of environment
 variables that will help cross-compile your projects using the
 Buildroot SDK: the +PATH+ will contain the SDK binaries, standard
 _autotools_ variables will be defined with the appropriate values, and
-- 
2.25.4

[Buildroot] [PATCH 2/3] package/environment-setup: make script idempotent

[Konrad Schwarz]

One (slight) problem with this kind of environment setup files is when they
are executed repeatedly (e.g., because something else is not quite right
yet), they tend to extend the PATH variable with the same directory,
creating a very unwieldy search path and potentially slowing down
searches for executable files.

This fix adds a test such that PATH is extended only when necessary.

The code has also been simplified
(using standard POSIX shell features) and make the generated
script more readable, e.g., by splitting overly long lines.

Signed-off-by: Konrad Schwarz <konrad.schwarz@siemens.com>
---
 .../environment-setup/environment-setup.mk    | 60 +++++++++----------
 1 file changed, 30 insertions(+), 30 deletions(-)

diff --git a/package/environment-setup/environment-setup.mk b/package/environment-setup/environment-setup.mk
index fcad013f0d..5cb703e421 100644
--- a/package/environment-setup/environment-setup.mk
+++ b/package/environment-setup/environment-setup.mk
@@ -4,38 +4,38 @@
 #
 ################################################################################
 
-ENVIRONMENT_SETUP_FILE = $(HOST_DIR)/environment-setup
-
 define HOST_ENVIRONMENT_SETUP_INSTALL_CMDS
-	cp package/environment-setup/environment-setup $(ENVIRONMENT_SETUP_FILE)
-	for var in $(TARGET_CONFIGURE_OPTS); do \
-		printf "export \"$$var\"\n" >> $(ENVIRONMENT_SETUP_FILE); \
-	done
-	printf "export \"ARCH=$(KERNEL_ARCH)\"\n" >> $(ENVIRONMENT_SETUP_FILE)
-	printf "export \"CROSS_COMPILE=$(TARGET_CROSS)\"\n" >> $(ENVIRONMENT_SETUP_FILE)
-	printf "export \"CONFIGURE_FLAGS=--target=$(GNU_TARGET_NAME) \
-		--host=$(GNU_TARGET_NAME) \
-		--build=$(GNU_HOST_NAME) \
-		--prefix=/usr \
-		--exec-prefix=/usr \
-		--sysconfdir=/etc \
-		--localstatedir=/var \
-		--program-prefix=\"\n" >> $(ENVIRONMENT_SETUP_FILE)
-	printf "alias configure=\"./configure \$${CONFIGURE_FLAGS}\"\n" \
-		>> $(ENVIRONMENT_SETUP_FILE)
-	printf "alias cmake=\"cmake \
-		-DCMAKE_TOOLCHAIN_FILE=$(HOST_DIR)/share/buildroot/toolchainfile.cmake \
-		-DCMAKE_INSTALL_PREFIX=/usr\"\n" >> $(ENVIRONMENT_SETUP_FILE)
-	$(SED) 's%$(HOST_DIR)%\$$SDK_PATH%g' \
-		-e 's%$(HOST_DIR)/bin/%%g' \
-		-e '/^export "PATH=/c\' \
-		$(ENVIRONMENT_SETUP_FILE)
-	printf "export \"PATH=\$$SDK_PATH/bin:\$$SDK_PATH/sbin:\$$PATH\"\n" \
-		>> $(ENVIRONMENT_SETUP_FILE)
-
+	{ \
+	cat package/environment-setup/environment-setup &&\
+	printf 'export "%s"\n' $(TARGET_CONFIGURE_OPTS) &&\
+	printf %b\
+	'export "ARCH=$(KERNEL_ARCH)"\n'\
+	'export "CROSS_COMPILE=$(TARGET_CROSS)"\n'\
+	'export CONFIGURE_FLAGS=\\\n'\
+		'"--target=$(GNU_TARGET_NAME) "\\\n'\
+		'"--host=$(GNU_TARGET_NAME) "\\\n'\
+		'"--build=$(GNU_HOST_NAME) "\\\n'\
+		'"--prefix=/usr "\\\n'\
+		'"--exec-prefix=/usr "\\\n'\
+		'"--sysconfdir=/etc "\\\n'\
+		'"--localstatedir=/var "\\\n'\
+		'"--program-prefix= "\n'\
+	'alias configure="./configure \$$CONFIGURE_FLAGS"\n'\
+	'alias cmake="cmake"\\\n'\
+		'" -DCMAKE_TOOLCHAIN_FILE'\
+		'=$(HOST_DIR)/share/buildroot/toolchainfile.cmake"\\\n'\
+		'" -DCMAKE_INSTALL_PREFIX=/usr"\n'\
+	'if ! command -v $${CROSS_COMPILE}gcc > /dev/null\n'\
+	'then	export PATH="$$SDK_PATH/bin:$$SDK_PATH/sbin:$$PATH"\n'\
+	'fi\n'\
+	\
 	$(if $(BR2_LINUX_KERNEL),\
-		printf "export \"KERNELDIR=$(LINUX_BUILDDIR)\"\n" \
-			>> $(ENVIRONMENT_SETUP_FILE),)
+		'export "KERNELDIR=$(LINUX_BUILDDIR)"\n')\
+	; } |\
+	sed -e 's%$(HOST_DIR)%\$$SDK_PATH%g'\
+		-e 's%$(HOST_DIR)/bin/%%g'\
+		-e '/^export "PATH=/c\'\
+		>$(HOST_DIR)/environment-setup
 endef
 
 $(eval $(host-generic-package))
-- 
2.25.4

[Buildroot] [PATCH 3/3] package/environment-setup: improve legibility

[Konrad Schwarz]

Replaces absolute paths in cross-compilation macros such as CC
with CROSS_COMPILE after first shortening CROSS_COMPILE to remove
leading directories.  This is possible, since the referenced
executables can be found by PATH search.

The primary movtivation for this is that the output of
build processes (make) becomes easier to read as less redundant
information is being presented.
Furthermore, the generated environment-setup script becomes easier
to read and manually modify, should this be necessary.

Although relying on PATH to find the cross-compilation executables
raises the possibility of mistakenly picking up wrong instances,
the names are fairly unique, so this risk should be acceptable.
Since in the original solution some executables were already being found
by PATH search (namely all those not specified by a make macro such as
CC), this change does not introduce fundamentally new risks.

Signed-off-by: Konrad Schwarz <konrad.schwarz@siemens.com>
---
 package/environment-setup/environment-setup.mk | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/package/environment-setup/environment-setup.mk b/package/environment-setup/environment-setup.mk
index 5cb703e421..920dc3003e 100644
--- a/package/environment-setup/environment-setup.mk
+++ b/package/environment-setup/environment-setup.mk
@@ -7,10 +7,17 @@
 define HOST_ENVIRONMENT_SETUP_INSTALL_CMDS
 	{ \
 	cat package/environment-setup/environment-setup &&\
-	printf 'export "%s"\n' $(TARGET_CONFIGURE_OPTS) &&\
+	printf 'export "CROSS_COMPILE=$(TARGET_CROSS)"\n' &&\
+	printf 'CROSS_COMPILE=$${CROSS_COMPILE#$$SDK_PATH/bin/}\n' &&\
+	printf 'export "%s"\n' $(TARGET_CONFIGURE_OPTS) |\
+			sed\
+			-e'/^export "PATH=/d'\
+			-e's:$(TARGET_CROSS):$${CROSS_COMPILE}:g'\
+			-e 's%$(HOST_DIR)/bin/%%g'\
+			-e 's%$(HOST_DIR)%\$$SDK_PATH%g'\
+			&&\
 	printf %b\
 	'export "ARCH=$(KERNEL_ARCH)"\n'\
-	'export "CROSS_COMPILE=$(TARGET_CROSS)"\n'\
 	'export CONFIGURE_FLAGS=\\\n'\
 		'"--target=$(GNU_TARGET_NAME) "\\\n'\
 		'"--host=$(GNU_TARGET_NAME) "\\\n'\
@@ -31,11 +38,7 @@ define HOST_ENVIRONMENT_SETUP_INSTALL_CMDS
 	\
 	$(if $(BR2_LINUX_KERNEL),\
 		'export "KERNELDIR=$(LINUX_BUILDDIR)"\n')\
-	; } |\
-	sed -e 's%$(HOST_DIR)%\$$SDK_PATH%g'\
-		-e 's%$(HOST_DIR)/bin/%%g'\
-		-e '/^export "PATH=/c\'\
-		>$(HOST_DIR)/environment-setup
+	; } >$(HOST_DIR)/environment-setup
 endef
 
 $(eval $(host-generic-package))
-- 
2.25.4

[Buildroot] [PATCH 4/4] package/environment-setup: fix spelling of the script file in the manual.

[Konrad Schwarz]

My previous patch of the same name was erroneous;
the name of the setup file was actually spelled correctly
in one of two instances, and reversed in the other;
I mistakingly reversed the order in both instances.

Signed-off-by: Konrad Schwarz <konrad.schwarz@siemens.com>
---
Interdiff:
  diff --git a/docs/manual/using-buildroot-toolchain.txt b/docs/manual/using-buildroot-toolchain.txt
  index e202c7d09b..09408ef05a 100644
  --- a/docs/manual/using-buildroot-toolchain.txt
  +++ b/docs/manual/using-buildroot-toolchain.txt
  @@ -37,7 +37,7 @@ For your convenience, by selecting the option
   +BR2_PACKAGE_HOST_ENVIRONMENT_SETUP+, you can get a
   +environment-setup+ script installed in +output/host/+ and therefore
   in your SDK.  This script can be sourced with
  -+. your/sdk/path/setup-environment+ to export a number of environment
  ++. your/sdk/path/environment-setup+ to export a number of environment
   variables that will help cross-compile your projects using the
   Buildroot SDK: the +PATH+ will contain the SDK binaries, standard
   _autotools_ variables will be defined with the appropriate values, and

 docs/manual/using-buildroot-toolchain.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/manual/using-buildroot-toolchain.txt b/docs/manual/using-buildroot-toolchain.txt
index e202c7d09b..09408ef05a 100644
--- a/docs/manual/using-buildroot-toolchain.txt
+++ b/docs/manual/using-buildroot-toolchain.txt
@@ -37,7 +37,7 @@ For your convenience, by selecting the option
 +BR2_PACKAGE_HOST_ENVIRONMENT_SETUP+, you can get a
 +environment-setup+ script installed in +output/host/+ and therefore
 in your SDK.  This script can be sourced with
-+. your/sdk/path/setup-environment+ to export a number of environment
++. your/sdk/path/environment-setup+ to export a number of environment
 variables that will help cross-compile your projects using the
 Buildroot SDK: the +PATH+ will contain the SDK binaries, standard
 _autotools_ variables will be defined with the appropriate values, and
-- 
2.25.4

[Buildroot] [PATCH 4/4] package/environment-setup: fix spelling of the script file in the manual.

[Matthew Weber]

Konrad,

On Sun, Jan 3, 2021 at 11:31 AM Konrad Schwarz
<konrad.schwarz@siemens.com> wrote:
>
> My previous patch of the same name was erroneous;
> the name of the setup file was actually spelled correctly
> in one of two instances, and reversed in the other;
> I mistakingly reversed the order in both instances.
>
> Signed-off-by: Konrad Schwarz <konrad.schwarz@siemens.com>

Reviewed-by: Matthew Weber <matthew.weber@rockwellcollins.com>

[Buildroot] [PATCH 2/3] package/environment-setup: make script idempotent

[Matthew Weber]

Konrad,

On Thu, Dec 31, 2020 at 3:51 PM Konrad Schwarz
<konrad.schwarz@siemens.com> wrote:
>
> One (slight) problem with this kind of environment setup files is when they
> are executed repeatedly (e.g., because something else is not quite right
> yet), they tend to extend the PATH variable with the same directory,
> creating a very unwieldy search path and potentially slowing down
> searches for executable files.
>
> This fix adds a test such that PATH is extended only when necessary.
>
> The code has also been simplified
> (using standard POSIX shell features) and make the generated
> script more readable, e.g., by splitting overly long lines.
>
> Signed-off-by: Konrad Schwarz <konrad.schwarz@siemens.com>
> ---
>  .../environment-setup/environment-setup.mk    | 60 +++++++++----------
>  1 file changed, 30 insertions(+), 30 deletions(-)
>
> diff --git a/package/environment-setup/environment-setup.mk b/package/environment-setup/environment-setup.mk
> index fcad013f0d..5cb703e421 100644
> --- a/package/environment-setup/environment-setup.mk
> +++ b/package/environment-setup/environment-setup.mk
> @@ -4,38 +4,38 @@
>  #
>  ################################################################################
>
> -ENVIRONMENT_SETUP_FILE = $(HOST_DIR)/environment-setup
> -
>  define HOST_ENVIRONMENT_SETUP_INSTALL_CMDS
> -       cp package/environment-setup/environment-setup $(ENVIRONMENT_SETUP_FILE)
> -       for var in $(TARGET_CONFIGURE_OPTS); do \
> -               printf "export \"$$var\"\n" >> $(ENVIRONMENT_SETUP_FILE); \
> -       done
> -       printf "export \"ARCH=$(KERNEL_ARCH)\"\n" >> $(ENVIRONMENT_SETUP_FILE)
> -       printf "export \"CROSS_COMPILE=$(TARGET_CROSS)\"\n" >> $(ENVIRONMENT_SETUP_FILE)
> -       printf "export \"CONFIGURE_FLAGS=--target=$(GNU_TARGET_NAME) \
> -               --host=$(GNU_TARGET_NAME) \
> -               --build=$(GNU_HOST_NAME) \
> -               --prefix=/usr \
> -               --exec-prefix=/usr \
> -               --sysconfdir=/etc \
> -               --localstatedir=/var \
> -               --program-prefix=\"\n" >> $(ENVIRONMENT_SETUP_FILE)
> -       printf "alias configure=\"./configure \$${CONFIGURE_FLAGS}\"\n" \
> -               >> $(ENVIRONMENT_SETUP_FILE)
> -       printf "alias cmake=\"cmake \
> -               -DCMAKE_TOOLCHAIN_FILE=$(HOST_DIR)/share/buildroot/toolchainfile.cmake \
> -               -DCMAKE_INSTALL_PREFIX=/usr\"\n" >> $(ENVIRONMENT_SETUP_FILE)
> -       $(SED) 's%$(HOST_DIR)%\$$SDK_PATH%g' \
> -               -e 's%$(HOST_DIR)/bin/%%g' \
> -               -e '/^export "PATH=/c\' \
> -               $(ENVIRONMENT_SETUP_FILE)
> -       printf "export \"PATH=\$$SDK_PATH/bin:\$$SDK_PATH/sbin:\$$PATH\"\n" \
> -               >> $(ENVIRONMENT_SETUP_FILE)
> -
> +       { \
> +       cat package/environment-setup/environment-setup &&\
> +       printf 'export "%s"\n' $(TARGET_CONFIGURE_OPTS) &&\
> +       printf %b\
> +       'export "ARCH=$(KERNEL_ARCH)"\n'\
> +       'export "CROSS_COMPILE=$(TARGET_CROSS)"\n'\
> +       'export CONFIGURE_FLAGS=\\\n'\
> +               '"--target=$(GNU_TARGET_NAME) "\\\n'\
> +               '"--host=$(GNU_TARGET_NAME) "\\\n'\
> +               '"--build=$(GNU_HOST_NAME) "\\\n'\
> +               '"--prefix=/usr "\\\n'\
> +               '"--exec-prefix=/usr "\\\n'\
> +               '"--sysconfdir=/etc "\\\n'\
> +               '"--localstatedir=/var "\\\n'\
> +               '"--program-prefix= "\n'\
> +       'alias configure="./configure \$$CONFIGURE_FLAGS"\n'\
> +       'alias cmake="cmake"\\\n'\
> +               '" -DCMAKE_TOOLCHAIN_FILE'\
> +               '=$(HOST_DIR)/share/buildroot/toolchainfile.cmake"\\\n'\
> +               '" -DCMAKE_INSTALL_PREFIX=/usr"\n'\
> +       'if ! command -v $${CROSS_COMPILE}gcc > /dev/null\n'\
> +       'then   export PATH="$$SDK_PATH/bin:$$SDK_PATH/sbin:$$PATH"\n'\
> +       'fi\n'\

Does this new path check work as you intend if the main system has the
normal gcc tools installed?  Maybe checking for the cross variable not
being empty would be a better option?

Regards,
Matt

[Buildroot] [PATCH 2/3] package/environment-setup: make script idempotent

[Schwarz, Konrad]

Hi Matt,

> -----Original Message-----
> From: Matthew Weber <matthew.weber@collins.com>
> Sent: Wednesday, January 6, 2021 18:34
> To: Schwarz, Konrad (T RDA IOT SES-DE) <konrad.schwarz@siemens.com>
> Cc: buildroot <buildroot@buildroot.org>
> Subject: Re: [Buildroot] [PATCH 2/3] package/environment-setup: make script idempotent
> 
> > One (slight) problem with this kind of environment setup files is when
> > they are executed repeatedly (e.g., because something else is not
> > quite right yet), they tend to extend the PATH variable with the same
> > directory, creating a very unwieldy search path and potentially
> > slowing down searches for executable files.
> >
> > This fix adds a test such that PATH is extended only when necessary.
> >
[...]
> +       'export "CROSS_COMPILE=$(TARGET_CROSS)"\n'\
[...]
> > +       'if ! command -v $${CROSS_COMPILE}gcc > /dev/null\n'\
> > +       'then   export PATH="$$SDK_PATH/bin:$$SDK_PATH/sbin:$$PATH"\n'\
> > +       'fi\n'\
> 
> Does this new path check work as you intend if the main system has the normal gcc tools installed?  Maybe checking for the cross
> variable not being empty would be a better option?

As an aside: if the main system hast the "normal gcc tools" installed (I'm assuming you mean the native tools) and visible on PATH you wouldn't want PATH extended either, right?

As written, the code extends PATH only when the Buildroot-specific cross-compilation toolchain cannot be found (using a PATH search).  Conversely: if the Buildroot-specific toolchain is already in PATH, PATH won't be extended.

The only situation I see in which this might be undesirable is if an identically-prefixed toolchain that should not be used were already in the PATH; the original code would simply override that (because it prefixes PATH).   I consider this a remote possibility, because the "vendor" part of the GNU target triplet should be unique to Buildroot, at least when using the internal compilation toolchain option; but fundamentally the same reasoning should apply to the external toolchain option (e.g., buildroot assumes the user knows what he is doing in this case).

When debugging PATH problems, I'd much prefer to have as clean of a PATH as possible; elimination of (redundant) duplicates is a key part of this.

Note that the script itself sets CROSS_COMPILE at the start, so I don't see when it would be empty.

--
Konrad

[Buildroot] [External] RE: [PATCH 2/3] package/environment-setup: make script idempotent

[Matthew Weber]

Konrad,


On Thu, Jan 7, 2021 at 3:58 AM Schwarz, Konrad
<konrad.schwarz@siemens.com> wrote:
>
> Hi Matt,
>
> > -----Original Message-----
> > From: Matthew Weber <matthew.weber@collins.com>
> > Sent: Wednesday, January 6, 2021 18:34
> > To: Schwarz, Konrad (T RDA IOT SES-DE) <konrad.schwarz@siemens.com>
> > Cc: buildroot <buildroot@buildroot.org>
> > Subject: Re: [Buildroot] [PATCH 2/3] package/environment-setup: make script idempotent
> >
> > > One (slight) problem with this kind of environment setup files is when
> > > they are executed repeatedly (e.g., because something else is not
> > > quite right yet), they tend to extend the PATH variable with the same
> > > directory, creating a very unwieldy search path and potentially
> > > slowing down searches for executable files.
> > >
> > > This fix adds a test such that PATH is extended only when necessary.
> > >
> [...]
> > +       'export "CROSS_COMPILE=$(TARGET_CROSS)"\n'\
> [...]
> > > +       'if ! command -v $${CROSS_COMPILE}gcc > /dev/null\n'\
> > > +       'then   export PATH="$$SDK_PATH/bin:$$SDK_PATH/sbin:$$PATH"\n'\
> > > +       'fi\n'\
> >
> > Does this new path check work as you intend if the main system has the normal gcc tools installed?  Maybe checking for the cross
> > variable not being empty would be a better option?
>
> As an aside: if the main system hast the "normal gcc tools" installed (I'm assuming you mean the native tools) and visible on PATH you wouldn't want PATH extended either, right?

Not really a case we should hit like I clarified below.  If that
command test failed, someone moved the script before sourcing or there
is a dependency issue where the command executable doesn't work.

>
> As written, the code extends PATH only when the Buildroot-specific cross-compilation toolchain cannot be found (using a PATH search).  Conversely: if the Buildroot-specific toolchain is already in PATH, PATH won't be extended.
>
> The only situation I see in which this might be undesirable is if an identically-prefixed toolchain that should not be used were already in the PATH; the original code would simply override that (because it prefixes PATH).   I consider this a remote possibility, because the "vendor" part of the GNU target triplet should be unique to Buildroot, at least when using the internal compilation toolchain option; but fundamentally the same reasoning should apply to the external toolchain option (e.g., buildroot assumes the user knows what he is doing in this case).

Could you just grep the PATH to see if the SDK path is present instead
of doing a command test?

>
> When debugging PATH problems, I'd much prefer to have as clean of a PATH as possible; elimination of (redundant) duplicates is a key part of this.
>
> Note that the script itself sets CROSS_COMPILE at the start, so I don't see when it would be empty.

Yeah, I was thinking about it from the perspective of someone moving
the script from its original location before sourcing (ie the
variables get set but w/ an incorrect path).  However that isn't
probably a real use case so it should be fine and like you said.

Matt

[Buildroot] [External] RE: [PATCH 2/3] package/environment-setup: make script idempotent

[Schwarz, Konrad]

Hi Matt,

> -----Original Message-----
> From: Matthew Weber <matthew.weber@collins.com>
> Sent: Thursday, January 7, 2021 19:57
> To: Schwarz, Konrad (T RDA IOT SES-DE) <konrad.schwarz@siemens.com>
> Cc: buildroot <buildroot@buildroot.org>
> Subject: Re: [External] RE: [Buildroot] [PATCH 2/3] package/environment-setup: make script idempotent
> > As written, the code extends PATH only when the Buildroot-specific cross-compilation toolchain cannot be found (using a PATH
> search).  Conversely: if the Buildroot-specific toolchain is already in PATH, PATH won't be extended.
> >
> > The only situation I see in which this might be undesirable is if an identically-prefixed toolchain that should not be used were already
> in the PATH; the original code would simply override that (because it prefixes PATH).   I consider this a remote possibility, because the
> "vendor" part of the GNU target triplet should be unique to Buildroot, at least when using the internal compilation toolchain option;
> but fundamentally the same reasoning should apply to the external toolchain option (e.g., buildroot assumes the user knows what he
> is doing in this case).
> 
> Could you just grep the PATH to see if the SDK path is present instead of doing a command test?

	command -v executable
is _the_ POSIX compliant way of checking if an executable can be found via PATH.  See https://pubs.opengroup.org/onlinepubs/9699919799/utilities/command.html.  Trying to recreate this via grep or other means would seem to be clearly more error prone (and command -v, being a built-in, is faster to boot -- no pun intended). Additionally, environments such as MSYS, where the path separation character is not : (colon) but ; (semicolon), as in MS-DOS, additionally complicate a manual re-implementation of command -v.

--
Konrad

[Buildroot] [External] RE: [PATCH 2/3] package/environment-setup: make script idempotent

[Arnout Vandecappelle]

On 07/01/2021 22:15, Schwarz, Konrad wrote:
> Hi Matt,
> 
>> -----Original Message-----
>> From: Matthew Weber <matthew.weber@collins.com>
>> Sent: Thursday, January 7, 2021 19:57
>> To: Schwarz, Konrad (T RDA IOT SES-DE) <konrad.schwarz@siemens.com>
>> Cc: buildroot <buildroot@buildroot.org>
>> Subject: Re: [External] RE: [Buildroot] [PATCH 2/3] package/environment-setup: make script idempotent
>>> As written, the code extends PATH only when the Buildroot-specific cross-compilation toolchain cannot be found (using a PATH
>> search).  Conversely: if the Buildroot-specific toolchain is already in PATH, PATH won't be extended.
>>>
>>> The only situation I see in which this might be undesirable is if an identically-prefixed toolchain that should not be used were already
>> in the PATH; the original code would simply override that (because it prefixes PATH).   I consider this a remote possibility, because the
>> "vendor" part of the GNU target triplet should be unique to Buildroot, at least when using the internal compilation toolchain option;
>> but fundamentally the same reasoning should apply to the external toolchain option (e.g., buildroot assumes the user knows what he
>> is doing in this case).
>>
>> Could you just grep the PATH to see if the SDK path is present instead of doing a command test?
> 
> 	command -v executable
> is _the_ POSIX compliant way of checking if an executable can be found via PATH.  See https://pubs.opengroup.org/onlinepubs/9699919799/utilities/command.html.  Trying to recreate this via grep or other means would seem to be clearly more error prone (and command -v, being a built-in, is faster to boot -- no pun intended). Additionally, environments such as MSYS, where the path separation character is not : (colon) but ; (semicolon), as in MS-DOS, additionally complicate a manual re-implementation of command -v.

 What Matt means is that, to avoid adding $SDK_PATH/bin to PATH when it is
already there, it is better to do

if ! echo "$PATH" | grep -q -E "(^|:)$SDK_PATH/bin($|:)"; then ...

 That way, you *exactly* check if it's a double entry or not.

 Regards,
 Arnout

[Buildroot] [PATCH 2/3] package/environment-setup: make script idempotent

[Arnout Vandecappelle]

On 31/12/2020 22:29, Konrad Schwarz wrote:
> One (slight) problem with this kind of environment setup files is when they
> are executed repeatedly (e.g., because something else is not quite right
> yet), they tend to extend the PATH variable with the same directory,
> creating a very unwieldy search path and potentially slowing down
> searches for executable files.
> 
> This fix adds a test such that PATH is extended only when necessary.
> 
> The code has also been simplified
> (using standard POSIX shell features) and make the generated
> script more readable, e.g., by splitting overly long lines.

 Maybe the generated script is more readable (and I'm not entirely convinced),
but the code in the .mk file is much *less* readable.

 If you want to make it more readable, I think it would be better to do change
the environment-setup script to environment-setup.in that contains @VAR@
references which are replaced with sed. In fact, since this kind of approach is
used in many places, it would be worth adding a make function that does exactly
that. But maybe that's going a bit too far :-)


 Also, this change of how the script is formatted should be split in a separate
patch from the PATH manipulation, they are pretty much unrelated.

 Regards,
 Arnout

> Signed-off-by: Konrad Schwarz <konrad.schwarz@siemens.com>
> ---
>  .../environment-setup/environment-setup.mk    | 60 +++++++++----------
>  1 file changed, 30 insertions(+), 30 deletions(-)
> 
> diff --git a/package/environment-setup/environment-setup.mk b/package/environment-setup/environment-setup.mk
> index fcad013f0d..5cb703e421 100644
> --- a/package/environment-setup/environment-setup.mk
> +++ b/package/environment-setup/environment-setup.mk
> @@ -4,38 +4,38 @@
>  #
>  ################################################################################
>  
> -ENVIRONMENT_SETUP_FILE = $(HOST_DIR)/environment-setup
> -
>  define HOST_ENVIRONMENT_SETUP_INSTALL_CMDS
> -	cp package/environment-setup/environment-setup $(ENVIRONMENT_SETUP_FILE)
> -	for var in $(TARGET_CONFIGURE_OPTS); do \
> -		printf "export \"$$var\"\n" >> $(ENVIRONMENT_SETUP_FILE); \
> -	done
> -	printf "export \"ARCH=$(KERNEL_ARCH)\"\n" >> $(ENVIRONMENT_SETUP_FILE)
> -	printf "export \"CROSS_COMPILE=$(TARGET_CROSS)\"\n" >> $(ENVIRONMENT_SETUP_FILE)
> -	printf "export \"CONFIGURE_FLAGS=--target=$(GNU_TARGET_NAME) \
> -		--host=$(GNU_TARGET_NAME) \
> -		--build=$(GNU_HOST_NAME) \
> -		--prefix=/usr \
> -		--exec-prefix=/usr \
> -		--sysconfdir=/etc \
> -		--localstatedir=/var \
> -		--program-prefix=\"\n" >> $(ENVIRONMENT_SETUP_FILE)
> -	printf "alias configure=\"./configure \$${CONFIGURE_FLAGS}\"\n" \
> -		>> $(ENVIRONMENT_SETUP_FILE)
> -	printf "alias cmake=\"cmake \
> -		-DCMAKE_TOOLCHAIN_FILE=$(HOST_DIR)/share/buildroot/toolchainfile.cmake \
> -		-DCMAKE_INSTALL_PREFIX=/usr\"\n" >> $(ENVIRONMENT_SETUP_FILE)
> -	$(SED) 's%$(HOST_DIR)%\$$SDK_PATH%g' \
> -		-e 's%$(HOST_DIR)/bin/%%g' \
> -		-e '/^export "PATH=/c\' \
> -		$(ENVIRONMENT_SETUP_FILE)
> -	printf "export \"PATH=\$$SDK_PATH/bin:\$$SDK_PATH/sbin:\$$PATH\"\n" \
> -		>> $(ENVIRONMENT_SETUP_FILE)
> -
> +	{ \
> +	cat package/environment-setup/environment-setup &&\
> +	printf 'export "%s"\n' $(TARGET_CONFIGURE_OPTS) &&\
> +	printf %b\
> +	'export "ARCH=$(KERNEL_ARCH)"\n'\
> +	'export "CROSS_COMPILE=$(TARGET_CROSS)"\n'\
> +	'export CONFIGURE_FLAGS=\\\n'\
> +		'"--target=$(GNU_TARGET_NAME) "\\\n'\
> +		'"--host=$(GNU_TARGET_NAME) "\\\n'\
> +		'"--build=$(GNU_HOST_NAME) "\\\n'\
> +		'"--prefix=/usr "\\\n'\
> +		'"--exec-prefix=/usr "\\\n'\
> +		'"--sysconfdir=/etc "\\\n'\
> +		'"--localstatedir=/var "\\\n'\
> +		'"--program-prefix= "\n'\
> +	'alias configure="./configure \$$CONFIGURE_FLAGS"\n'\
> +	'alias cmake="cmake"\\\n'\
> +		'" -DCMAKE_TOOLCHAIN_FILE'\
> +		'=$(HOST_DIR)/share/buildroot/toolchainfile.cmake"\\\n'\
> +		'" -DCMAKE_INSTALL_PREFIX=/usr"\n'\
> +	'if ! command -v $${CROSS_COMPILE}gcc > /dev/null\n'\
> +	'then	export PATH="$$SDK_PATH/bin:$$SDK_PATH/sbin:$$PATH"\n'\
> +	'fi\n'\
> +	\
>  	$(if $(BR2_LINUX_KERNEL),\
> -		printf "export \"KERNELDIR=$(LINUX_BUILDDIR)\"\n" \
> -			>> $(ENVIRONMENT_SETUP_FILE),)
> +		'export "KERNELDIR=$(LINUX_BUILDDIR)"\n')\
> +	; } |\
> +	sed -e 's%$(HOST_DIR)%\$$SDK_PATH%g'\
> +		-e 's%$(HOST_DIR)/bin/%%g'\
> +		-e '/^export "PATH=/c\'\
> +		>$(HOST_DIR)/environment-setup
>  endef
>  
>  $(eval $(host-generic-package))
> 

[Buildroot] [PATCH 4/4] package/environment-setup: fix spelling of the script file in the manual.

[Arnout Vandecappelle]

On 03/01/2021 18:29, Konrad Schwarz wrote:
> My previous patch of the same name was erroneous;
> the name of the setup file was actually spelled correctly
> in one of two instances, and reversed in the other;
> I mistakingly reversed the order in both instances.
> 
> Signed-off-by: Konrad Schwarz <konrad.schwarz@siemens.com>

 When you fix a patch that hasn't been applied yet, please don't send a patch on
top of the previous one. Instead, squash the two and just send the final patch.
Which in this case would be:

commit 7197b1bc44395cbe927dc56f0b7fc53d8ce07636 (HEAD -> master)
Author: Konrad Schwarz <konrad.schwarz@siemens.com>
Date:   Thu Dec 31 22:29:47 2020

    package/environment-setup: fix spelling of the script file in the manual.

    The manual incorrectly refers to the script file as `setup-environment';
    it is actually called `environment-setup'.

    Signed-off-by: Konrad Schwarz <konrad.schwarz@siemens.com>
    Reviewed-by: Matthew Weber <matthew.weber@rockwellcollins.com>
    Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>

diff --git a/docs/manual/using-buildroot-toolchain.txt
b/docs/manual/using-buildroot-toolchain.txt
index 110be5939f..09408ef05a 100644
--- a/docs/manual/using-buildroot-toolchain.txt
+++ b/docs/manual/using-buildroot-toolchain.txt
@@ -35,7 +35,7 @@ generating a tarball.

 For your convenience, by selecting the option
 +BR2_PACKAGE_HOST_ENVIRONMENT_SETUP+, you can get a
-+setup-environment+ script installed in +output/host/+ and therefore
++environment-setup+ script installed in +output/host/+ and therefore
 in your SDK.  This script can be sourced with
 +. your/sdk/path/environment-setup+ to export a number of environment
 variables that will help cross-compile your projects using the



 Also, make sure that the commit message doesn't contain "personal" messages
like "I mistakingly reversed the order".


 I fixed all of that and applied to master, thanks.

> ---
> Interdiff:
>   diff --git a/docs/manual/using-buildroot-toolchain.txt b/docs/manual/using-buildroot-toolchain.txt
>   index e202c7d09b..09408ef05a 100644
>   --- a/docs/manual/using-buildroot-toolchain.txt
>   +++ b/docs/manual/using-buildroot-toolchain.txt
>   @@ -37,7 +37,7 @@ For your convenience, by selecting the option
>    +BR2_PACKAGE_HOST_ENVIRONMENT_SETUP+, you can get a
>    +environment-setup+ script installed in +output/host/+ and therefore
>    in your SDK.  This script can be sourced with
>   -+. your/sdk/path/setup-environment+ to export a number of environment
>   ++. your/sdk/path/environment-setup+ to export a number of environment
>    variables that will help cross-compile your projects using the
>    Buildroot SDK: the +PATH+ will contain the SDK binaries, standard
>    _autotools_ variables will be defined with the appropriate values, and

 This interdiff is actually interesting - but here you actually just copied the
patch that is just below...


 Regards,
 Arnout

> 
>  docs/manual/using-buildroot-toolchain.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/docs/manual/using-buildroot-toolchain.txt b/docs/manual/using-buildroot-toolchain.txt
> index e202c7d09b..09408ef05a 100644
> --- a/docs/manual/using-buildroot-toolchain.txt
> +++ b/docs/manual/using-buildroot-toolchain.txt
> @@ -37,7 +37,7 @@ For your convenience, by selecting the option
>  +BR2_PACKAGE_HOST_ENVIRONMENT_SETUP+, you can get a
>  +environment-setup+ script installed in +output/host/+ and therefore
>  in your SDK.  This script can be sourced with
> -+. your/sdk/path/setup-environment+ to export a number of environment
> ++. your/sdk/path/environment-setup+ to export a number of environment
>  variables that will help cross-compile your projects using the
>  Buildroot SDK: the +PATH+ will contain the SDK binaries, standard
>  _autotools_ variables will be defined with the appropriate values, and
> 

[Buildroot] [PATCH 1/3] package/environment-setup: fix spelling of the script file in the manual.

[Peter Korsgaard]

>>>>> "Konrad" == Konrad Schwarz <konrad.schwarz@siemens.com> writes:

 > The manual incorrectly refers to the script file as `setup-environment';
 > it is actually called `environment-setup'.

 > Signed-off-by: Konrad Schwarz <konrad.schwarz@siemens.com>

Committed to 2020.11.x, thanks.

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH 2/3] package/environment-setup: make script idempotent

[Schwarz, Konrad]

Hi Arnout,

> -----Original Message-----
> From: Arnout Vandecappelle <arnout@mind.be>
> Sent: Thursday, January 7, 2021 22:43
> To: Schwarz, Konrad (T RDA IOT SES-DE) <konrad.schwarz@siemens.com>; buildroot at buildroot.org
> Subject: Re: [Buildroot] [PATCH 2/3] package/environment-setup: make script idempotent

> > The code has also been simplified
> > (using standard POSIX shell features) and make the generated script
> > more readable, e.g., by splitting overly long lines.
> 
>  Maybe the generated script is more readable (and I'm not entirely convinced), but the code in the .mk file is much *less* readable.

Well, beauty is obviously in the eye of the beholder. 

Objectively, the change eliminates wrapping each generated line with
"printf ... >> $(ENVIRONMENT_SETUP_FILE)" by placing the code in a compound block,
and can eliminate the variable ENVIRONMENT_SETUP_FILE, since its value ends up being used
only once in the result.

It eliminates the for ;do done loop over TARGET_CONFIGURE_OPTS, because printf(1) has this ability built in.

It eliminates all occurrences of backslash-escaped double quotes, because it uses single quotes to
quote strings (that may contain double quotes).

Backslash escapes are still required to indicate the ends of continued lines in the generated shell file
(i.e., a backslash followed by a newline), but I value the readability of the generated file high enough so I am
willing to make that trade-off.   This is because I expect many more users to look at the generated script than its'
generator to figure out what the script does (and where it fails).

The number of these sequences could have been reduced by using more printf
statements; I don't think that alternative is a clear win, because you would end up with more mental switches
between generator and generated code.

It eliminates re-editing of the generated file by using sed as a filter in a pipeline.

>  If you want to make it more readable, I think it would be better to do change the environment-setup script to environment-setup.in
> that contains @VAR@ references which are replaced with sed. In fact, since this kind of approach is used in many places, it would be
> worth adding a make function that does exactly that. But maybe that's going a bit too far :-)

I don't see how that technique is applicable here; simple substitution of this sort is also prone to breakage
(aka SQL injection).

>  Also, this change of how the script is formatted should be split in a separate patch from the PATH manipulation, they are pretty much
> unrelated.

I hope the individual changes to the script mechanics have now been sufficiently motivated.

--
Konrad

[Buildroot] [External] RE: [PATCH 2/3] package/environment-setup: make script idempotent

[Schwarz, Konrad]

Hi Arnout,

> >> Could you just grep the PATH to see if the SDK path is present instead of doing a command test?
> >
> > 	command -v executable
> > is _the_ POSIX compliant way of checking if an executable can be found via PATH.  See
> https://pubs.opengroup.org/onlinepubs/9699919799/utilities/command.html.  Trying to recreate this via grep or other means would
> seem to be clearly more error prone (and command -v, being a built-in, is faster to boot -- no pun intended). Additionally,
> environments such as MSYS, where the path separation character is not : (colon) but ; (semicolon), as in MS-DOS, additionally
> complicate a manual re-implementation of command -v.
> 
>  What Matt means is that, to avoid adding $SDK_PATH/bin to PATH when it is already there, it is better to do
> 
> if ! echo "$PATH" | grep -q -E "(^|:)$SDK_PATH/bin($|:)"; then ...
> 
>  That way, you *exactly* check if it's a double entry or not.

As noted above, this would fail in MSYS.  Similar to my suggestion, this code is not exactly equivalent to the original:
 there is no guarantee that a cross-compilation toolchain found by PATH search is actually the one in $SDK_PATH/bin.
(Portable code should use printf %s\\n "$PATH" rather than echo).  All in all, I fail to see substantial benefit of this
suggestion.

You could however use
	test "$(command -v ${CROSS_COMPILE}gcc)" = "$SDK_PATH/bin/${CROSS_COMPILE}gcc"
to be 100% sure.

--
Konrad

[Buildroot] [External] RE: [PATCH 2/3] package/environment-setup: make script idempotent

[Arnout Vandecappelle]

On 08/01/2021 14:10, Schwarz, Konrad wrote:
> 
> Hi Arnout,
> 
>>>> Could you just grep the PATH to see if the SDK path is present instead of doing a command test?
>>>
>>> 	command -v executable
>>> is _the_ POSIX compliant way of checking if an executable can be found via PATH.  See
>> https://pubs.opengroup.org/onlinepubs/9699919799/utilities/command.html.  Trying to recreate this via grep or other means would
>> seem to be clearly more error prone (and command -v, being a built-in, is faster to boot -- no pun intended). Additionally,
>> environments such as MSYS, where the path separation character is not : (colon) but ; (semicolon), as in MS-DOS, additionally
>> complicate a manual re-implementation of command -v.
>>
>>  What Matt means is that, to avoid adding $SDK_PATH/bin to PATH when it is already there, it is better to do
>>
>> if ! echo "$PATH" | grep -q -E "(^|:)$SDK_PATH/bin($|:)"; then ...
>>
>>  That way, you *exactly* check if it's a double entry or not.
> 
> As noted above, this would fail in MSYS.

 Does the rest van Buildroot work in MSYS? I'd be very surprised since things
already often break on FreeBSD...


>  Similar to my suggestion, this code is not exactly equivalent to the original:
>  there is no guarantee that a cross-compilation toolchain found by PATH search is actually the one in $SDK_PATH/bin.
> (Portable code should use printf %s\\n "$PATH" rather than echo).

 Why? echo seems pretty well specified in POSIX, as long as no special
characters need to be used.


>  All in all, I fail to see substantial benefit of this
> suggestion.

 The problem indicated by Matt is that it is possible that a pre-installed
toolchain can already be found in $PATH. When such a toolchain is used as an
external toolchain, Buildroot will generate a wrapper executable with exactly
the same name as the original toolchain. Concretely: if you define
/usr/local/bin/aarch64-none-linux-gnu-gcc as the pre-installed external
toolchain, Buildroot will create a symlink
output/host/bin/aarch64-none-linux-gnu-gcc that points to the wrapper
executable, and the wrapper looks up aarch64-none-linux-gnu-gcc in /usr/local/bin.

 With this change, if /usr/local/bin is already in $PATH, environment-setup will
not extend PATH with the buildroot wrapper. Thus, if you run
aarch64-none-linux-gnu-gcc, you will not get the arch specification and the
pointer to the sysroot and all the rest that is added by the wrapper - which
essentially makes the toolchain misbehave.

 Although I don't care much about this environment-setup script, I really don't
want to make it completely broken :-)


> You could however use
> 	test "$(command -v ${CROSS_COMPILE}gcc)" = "$SDK_PATH/bin/${CROSS_COMPILE}gcc"
> to be 100% sure.

 That would indeed be a good option.


 Note BTW that I marked the series as Changes Requested, so it won't be applied
unless you repost it.

 Regards,
 Arnout

[Buildroot] [PATCH 2/3] package/environment-setup: make script idempotent

[Arnout Vandecappelle]

On 08/01/2021 13:32, Schwarz, Konrad wrote:
> Hi Arnout,
> 
>> -----Original Message-----
>> From: Arnout Vandecappelle <arnout@mind.be>
>> Sent: Thursday, January 7, 2021 22:43
>> To: Schwarz, Konrad (T RDA IOT SES-DE) <konrad.schwarz@siemens.com>; buildroot at buildroot.org
>> Subject: Re: [Buildroot] [PATCH 2/3] package/environment-setup: make script idempotent
> 
>>> The code has also been simplified
>>> (using standard POSIX shell features) and make the generated script
>>> more readable, e.g., by splitting overly long lines.
>>
>>  Maybe the generated script is more readable (and I'm not entirely convinced), but the code in the .mk file is much *less* readable.
> 
> Well, beauty is obviously in the eye of the beholder. 
> 
> Objectively, the change eliminates wrapping each generated line with
> "printf ... >> $(ENVIRONMENT_SETUP_FILE)" by placing the code in a compound block,
> and can eliminate the variable ENVIRONMENT_SETUP_FILE, since its value ends up being used
> only once in the result.
> 
> It eliminates the for ;do done loop over TARGET_CONFIGURE_OPTS, because printf(1) has this ability built in.
> 
> It eliminates all occurrences of backslash-escaped double quotes, because it uses single quotes to
> quote strings (that may contain double quotes).

 Wow, I missed all of that... I just saw all the backslashes.


> Backslash escapes are still required to indicate the ends of continued lines in the generated shell file
> (i.e., a backslash followed by a newline), but I value the readability of the generated file high enough so I am
> willing to make that trade-off.   This is because I expect many more users to look at the generated script than its'
> generator to figure out what the script does (and where it fails).
> 
> The number of these sequences could have been reduced by using more printf
> statements; I don't think that alternative is a clear win, because you would end up with more mental switches
> between generator and generated code.
> 
> It eliminates re-editing of the generated file by using sed as a filter in a pipeline.
> 
>>  If you want to make it more readable, I think it would be better to do change the environment-setup script to environment-setup.in
>> that contains @VAR@ references which are replaced with sed. In fact, since this kind of approach is used in many places, it would be
>> worth adding a make function that does exactly that. But maybe that's going a bit too far :-)
> 
> I don't see how that technique is applicable here; simple substitution of this sort is also prone to breakage
> (aka SQL injection).

 Hardly... It would require something like @GNU_TARGET_NAME@ to be present in
the path. If a user does that to themselves, I don't feel we need to cater to them.


>>  Also, this change of how the script is formatted should be split in a separate patch from the PATH manipulation, they are pretty much
>> unrelated.
> 
> I hope the individual changes to the script mechanics have now been sufficiently motivated.

 I'm not sure I see your point here. Unrelated changes should be in seperate
patches, plain and simple.

 Regards,
 Arnout