[libgomp,testsuite] Fix hardcoded libexec in plugin/configfrag.ac

Message ID 20220328084543.GA5967@delia.home
State New
Headers
Series [libgomp,testsuite] Fix hardcoded libexec in plugin/configfrag.ac |

Commit Message

Tom de Vries March 28, 2022, 8:45 a.m. UTC
  Hi,

When building an nvptx offloading configuration on openSUSE Leap 15.3, the
site script /usr/share/site/x86_64-unknown-linux-gnu is activated, setting
libexecdir to ${exec_prefix}/lib rather than ${exec_prefix}/libexec:
...
| # If user did not specify libexecdir, set the correct target:
| # Nor FHS nor openSUSE allow prefix/libexec. Let's default to prefix/lib.
|
| if test "$libexecdir" = '${exec_prefix}/libexec' ; then
|       libexecdir='${exec_prefix}/lib'
| fi
...

However, in libgomp libgomp/plugin/configfrag.ac we hardcode libexec:
...
    # Configure additional search paths.
    if test x"$tgt_dir" != x; then
      offload_additional_options="$offload_additional_options \
        -B$tgt_dir/libexec/gcc/\$(target_alias)/\$(gcc_version) \
	-B$tgt_dir/bin"
...

Fix this by using /$(libexecdir:\$(exec_prefix)/%=%)/ instead of /libexec/.

Tested on x86_64-linux with nvptx accelerator.

OK for trunk?

Thanks,
- Tom

[libgomp, testsuite] Fix hardcoded libexec in plugin/configfrag.ac

libgomp/ChangeLog:

2022-03-28  Tom de Vries  <tdevries@suse.de>

	* plugin/configfrag.ac: Use /$(libexecdir:\$(exec_prefix)/%=%)/
	instead of /libexec/.
	* configure: Regenerate.

---
 libgomp/configure            | 2 +-
 libgomp/plugin/configfrag.ac | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
  

Comments

Richard Biener March 28, 2022, 8:49 a.m. UTC | #1
On Mon, 28 Mar 2022, Tom de Vries wrote:

> Hi,
> 
> When building an nvptx offloading configuration on openSUSE Leap 15.3, the
> site script /usr/share/site/x86_64-unknown-linux-gnu is activated, setting
> libexecdir to ${exec_prefix}/lib rather than ${exec_prefix}/libexec:
> ...
> | # If user did not specify libexecdir, set the correct target:
> | # Nor FHS nor openSUSE allow prefix/libexec. Let's default to prefix/lib.
> |
> | if test "$libexecdir" = '${exec_prefix}/libexec' ; then
> |       libexecdir='${exec_prefix}/lib'
> | fi
> ...
> 
> However, in libgomp libgomp/plugin/configfrag.ac we hardcode libexec:
> ...
>     # Configure additional search paths.
>     if test x"$tgt_dir" != x; then
>       offload_additional_options="$offload_additional_options \
>         -B$tgt_dir/libexec/gcc/\$(target_alias)/\$(gcc_version) \
> 	-B$tgt_dir/bin"
> ...
> 
> Fix this by using /$(libexecdir:\$(exec_prefix)/%=%)/ instead of /libexec/.
> 
> Tested on x86_64-linux with nvptx accelerator.
> 
> OK for trunk?

OK in principle, but I have no idea on how portable

$(libexecdir:\$(exec_prefix)/%=%)

is going to be?  We should aim for POSIX shell compatibility here,
whatever that exactly is.

Richard.

> Thanks,
> - Tom
> 
> [libgomp, testsuite] Fix hardcoded libexec in plugin/configfrag.ac
> 
> libgomp/ChangeLog:
> 
> 2022-03-28  Tom de Vries  <tdevries@suse.de>
> 
> 	* plugin/configfrag.ac: Use /$(libexecdir:\$(exec_prefix)/%=%)/
> 	instead of /libexec/.
> 	* configure: Regenerate.
> 
> ---
>  libgomp/configure            | 2 +-
>  libgomp/plugin/configfrag.ac | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/libgomp/configure b/libgomp/configure
> index a73a6d44003..bdbe3d142d1 100755
> --- a/libgomp/configure
> +++ b/libgomp/configure
> @@ -15419,7 +15419,7 @@ rm -f core conftest.err conftest.$ac_objext \
>      fi
>      # Configure additional search paths.
>      if test x"$tgt_dir" != x; then
> -      offload_additional_options="$offload_additional_options -B$tgt_dir/libexec/gcc/\$(target_alias)/\$(gcc_version) -B$tgt_dir/bin"
> +      offload_additional_options="$offload_additional_options -B$tgt_dir/\$(libexecdir:\$(exec_prefix)/%=%)/gcc/\$(target_alias)/\$(gcc_version) -B$tgt_dir/bin"
>        offload_additional_lib_paths="$offload_additional_lib_paths:$tgt_dir/lib64:$tgt_dir/lib:$tgt_dir/lib32"
>      else
>        offload_additional_options="$offload_additional_options -B\$(libexecdir)/gcc/\$(target_alias)/\$(gcc_version) -B\$(bindir)"
> diff --git a/libgomp/plugin/configfrag.ac b/libgomp/plugin/configfrag.ac
> index da573bd8387..9f9d0a7f08c 100644
> --- a/libgomp/plugin/configfrag.ac
> +++ b/libgomp/plugin/configfrag.ac
> @@ -254,7 +254,7 @@ if test x"$enable_offload_targets" != x; then
>      fi
>      # Configure additional search paths.
>      if test x"$tgt_dir" != x; then
> -      offload_additional_options="$offload_additional_options -B$tgt_dir/libexec/gcc/\$(target_alias)/\$(gcc_version) -B$tgt_dir/bin"
> +      offload_additional_options="$offload_additional_options -B$tgt_dir/\$(libexecdir:\$(exec_prefix)/%=%)/gcc/\$(target_alias)/\$(gcc_version) -B$tgt_dir/bin"
>        offload_additional_lib_paths="$offload_additional_lib_paths:$tgt_dir/lib64:$tgt_dir/lib:$tgt_dir/lib32"
>      else
>        offload_additional_options="$offload_additional_options -B\$(libexecdir)/gcc/\$(target_alias)/\$(gcc_version) -B\$(bindir)"
>
  
Tom de Vries March 28, 2022, 11:54 a.m. UTC | #2
On 3/28/22 10:49, Richard Biener wrote:
> On Mon, 28 Mar 2022, Tom de Vries wrote:
> 
>> Hi,
>>
>> When building an nvptx offloading configuration on openSUSE Leap 15.3, the
>> site script /usr/share/site/x86_64-unknown-linux-gnu is activated, setting
>> libexecdir to ${exec_prefix}/lib rather than ${exec_prefix}/libexec:
>> ...
>> | # If user did not specify libexecdir, set the correct target:
>> | # Nor FHS nor openSUSE allow prefix/libexec. Let's default to prefix/lib.
>> |
>> | if test "$libexecdir" = '${exec_prefix}/libexec' ; then
>> |       libexecdir='${exec_prefix}/lib'
>> | fi
>> ...
>>
>> However, in libgomp libgomp/plugin/configfrag.ac we hardcode libexec:
>> ...
>>      # Configure additional search paths.
>>      if test x"$tgt_dir" != x; then
>>        offload_additional_options="$offload_additional_options \
>>          -B$tgt_dir/libexec/gcc/\$(target_alias)/\$(gcc_version) \
>> 	-B$tgt_dir/bin"
>> ...
>>
>> Fix this by using /$(libexecdir:\$(exec_prefix)/%=%)/ instead of /libexec/.
>>
>> Tested on x86_64-linux with nvptx accelerator.
>>
>> OK for trunk?
> 
> OK in principle, but I have no idea on how portable
> 
> $(libexecdir:\$(exec_prefix)/%=%)
> 
> is going to be?  We should aim for POSIX shell compatibility here,
> whatever that exactly is.

I tried to avoid this construct by using shell variable substitution, 
but then I end up using $(shell ...) instead, I'm not sure if that is 
any better.

Thanks,
- Tom
  
Andreas Schwab March 28, 2022, noon UTC | #3
On Mär 28 2022, Richard Biener via Gcc-patches wrote:

> OK in principle, but I have no idea on how portable
>
> $(libexecdir:\$(exec_prefix)/%=%)
>
> is going to be?

We already require GNU make, don't we?

> We should aim for POSIX shell compatibility here, whatever that
> exactly is.

It's not a shell construct.
  
Richard Biener March 28, 2022, 12:04 p.m. UTC | #4
On Mon, 28 Mar 2022, Andreas Schwab wrote:

> On Mär 28 2022, Richard Biener via Gcc-patches wrote:
> 
> > OK in principle, but I have no idea on how portable
> >
> > $(libexecdir:\$(exec_prefix)/%=%)
> >
> > is going to be?
> 
> We already require GNU make, don't we?
> 
> > We should aim for POSIX shell compatibility here, whatever that
> > exactly is.
> 
> It's not a shell construct.

Ah, it's only substituted into Makefile.in - in that case yes,
we already require GNU make.  If it's evaluated by that and not
a subshell of it then it should be indeed fine.

Richard.
  
Tom de Vries March 28, 2022, 12:19 p.m. UTC | #5
On 3/28/22 14:04, Richard Biener wrote:
> On Mon, 28 Mar 2022, Andreas Schwab wrote:
> 
>> On Mär 28 2022, Richard Biener via Gcc-patches wrote:
>>
>>> OK in principle, but I have no idea on how portable
>>>
>>> $(libexecdir:\$(exec_prefix)/%=%)
>>>
>>> is going to be?
>>
>> We already require GNU make, don't we?
>>
>>> We should aim for POSIX shell compatibility here, whatever that
>>> exactly is.
>>
>> It's not a shell construct.
> 
> Ah, it's only substituted into Makefile.in - in that case yes,
> we already require GNU make.  If it's evaluated by that and not
> a subshell of it then it should be indeed fine.

Ack, then committed the first version.

Thanks,
- Tom
  

Patch

diff --git a/libgomp/configure b/libgomp/configure
index a73a6d44003..bdbe3d142d1 100755
--- a/libgomp/configure
+++ b/libgomp/configure
@@ -15419,7 +15419,7 @@  rm -f core conftest.err conftest.$ac_objext \
     fi
     # Configure additional search paths.
     if test x"$tgt_dir" != x; then
-      offload_additional_options="$offload_additional_options -B$tgt_dir/libexec/gcc/\$(target_alias)/\$(gcc_version) -B$tgt_dir/bin"
+      offload_additional_options="$offload_additional_options -B$tgt_dir/\$(libexecdir:\$(exec_prefix)/%=%)/gcc/\$(target_alias)/\$(gcc_version) -B$tgt_dir/bin"
       offload_additional_lib_paths="$offload_additional_lib_paths:$tgt_dir/lib64:$tgt_dir/lib:$tgt_dir/lib32"
     else
       offload_additional_options="$offload_additional_options -B\$(libexecdir)/gcc/\$(target_alias)/\$(gcc_version) -B\$(bindir)"
diff --git a/libgomp/plugin/configfrag.ac b/libgomp/plugin/configfrag.ac
index da573bd8387..9f9d0a7f08c 100644
--- a/libgomp/plugin/configfrag.ac
+++ b/libgomp/plugin/configfrag.ac
@@ -254,7 +254,7 @@  if test x"$enable_offload_targets" != x; then
     fi
     # Configure additional search paths.
     if test x"$tgt_dir" != x; then
-      offload_additional_options="$offload_additional_options -B$tgt_dir/libexec/gcc/\$(target_alias)/\$(gcc_version) -B$tgt_dir/bin"
+      offload_additional_options="$offload_additional_options -B$tgt_dir/\$(libexecdir:\$(exec_prefix)/%=%)/gcc/\$(target_alias)/\$(gcc_version) -B$tgt_dir/bin"
       offload_additional_lib_paths="$offload_additional_lib_paths:$tgt_dir/lib64:$tgt_dir/lib:$tgt_dir/lib32"
     else
       offload_additional_options="$offload_additional_options -B\$(libexecdir)/gcc/\$(target_alias)/\$(gcc_version) -B\$(bindir)"