[v2,1/2] build: fix list_dir_globs failure in MSYS2

Message ID TYAP286MB0300BB597DDFC33CF7D2BA70CCF9A@TYAP286MB0300.JPNP286.PROD.OUTLOOK.COM (mailing list archive)
State New
Delegated to: Thomas Monjalon
Headers
Series [v2,1/2] build: fix list_dir_globs failure in MSYS2 |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/intel-Functional success Functional PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-compile-amd64-testing success Testing PASS
ci/iol-compile-arm64-testing success Testing PASS
ci/iol-unit-amd64-testing success Testing PASS
ci/iol-unit-arm64-testing success Testing PASS
ci/iol-sample-apps-testing success Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS

Commit Message

Ric Li Sept. 20, 2023, 2:18 p.m. UTC
When running 'meson setup' on Windows with MSYS2,
"list-dir-globs.py * failed with status 1".

Avoid using globbing to get components for app build
since they are already listed in the meson file.

Signed-off-by: Ric Li <ricmli@outlook.com>
---
 app/meson.build | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)
  

Comments

Bruce Richardson Sept. 20, 2023, 3:07 p.m. UTC | #1
On Wed, Sep 20, 2023 at 10:18:45PM +0800, Ric Li wrote:
> When running 'meson setup' on Windows with MSYS2,
> "list-dir-globs.py * failed with status 1".
> 
> Avoid using globbing to get components for app build
> since they are already listed in the meson file.
> 
> Signed-off-by: Ric Li <ricmli@outlook.com>
> ---

Acked-by: Bruce Richardson <bruce.richardson@intel.com>
  
Thomas Monjalon Oct. 11, 2023, 3:27 p.m. UTC | #2
20/09/2023 16:18, Ric Li:
> When running 'meson setup' on Windows with MSYS2,
> "list-dir-globs.py * failed with status 1".

We don't know why it is failing?
What about other usages of list_dir_globs in drivers and lib?

> Avoid using globbing to get components for app build
> since they are already listed in the meson file.

I don't understand the logic.

> +disable_apps = ',' + get_option('disable_apps')
> +disable_apps = run_command(list_dir_globs, disable_apps, check: true).stdout().split()

This could fail.

> +
> +enable_apps = ',' + get_option('enable_apps')
> +enable_apps = run_command(list_dir_globs, enable_apps, check: true).stdout().split()
> +if enable_apps.length() == 0
> +    enable_apps = apps
> +endif

If nothing is enabled, we enable all?
  
Bruce Richardson Oct. 11, 2023, 3:34 p.m. UTC | #3
On Wed, Oct 11, 2023 at 05:27:22PM +0200, Thomas Monjalon wrote:
> 20/09/2023 16:18, Ric Li:
> > When running 'meson setup' on Windows with MSYS2,
> > "list-dir-globs.py * failed with status 1".
> 
> We don't know why it is failing?
> What about other usages of list_dir_globs in drivers and lib?
> 
> > Avoid using globbing to get components for app build
> > since they are already listed in the meson file.
> 
> I don't understand the logic.
> 
> > +disable_apps = ',' + get_option('disable_apps')
> > +disable_apps = run_command(list_dir_globs, disable_apps, check: true).stdout().split()
> 
> This could fail.
> 
> > +
> > +enable_apps = ',' + get_option('enable_apps')
> > +enable_apps = run_command(list_dir_globs, enable_apps, check: true).stdout().split()
> > +if enable_apps.length() == 0
> > +    enable_apps = apps
> > +endif
> 
> If nothing is enabled, we enable all?
> 
Yes, if the enable_apps list is empty we should enable everything.
However, on reviewing the v2, I missed the fact that this patch is
removing the expansion of the disable_apps value.

Given your comment, this check can probably also be improved by checking
the get_option('enable_apps') length, rather than the expanded version.

/Bruce
  
Ric Li Oct. 24, 2023, 4:08 p.m. UTC | #4
On 2023/10/11 23:34, Bruce Richardson wrote:
> On Wed, Oct 11, 2023 at 05:27:22PM +0200, Thomas Monjalon wrote:
>> 20/09/2023 16:18, Ric Li:
>>> When running 'meson setup' on Windows with MSYS2,
>>> "list-dir-globs.py * failed with status 1".
>>
>> We don't know why it is failing?
>> What about other usages of list_dir_globs in drivers and lib?

Looks lile MSYS2 shell expands this wildcard automatically
before passing it to the child process.

I print the args in list-dir-globs.py and found that args are
the expanded dir names, so the len(sys.argv) is larger than 2,
which makes this script fail. The '*/*' arg in drivers/meson.build
works well just as expected, and no '*' used in lib.

This is from MSYS2 documentation:
"Windows programs parse the command line themselves, it isn't parsed for them by
the calling process, as on Linux. This means that if wildcards (glob patterns) are
to be accepted by the program, it has to be able to expand them somehow. MinGW-w64
supplies the correct start-up code, so it happens automatically, in a manner
compatible with MSVC-compiled programs. If undesirable, the behavior can be
disabled at program build."

I think this fix is not needed if we can find a way to disable the auto-expanding
behaviour of the MSYS2 program. I've tried the runtime way by setting
"MSYS=noglob" envvar but not working here...

>>
>>> Avoid using globbing to get components for app build
>>> since they are already listed in the meson file.
>>
>> I don't understand the logic.
>>
>>> +disable_apps = ',' + get_option('disable_apps')
>>> +disable_apps = run_command(list_dir_globs, disable_apps, check: true).stdout().split()
>>
>> This could fail.>>
>>> +
>>> +enable_apps = ',' + get_option('enable_apps')
>>> +enable_apps = run_command(list_dir_globs, enable_apps, check: true).stdout().split()
>>> +if enable_apps.length() == 0
>>> +    enable_apps = apps
>>> +endif
>>
>> If nothing is enabled, we enable all?
>>
> Yes, if the enable_apps list is empty we should enable everything.
> However, on reviewing the v2, I missed the fact that this patch is
> removing the expansion of the disable_apps value.> 
> Given your comment, this check can probably also be improved by checking
> the get_option('enable_apps') length, rather than the expanded version.
> 
> /Bruce
  
Thomas Monjalon Feb. 18, 2024, 1:45 p.m. UTC | #5
Any update?
Would you like to provide a v3?


24/10/2023 18:08, Ric Li:
> 
> On 2023/10/11 23:34, Bruce Richardson wrote:
> > On Wed, Oct 11, 2023 at 05:27:22PM +0200, Thomas Monjalon wrote:
> >> 20/09/2023 16:18, Ric Li:
> >>> When running 'meson setup' on Windows with MSYS2,
> >>> "list-dir-globs.py * failed with status 1".
> >>
> >> We don't know why it is failing?
> >> What about other usages of list_dir_globs in drivers and lib?
> 
> Looks lile MSYS2 shell expands this wildcard automatically
> before passing it to the child process.
> 
> I print the args in list-dir-globs.py and found that args are
> the expanded dir names, so the len(sys.argv) is larger than 2,
> which makes this script fail. The '*/*' arg in drivers/meson.build
> works well just as expected, and no '*' used in lib.
> 
> This is from MSYS2 documentation:
> "Windows programs parse the command line themselves, it isn't parsed for them by
> the calling process, as on Linux. This means that if wildcards (glob patterns) are
> to be accepted by the program, it has to be able to expand them somehow. MinGW-w64
> supplies the correct start-up code, so it happens automatically, in a manner
> compatible with MSVC-compiled programs. If undesirable, the behavior can be
> disabled at program build."
> 
> I think this fix is not needed if we can find a way to disable the auto-expanding
> behaviour of the MSYS2 program. I've tried the runtime way by setting
> "MSYS=noglob" envvar but not working here...
> 
> >>
> >>> Avoid using globbing to get components for app build
> >>> since they are already listed in the meson file.
> >>
> >> I don't understand the logic.
> >>
> >>> +disable_apps = ',' + get_option('disable_apps')
> >>> +disable_apps = run_command(list_dir_globs, disable_apps, check: true).stdout().split()
> >>
> >> This could fail.>>
> >>> +
> >>> +enable_apps = ',' + get_option('enable_apps')
> >>> +enable_apps = run_command(list_dir_globs, enable_apps, check: true).stdout().split()
> >>> +if enable_apps.length() == 0
> >>> +    enable_apps = apps
> >>> +endif
> >>
> >> If nothing is enabled, we enable all?
> >>
> > Yes, if the enable_apps list is empty we should enable everything.
> > However, on reviewing the v2, I missed the fact that this patch is
> > removing the expansion of the disable_apps value.> 
> > Given your comment, this check can probably also be improved by checking
> > the get_option('enable_apps') length, rather than the expanded version.
> > 
> > /Bruce
>
  

Patch

diff --git a/app/meson.build b/app/meson.build
index e4bf5c531c..75ac37bef9 100644
--- a/app/meson.build
+++ b/app/meson.build
@@ -5,15 +5,6 @@  if is_ms_compiler
     subdir_done()
 endif
 
-disable_apps = ',' + get_option('disable_apps')
-disable_apps = run_command(list_dir_globs, disable_apps, check: true).stdout().split()
-
-enable_apps = ',' + get_option('enable_apps')
-enable_apps = run_command(list_dir_globs, enable_apps, check: true).stdout().split()
-if enable_apps.length() == 0
-    enable_apps = run_command(list_dir_globs, '*', check: true).stdout().split()
-endif
-
 apps = [
         'dumpcap',
         'pdump',
@@ -41,6 +32,15 @@  if get_option('tests')
     apps += 'test'
 endif
 
+disable_apps = ',' + get_option('disable_apps')
+disable_apps = run_command(list_dir_globs, disable_apps, check: true).stdout().split()
+
+enable_apps = ',' + get_option('enable_apps')
+enable_apps = run_command(list_dir_globs, enable_apps, check: true).stdout().split()
+if enable_apps.length() == 0
+    enable_apps = apps
+endif
+
 default_cflags = machine_args + ['-DALLOW_EXPERIMENTAL_API']
 default_ldflags = []
 if get_option('default_library') == 'static' and not is_windows