diff mbox series

[v3] test: remove meson dependency on /proc file

Message ID 20200410102951.1398001-1-thomas@monjalon.net (mailing list archive)
State Superseded, archived
Headers
Series [v3] test: remove meson dependency on /proc file |

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/travis-robot success Travis build: passed
ci/iol-testing success Testing PASS
ci/Intel-compilation success Compilation OK

Commit Message

Thomas Monjalon April 10, 2020, 10:29 a.m. UTC 
  Meson is detecting the path /proc/sys/vm/nr_hugepages in the call to cat
in app/test/meson.build and then adding it as a build dependency.
This causes build loop if the timestamp of this file keeps changing.

It is fixed by hiding hugepage check in a shell script.

Fixes: 77784ef0fba8 ("test: allow no-huge mode for fast-tests")

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
Tested-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
Reviewed-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
Acked-by: Aaron Conole <aconole@redhat.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
---

v2: use variable as pointed by Lukasz
v3: avoid TOCTOU issue as suggested by David
	I'm afraid it requires to re-confirm every reviews above.

---
 MAINTAINERS              | 1 +
 app/test/has-hugepage.sh | 9 +++++++++
 app/test/meson.build     | 8 ++------
 3 files changed, 12 insertions(+), 6 deletions(-)
 create mode 100755 app/test/has-hugepage.sh

Comments

Bruce Richardson April 10, 2020, 10:42 a.m. UTC | #1 
On Fri, Apr 10, 2020 at 12:29:50PM +0200, Thomas Monjalon wrote:
> Meson is detecting the path /proc/sys/vm/nr_hugepages in the call to cat
> in app/test/meson.build and then adding it as a build dependency.
> This causes build loop if the timestamp of this file keeps changing.
> 
> It is fixed by hiding hugepage check in a shell script.
> 
> Fixes: 77784ef0fba8 ("test: allow no-huge mode for fast-tests")
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> Tested-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
> Reviewed-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
> Acked-by: Aaron Conole <aconole@redhat.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> ---
> 
> v2: use variable as pointed by Lukasz
> v3: avoid TOCTOU issue as suggested by David
> 	I'm afraid it requires to re-confirm every reviews above.
> 
> ---
>  MAINTAINERS              | 1 +
>  app/test/has-hugepage.sh | 9 +++++++++
>  app/test/meson.build     | 8 ++------
>  3 files changed, 12 insertions(+), 6 deletions(-)
>  create mode 100755 app/test/has-hugepage.sh
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 4800f6884a..aa619b6762 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1471,6 +1471,7 @@ F: app/test/Makefile
>  F: app/test/autotest*
>  F: app/test/commands.c
>  F: app/test/get-coremask.sh
> +F: app/test/has-hugepage.sh
>  F: app/test/packet_burst_generator.c
>  F: app/test/packet_burst_generator.h
>  F: app/test/process.h
> diff --git a/app/test/has-hugepage.sh b/app/test/has-hugepage.sh
> new file mode 100755
> index 0000000000..865e66cddd
> --- /dev/null
> +++ b/app/test/has-hugepage.sh
> @@ -0,0 +1,9 @@
> +#! /bin/sh
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright 2020 Mellanox Technologies, Ltd
> +
> +if [ "$(uname)" = "Linux" ] ; then
> +	cat /proc/sys/vm/nr_hugepages || echo 0
> +else
> +	echo 0
> +fi
> diff --git a/app/test/meson.build b/app/test/meson.build
> index 351d29cb65..542408d614 100644
> --- a/app/test/meson.build
> +++ b/app/test/meson.build
> @@ -399,12 +399,8 @@ dpdk_test = executable('dpdk-test',
>  
>  has_hugepage = true
>  if is_linux

Since you check for linux in the script, you can drop these two lines, and
then just replace the whole branch with:

has_hugepages = run_command('has-hugepage.sh').stdout().strip() != '0'

> -	check_hugepage = run_command('cat',
> -				     '/proc/sys/vm/nr_hugepages')
> -	if (check_hugepage.returncode() != 0 or
> -	    check_hugepage.stdout().strip() == '0')
> -		has_hugepage = false
> -	endif
> +	check_hugepage = find_program('has-hugepage.sh')
> +	has_hugepage = run_command(check_hugepage).stdout().strip() != '0'
>  endif
>  message('hugepage availability: @0@'.format(has_hugepage))
>  
> -- 
> 2.26.0
>
Thomas Monjalon April 10, 2020, 12:27 p.m. UTC | #2 
10/04/2020 12:42, Bruce Richardson:
> On Fri, Apr 10, 2020 at 12:29:50PM +0200, Thomas Monjalon wrote:
> > --- a/app/test/meson.build
> > +++ b/app/test/meson.build
> >  has_hugepage = true
> >  if is_linux
> 
> Since you check for linux in the script, you can drop these two lines,

The issue is for Windows.
I am not sure how we will skip shell scripts
when adding Windows support for this application.
So there are two options now:
	a) remove Linux check before calling the script and ignore Windows support for now
	b) keep Linux check without knowing whether it will be useful for Windows support

We vote a?
Bruce Richardson April 10, 2020, 1:25 p.m. UTC | #3 
On Fri, Apr 10, 2020 at 02:27:30PM +0200, Thomas Monjalon wrote:
> 10/04/2020 12:42, Bruce Richardson:
> > On Fri, Apr 10, 2020 at 12:29:50PM +0200, Thomas Monjalon wrote:
> > > --- a/app/test/meson.build
> > > +++ b/app/test/meson.build
> > >  has_hugepage = true
> > >  if is_linux
> > 
> > Since you check for linux in the script, you can drop these two lines,
> 
> The issue is for Windows.
> I am not sure how we will skip shell scripts
> when adding Windows support for this application.
> So there are two options now:
> 	a) remove Linux check before calling the script and ignore Windows support for now
> 	b) keep Linux check without knowing whether it will be useful for Windows support
> 
> We vote a?
> 
c) Write all such scripts in python, to allow them to run everywhere. :-)

Given that windows is the problem, having the is_linux check in the
meson.build file makes most sense - in which case I don't think we need the
check for linux in the script.

/Bruce
Aaron Conole April 10, 2020, 2:05 p.m. UTC | #4 
Thomas Monjalon <thomas@monjalon.net> writes:

> Meson is detecting the path /proc/sys/vm/nr_hugepages in the call to cat
> in app/test/meson.build and then adding it as a build dependency.
> This causes build loop if the timestamp of this file keeps changing.
>
> It is fixed by hiding hugepage check in a shell script.
>
> Fixes: 77784ef0fba8 ("test: allow no-huge mode for fast-tests")
>
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> Tested-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
> Reviewed-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
> Acked-by: Aaron Conole <aconole@redhat.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> ---
>
> v2: use variable as pointed by Lukasz
> v3: avoid TOCTOU issue as suggested by David
> 	I'm afraid it requires to re-confirm every reviews above.

Still looks good to me.
Thomas Monjalon April 10, 2020, 2:41 p.m. UTC | #5 
10/04/2020 15:25, Bruce Richardson:
> On Fri, Apr 10, 2020 at 02:27:30PM +0200, Thomas Monjalon wrote:
> > 10/04/2020 12:42, Bruce Richardson:
> > > On Fri, Apr 10, 2020 at 12:29:50PM +0200, Thomas Monjalon wrote:
> > > > --- a/app/test/meson.build
> > > > +++ b/app/test/meson.build
> > > >  has_hugepage = true
> > > >  if is_linux
> > > 
> > > Since you check for linux in the script, you can drop these two lines,
> > 
> > The issue is for Windows.
> > I am not sure how we will skip shell scripts
> > when adding Windows support for this application.
> > So there are two options now:
> > 	a) remove Linux check before calling the script and ignore Windows support for now
> > 	b) keep Linux check without knowing whether it will be useful for Windows support
> > 
> > We vote a?
> > 
> c) Write all such scripts in python, to allow them to run everywhere. :-)
> 
> Given that windows is the problem, having the is_linux check in the
> meson.build file makes most sense - in which case I don't think we need the
> check for linux in the script.

Yes, let's make it simple for now.
Lukasz Wojciechowski April 10, 2020, 8:47 p.m. UTC | #6 
W dniu 10.04.2020 o 16:41, Thomas Monjalon pisze:
> 10/04/2020 15:25, Bruce Richardson:
>> On Fri, Apr 10, 2020 at 02:27:30PM +0200, Thomas Monjalon wrote:
>>> 10/04/2020 12:42, Bruce Richardson:
>>>> On Fri, Apr 10, 2020 at 12:29:50PM +0200, Thomas Monjalon wrote:
>>>>> --- a/app/test/meson.build
>>>>> +++ b/app/test/meson.build
>>>>>   has_hugepage = true
>>>>>   if is_linux
>>>> Since you check for linux in the script, you can drop these two lines,
>>> The issue is for Windows.
>>> I am not sure how we will skip shell scripts
>>> when adding Windows support for this application.
>>> So there are two options now:
>>> 	a) remove Linux check before calling the script and ignore Windows support for now
>>> 	b) keep Linux check without knowing whether it will be useful for Windows support
>>>
>>> We vote a?
>>>
>> c) Write all such scripts in python, to allow them to run everywhere. :-)
>>
>> Given that windows is the problem, having the is_linux check in the
>> meson.build file makes most sense - in which case I don't think we need the
>> check for linux in the script.
> Yes, let's make it simple for now.
>

a) is ok as the current meson.build won't work in Windows environment at 
all.
c) would be best, but I guess it's a little bit to much for this patch 
and the whole build system could be adjusted to windows in separate thread.

And I agree with Bruce, that the check for Linux in script is not needed.

>
Stephen Hemminger April 10, 2020, 10:22 p.m. UTC | #7 
On Fri, 10 Apr 2020 22:47:44 +0200
Lukasz Wojciechowski <l.wojciechow@partner.samsung.com> wrote:

> W dniu 10.04.2020 o 16:41, Thomas Monjalon pisze:
> > 10/04/2020 15:25, Bruce Richardson:  
> >> On Fri, Apr 10, 2020 at 02:27:30PM +0200, Thomas Monjalon wrote:  
> >>> 10/04/2020 12:42, Bruce Richardson:  
> >>>> On Fri, Apr 10, 2020 at 12:29:50PM +0200, Thomas Monjalon wrote:  
> >>>>> --- a/app/test/meson.build
> >>>>> +++ b/app/test/meson.build
> >>>>>   has_hugepage = true
> >>>>>   if is_linux  
> >>>> Since you check for linux in the script, you can drop these two lines,  
> >>> The issue is for Windows.
> >>> I am not sure how we will skip shell scripts
> >>> when adding Windows support for this application.
> >>> So there are two options now:
> >>> 	a) remove Linux check before calling the script and ignore Windows support for now
> >>> 	b) keep Linux check without knowing whether it will be useful for Windows support
> >>>
> >>> We vote a?
> >>>  
> >> c) Write all such scripts in python, to allow them to run everywhere. :-)
> >>
> >> Given that windows is the problem, having the is_linux check in the
> >> meson.build file makes most sense - in which case I don't think we need the
> >> check for linux in the script.  
> > Yes, let's make it simple for now.
> >  
> 
> a) is ok as the current meson.build won't work in Windows environment at 
> all.
> c) would be best, but I guess it's a little bit to much for this patch 
> and the whole build system could be adjusted to windows in separate thread.
> 
> And I agree with Bruce, that the check for Linux in script is not needed.
> 
> >  

Building in a container may not give access to /proc.
Thomas Monjalon April 15, 2020, 1:14 p.m. UTC | #8 
10/04/2020 22:47, Lukasz Wojciechowski:
> W dniu 10.04.2020 o 16:41, Thomas Monjalon pisze:
> > 10/04/2020 15:25, Bruce Richardson:
> >> On Fri, Apr 10, 2020 at 02:27:30PM +0200, Thomas Monjalon wrote:
> >>> 10/04/2020 12:42, Bruce Richardson:
> >>>> On Fri, Apr 10, 2020 at 12:29:50PM +0200, Thomas Monjalon wrote:
> >>>>> --- a/app/test/meson.build
> >>>>> +++ b/app/test/meson.build
> >>>>>   has_hugepage = true
> >>>>>   if is_linux
> >>>> Since you check for linux in the script, you can drop these two lines,
> >>> The issue is for Windows.
> >>> I am not sure how we will skip shell scripts
> >>> when adding Windows support for this application.
> >>> So there are two options now:
> >>> 	a) remove Linux check before calling the script and ignore Windows support for now
> >>> 	b) keep Linux check without knowing whether it will be useful for Windows support
> >>>
> >>> We vote a?
> >>>
> >> c) Write all such scripts in python, to allow them to run everywhere. :-)
> >>
> >> Given that windows is the problem, having the is_linux check in the
> >> meson.build file makes most sense - in which case I don't think we need the
> >> check for linux in the script.
> > Yes, let's make it simple for now.
> >
> 
> a) is ok as the current meson.build won't work in Windows environment at 
> all.
> c) would be best, but I guess it's a little bit to much for this patch 
> and the whole build system could be adjusted to windows in separate thread.
> 
> And I agree with Bruce, that the check for Linux in script is not needed.

I decide to go with initial Bruce's suggestion:
check OS in script and remove check from meson.
The reasons:
	- the script is standalone and can be extended for FreeBSD
	- if Windows support is required, converting to python is better
	  than a check in meson
diff mbox series

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index 4800f6884a..aa619b6762 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1471,6 +1471,7 @@  F: app/test/Makefile
 F: app/test/autotest*
 F: app/test/commands.c
 F: app/test/get-coremask.sh
+F: app/test/has-hugepage.sh
 F: app/test/packet_burst_generator.c
 F: app/test/packet_burst_generator.h
 F: app/test/process.h
diff --git a/app/test/has-hugepage.sh b/app/test/has-hugepage.sh
new file mode 100755
index 0000000000..865e66cddd
--- /dev/null
+++ b/app/test/has-hugepage.sh
@@ -0,0 +1,9 @@ 
+#! /bin/sh
+# SPDX-License-Identifier: BSD-3-Clause
+# Copyright 2020 Mellanox Technologies, Ltd
+
+if [ "$(uname)" = "Linux" ] ; then
+	cat /proc/sys/vm/nr_hugepages || echo 0
+else
+	echo 0
+fi
diff --git a/app/test/meson.build b/app/test/meson.build
index 351d29cb65..542408d614 100644
--- a/app/test/meson.build
+++ b/app/test/meson.build
@@ -399,12 +399,8 @@  dpdk_test = executable('dpdk-test',
 
 has_hugepage = true
 if is_linux
-	check_hugepage = run_command('cat',
-				     '/proc/sys/vm/nr_hugepages')
-	if (check_hugepage.returncode() != 0 or
-	    check_hugepage.stdout().strip() == '0')
-		has_hugepage = false
-	endif
+	check_hugepage = find_program('has-hugepage.sh')
+	has_hugepage = run_command(check_hugepage).stdout().strip() != '0'
 endif
 message('hugepage availability: @0@'.format(has_hugepage))