[PATCH] configure: respect --with-build-time-tools [PR43301]

[PATCH] configure: respect --with-build-time-tools [PR43301]

[Eric Gallager]

[-- Attachment #1: Type: text/plain, Size: 390 bytes --]

Hi, there's been a patch sitting in bug 43301 for over a decade that I
think still makes sense to apply, so I rebased it against current
trunk and found that it still applies. It just makes the configure
script respect the --with-build-time-tools flag. OK to commit?

ChangeLog:

    PR bootstrap/43301
    * configure: Regenerate.
    * configure.ac: Respect --with-build-time-tools flag.

[-- Attachment #2: patch-configure_1.diff --]
[-- Type: application/octet-stream, Size: 1040 bytes --]

diff --git a/configure b/configure
index 65d7078dbe7..4d46b94ebc4 100755
--- a/configure
+++ b/configure
@@ -12850,7 +12850,9 @@ fi
 # Check whether --with-build-time-tools was given.
 if test "${with_build_time_tools+set}" = set; then :
   withval=$with_build_time_tools; case x"$withval" in
-     x/*) ;;
+     x/*)
+       with_build_time_tools=$withval
+       ;;
      *)
        with_build_time_tools=
        { $as_echo "$as_me:${as_lineno-$LINENO}: WARNING: argument to --with-build-time-tools must be an absolute path" >&5
diff --git a/configure.ac b/configure.ac
index 371def9b421..96a878a14cb 100644
--- a/configure.ac
+++ b/configure.ac
@@ -3600,7 +3600,9 @@ AC_ARG_WITH([build-time-tools],
   [AS_HELP_STRING([--with-build-time-tools=PATH],
 		  [use given path to find target tools during the build])],
   [case x"$withval" in
-     x/*) ;;
+     x/*)
+       with_build_time_tools=$withval
+       ;;
      *)
        with_build_time_tools=
        AC_MSG_WARN([argument to --with-build-time-tools must be an absolute path])

Re: [PATCH] configure: respect --with-build-time-tools [PR43301]

[Andreas Schwab]

On Jul 31 2022, Eric Gallager via Gcc-patches wrote:

> It just makes the configure script respect the --with-build-time-tools
> flag.

Why does it make any difference?

> diff --git a/configure b/configure
> index 65d7078dbe7..4d46b94ebc4 100755
> --- a/configure
> +++ b/configure
> @@ -12850,7 +12850,9 @@ fi
>  # Check whether --with-build-time-tools was given.
>  if test "${with_build_time_tools+set}" = set; then :
>    withval=$with_build_time_tools; case x"$withval" in
> -     x/*) ;;
> +     x/*)
> +       with_build_time_tools=$withval

This just reassigns the value that was retrieved a couple of lines
above from the very same variable.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] configure: respect --with-build-time-tools [PR43301]

[Eric Gallager]

On Mon, Aug 1, 2022 at 3:54 AM Andreas Schwab <schwab@suse.de> wrote:
>
> On Jul 31 2022, Eric Gallager via Gcc-patches wrote:
>
> > It just makes the configure script respect the --with-build-time-tools
> > flag.
>
> Why does it make any difference?
>

See the original bug report: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=43301

> > diff --git a/configure b/configure
> > index 65d7078dbe7..4d46b94ebc4 100755
> > --- a/configure
> > +++ b/configure
> > @@ -12850,7 +12850,9 @@ fi
> >  # Check whether --with-build-time-tools was given.
> >  if test "${with_build_time_tools+set}" = set; then :
> >    withval=$with_build_time_tools; case x"$withval" in
> > -     x/*) ;;
> > +     x/*)
> > +       with_build_time_tools=$withval
>
> This just reassigns the value that was retrieved a couple of lines
> above from the very same variable.
>

Oh, ok, so I guess this isn't necessary after all? In which case we
can just close 43301 as INVALID then?

> --
> Andreas Schwab, SUSE Labs, schwab@suse.de
> GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
> "And now for something completely different."

Re: [PATCH] configure: respect --with-build-time-tools [PR43301]

[Alexandre Oliva]

Hello, Eric,

Thanks for looking into this.

On Aug  1, 2022, Eric Gallager via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:

>> This just reassigns the value that was retrieved a couple of lines
>> above from the very same variable.

> Oh, ok, so I guess this isn't necessary after all?

Yeah, I don't see how this patch could make any difference as to the
reported problem.

> In which case we can just close 43301 as INVALID then?

AFAICT, with_build_time_tools is dealt with in the top level configure,
setting up *_FOR_TARGET after searching for the tool names in the
specified location.

However, there's a potentially confusing consequence of the current
code: gcc/configure looks for ./as$build_exeext in the build tree, and
uses that without overwriting it if found, so if an earlier configure
run created an 'as' script, a reconfigure will just use it, without
creating the file again, even if it would have changed
ORIGINAL_AS_FOR_TARGET in it.

I suppose if the patch was tested by the original submitter on a clean
build tree, so it would *appear* to have made a difference in fixing the
problem, while it was actually just a no-op, and the apparent fix was a
consequence of the clean build tree.

So, the patch is not useful, but we may want to avoid the confusing
scenario somehow.

I suppose the point of not creating such a tiny script again is not to
avoid unnecessary rebuilding of dependencies (I don't even see how
dependencies on the script would come into play), so creating it again
wouldn't hurt.  However, we wish to avoid overwriting an assembler
copied into the build tree for use by gcc during the build.  Perhaps:

-elif test -x as$build_exeext; then
+elif test -x as$build_exeext \
+       && { test "x$build_exeext" != "x" \
+            || test "x`grep '^# Invoke as, ld or nm from the build tree' \
+                         as`" = "x"; }; then

WDYT?

-- 
Alexandre Oliva, happy hacker                https://FSFLA.org/blogs/lxo/
   Free Software Activist                       GNU Toolchain Engineer
Disinformation flourishes because many people care deeply about injustice
but very few check the facts.  Ask me about <https://stallmansupport.org>

Re: [PATCH] configure: respect --with-build-time-tools [PR43301]

[Eric Gallager]

On Tue, Aug 2, 2022 at 1:24 AM Alexandre Oliva <oliva@gnu.org> wrote:
>
> Hello, Eric,
>
> Thanks for looking into this.
>
> On Aug  1, 2022, Eric Gallager via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
>
> >> This just reassigns the value that was retrieved a couple of lines
> >> above from the very same variable.
>
> > Oh, ok, so I guess this isn't necessary after all?
>
> Yeah, I don't see how this patch could make any difference as to the
> reported problem.
>
> > In which case we can just close 43301 as INVALID then?
>
> AFAICT, with_build_time_tools is dealt with in the top level configure,
> setting up *_FOR_TARGET after searching for the tool names in the
> specified location.
>
> However, there's a potentially confusing consequence of the current
> code: gcc/configure looks for ./as$build_exeext in the build tree, and
> uses that without overwriting it if found, so if an earlier configure
> run created an 'as' script, a reconfigure will just use it, without
> creating the file again, even if it would have changed
> ORIGINAL_AS_FOR_TARGET in it.
>
> I suppose if the patch was tested by the original submitter on a clean
> build tree, so it would *appear* to have made a difference in fixing the
> problem, while it was actually just a no-op, and the apparent fix was a
> consequence of the clean build tree.
>
> So, the patch is not useful, but we may want to avoid the confusing
> scenario somehow.
>
> I suppose the point of not creating such a tiny script again is not to
> avoid unnecessary rebuilding of dependencies (I don't even see how
> dependencies on the script would come into play), so creating it again
> wouldn't hurt.  However, we wish to avoid overwriting an assembler
> copied into the build tree for use by gcc during the build.  Perhaps:
>
> -elif test -x as$build_exeext; then
> +elif test -x as$build_exeext \
> +       && { test "x$build_exeext" != "x" \
> +            || test "x`grep '^# Invoke as, ld or nm from the build tree' \
> +                         as`" = "x"; }; then
>
> WDYT?

Hi, thanks for the feedback; I'm a bit confused, though: where exactly
would this proposed change go?

>
> --
> Alexandre Oliva, happy hacker                https://FSFLA.org/blogs/lxo/
>    Free Software Activist                       GNU Toolchain Engineer
> Disinformation flourishes because many people care deeply about injustice
> but very few check the facts.  Ask me about <https://stallmansupport.org>

Re: [PATCH] configure: respect --with-build-time-tools [PR43301]

[Alexandre Oliva]

On Aug  2, 2022, Eric Gallager <egall@gwmail.gwu.edu> wrote:

> On Tue, Aug 2, 2022 at 1:24 AM Alexandre Oliva <oliva@gnu.org> wrote:

>> -elif test -x as$build_exeext; then
>> +elif test -x as$build_exeext \
>> +       && { test "x$build_exeext" != "x" \
>> +            || test "x`grep '^# Invoke as, ld or nm from the build tree' \
>> +                         as`" = "x"; }; then
>> 
>> WDYT?

> Hi, thanks for the feedback; I'm a bit confused, though: where exactly
> would this proposed change go?

gcc/configure.ac, just before:

	# Build using assembler in the current directory.
	gcc_cv_as=./as$build_exeext

-- 
Alexandre Oliva, happy hacker                https://FSFLA.org/blogs/lxo/
   Free Software Activist                       GNU Toolchain Engineer
Disinformation flourishes because many people care deeply about injustice
but very few check the facts.  Ask me about <https://stallmansupport.org>

Re: [PATCH] configure: respect --with-build-time-tools [PR43301]

[Eric Gallager]

On Tue, Aug 2, 2022 at 11:33 PM Alexandre Oliva <oliva@gnu.org> wrote:
>
> On Aug  2, 2022, Eric Gallager <egall@gwmail.gwu.edu> wrote:
>
> > On Tue, Aug 2, 2022 at 1:24 AM Alexandre Oliva <oliva@gnu.org> wrote:
>
> >> -elif test -x as$build_exeext; then
> >> +elif test -x as$build_exeext \
> >> +       && { test "x$build_exeext" != "x" \
> >> +            || test "x`grep '^# Invoke as, ld or nm from the build tree' \
> >> +                         as`" = "x"; }; then
> >>
> >> WDYT?
>
> > Hi, thanks for the feedback; I'm a bit confused, though: where exactly
> > would this proposed change go?
>
> gcc/configure.ac, just before:
>
>         # Build using assembler in the current directory.
>         gcc_cv_as=./as$build_exeext
>

OK, after reviewing the surrounding code, I think it makes sense; do
you want to commit it, or should I?
Thanks,
Eric

> --
> Alexandre Oliva, happy hacker                https://FSFLA.org/blogs/lxo/
>    Free Software Activist                       GNU Toolchain Engineer
> Disinformation flourishes because many people care deeply about injustice
> but very few check the facts.  Ask me about <https://stallmansupport.org>

Re: [PATCH] configure: respect --with-build-time-tools [PR43301]

[Alexandre Oliva]

On Aug  3, 2022, Eric Gallager <egall@gwmail.gwu.edu> wrote:

> OK, after reviewing the surrounding code, I think it makes sense; do
> you want to commit it, or should I?

Please proceed, if you have the cycles to give it a round of meaningful
testing (as in, including reconfigure with configure-generated as, and
also with external as in place).

Adjusting the analogous test patterns covering the other tools and
generated scripts would surely be welcome as well ;-)

Thanks!

-- 
Alexandre Oliva, happy hacker                https://FSFLA.org/blogs/lxo/
   Free Software Activist                       GNU Toolchain Engineer
Disinformation flourishes because many people care deeply about injustice
but very few check the facts.  Ask me about <https://stallmansupport.org>