test: Don't add current build directory to include path
diff mbox series

Message ID 20240504140237.19117-1-laurent.pinchart@ideasonboard.com
State Accepted
Commit 6cd849125888782ed12af87f80a1ee9c43bcf553
Headers show
Series
  • test: Don't add current build directory to include path
Related show

Commit Message

Laurent Pinchart May 4, 2024, 2:02 p.m. UTC 
Meson adds the current source and build directory to the include path by
default. This causes a namespace clash in tests when using C++20, as the
Span class test is compiled into a binary named 'span', which then gets
included by source code through indirect '#include <span>' directives.
Unsurprisingly, the compiler doesn't react happily when fed binary data.

We could work around the problem by renaming the test executable, but
disabling the implicit inclusion of the local directory is a more
generic solution that will avoid similar issues in the future.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
This patch makes libcamera build with cpp_std=c++20. Once merged, I will
add a CI test to avoid introduction C++20 compilation breakages, paving
the way to move to the newer language version once the ecosystem will be
ready.
---
 test/meson.build | 3 +++
 1 file changed, 3 insertions(+)


base-commit: bf4695266bfca8cc21bcf10a3281e874ebce0d27

Comments

Kieran Bingham May 7, 2024, 4:15 p.m. UTC | #1 
Quoting Laurent Pinchart (2024-05-04 15:02:37)
> Meson adds the current source and build directory to the include path by
> default. This causes a namespace clash in tests when using C++20, as the
> Span class test is compiled into a binary named 'span', which then gets
> included by source code through indirect '#include <span>' directives.
> Unsurprisingly, the compiler doesn't react happily when fed binary data.

Ouch, so the #include is pulling in a locally build binary... I thought
include < > would only search the ...

Re-reads title ...
Re-reads first paragraph.

Oh :-S

 
> We could work around the problem by renaming the test executable, but
> disabling the implicit inclusion of the local directory is a more
> generic solution that will avoid similar issues in the future.
> 

Agreed.


> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> This patch makes libcamera build with cpp_std=c++20. Once merged, I will
> add a CI test to avoid introduction C++20 compilation breakages, paving
> the way to move to the newer language version once the ecosystem will be
> ready.

That sounds good.

My only 'small' comment is should we enable this setting globally. I
would not have expected "#include <file>" to include ./file ...

But maybe that's more effort than it's worth anyway, and it might not be
easy to configure this globally in meson.

So...

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> ---
>  test/meson.build | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/test/meson.build b/test/meson.build
> index 8b6057d4e800..5ed052ed62c8 100644
> --- a/test/meson.build
> +++ b/test/meson.build
> @@ -89,6 +89,7 @@ foreach test : public_tests
>  
>      exe = executable(test['name'], test['sources'],
>                       dependencies : deps,
> +                     implicit_include_directories : false,
>                       link_with : test_libraries,
>                       include_directories : test_includes_public)
>  
> @@ -103,6 +104,7 @@ foreach test : internal_tests
>  
>      exe = executable(test['name'], test['sources'],
>                       dependencies : deps,
> +                     implicit_include_directories : false,
>                       link_with : test_libraries,
>                       include_directories : test_includes_internal)
>  
> @@ -117,6 +119,7 @@ foreach test : internal_non_parallel_tests
>  
>      exe = executable(test['name'], test['sources'],
>                       dependencies : deps,
> +                     implicit_include_directories : false,
>                       link_with : test_libraries,
>                       include_directories : test_includes_internal)
>  
> 
> base-commit: bf4695266bfca8cc21bcf10a3281e874ebce0d27
> -- 
> Regards,
> 
> Laurent Pinchart
>
Laurent Pinchart May 7, 2024, 4:20 p.m. UTC | #2 
On Tue, May 07, 2024 at 05:15:54PM +0100, Kieran Bingham wrote:
> Quoting Laurent Pinchart (2024-05-04 15:02:37)
> > Meson adds the current source and build directory to the include path by
> > default. This causes a namespace clash in tests when using C++20, as the
> > Span class test is compiled into a binary named 'span', which then gets
> > included by source code through indirect '#include <span>' directives.
> > Unsurprisingly, the compiler doesn't react happily when fed binary data.
> 
> Ouch, so the #include is pulling in a locally build binary... I thought
> include < > would only search the ...
> 
> Re-reads title ...
> Re-reads first paragraph.
> 
> Oh :-S
> 
>  
> > We could work around the problem by renaming the test executable, but
> > disabling the implicit inclusion of the local directory is a more
> > generic solution that will avoid similar issues in the future.
> > 
> 
> Agreed.
> 
> 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > This patch makes libcamera build with cpp_std=c++20. Once merged, I will
> > add a CI test to avoid introduction C++20 compilation breakages, paving
> > the way to move to the newer language version once the ecosystem will be
> > ready.
> 
> That sounds good.
> 
> My only 'small' comment is should we enable this setting globally. I
> would not have expected "#include <file>" to include ./file ...

That would probably work, but I didn't see a way to do this globally at
the project level.

> But maybe that's more effort than it's worth anyway, and it might not be
> easy to configure this globally in meson.
> 
> So...
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> > ---
> >  test/meson.build | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/test/meson.build b/test/meson.build
> > index 8b6057d4e800..5ed052ed62c8 100644
> > --- a/test/meson.build
> > +++ b/test/meson.build
> > @@ -89,6 +89,7 @@ foreach test : public_tests
> >  
> >      exe = executable(test['name'], test['sources'],
> >                       dependencies : deps,
> > +                     implicit_include_directories : false,
> >                       link_with : test_libraries,
> >                       include_directories : test_includes_public)
> >  
> > @@ -103,6 +104,7 @@ foreach test : internal_tests
> >  
> >      exe = executable(test['name'], test['sources'],
> >                       dependencies : deps,
> > +                     implicit_include_directories : false,
> >                       link_with : test_libraries,
> >                       include_directories : test_includes_internal)
> >  
> > @@ -117,6 +119,7 @@ foreach test : internal_non_parallel_tests
> >  
> >      exe = executable(test['name'], test['sources'],
> >                       dependencies : deps,
> > +                     implicit_include_directories : false,
> >                       link_with : test_libraries,
> >                       include_directories : test_includes_internal)
> >  
> > 
> > base-commit: bf4695266bfca8cc21bcf10a3281e874ebce0d27
Jacopo Mondi May 7, 2024, 4:25 p.m. UTC | #3 
Hi Laurent

On Sat, May 04, 2024 at 05:02:37PM GMT, Laurent Pinchart wrote:
> Meson adds the current source and build directory to the include path by
> default. This causes a namespace clash in tests when using C++20, as the
> Span class test is compiled into a binary named 'span', which then gets
> included by source code through indirect '#include <span>' directives.
> Unsurprisingly, the compiler doesn't react happily when fed binary data.
>
> We could work around the problem by renaming the test executable, but
> disabling the implicit inclusion of the local directory is a more
> generic solution that will avoid similar issues in the future.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
  j

> ---
> This patch makes libcamera build with cpp_std=c++20. Once merged, I will
> add a CI test to avoid introduction C++20 compilation breakages, paving
> the way to move to the newer language version once the ecosystem will be
> ready.
> ---
>  test/meson.build | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/test/meson.build b/test/meson.build
> index 8b6057d4e800..5ed052ed62c8 100644
> --- a/test/meson.build
> +++ b/test/meson.build
> @@ -89,6 +89,7 @@ foreach test : public_tests
>
>      exe = executable(test['name'], test['sources'],
>                       dependencies : deps,
> +                     implicit_include_directories : false,
>                       link_with : test_libraries,
>                       include_directories : test_includes_public)
>
> @@ -103,6 +104,7 @@ foreach test : internal_tests
>
>      exe = executable(test['name'], test['sources'],
>                       dependencies : deps,
> +                     implicit_include_directories : false,
>                       link_with : test_libraries,
>                       include_directories : test_includes_internal)
>
> @@ -117,6 +119,7 @@ foreach test : internal_non_parallel_tests
>
>      exe = executable(test['name'], test['sources'],
>                       dependencies : deps,
> +                     implicit_include_directories : false,
>                       link_with : test_libraries,
>                       include_directories : test_includes_internal)
>
>
> base-commit: bf4695266bfca8cc21bcf10a3281e874ebce0d27
> --
> Regards,
>
> Laurent Pinchart
>

Patch
diff mbox series 

diff --git a/test/meson.build b/test/meson.build
index 8b6057d4e800..5ed052ed62c8 100644
--- a/test/meson.build
+++ b/test/meson.build
@@ -89,6 +89,7 @@  foreach test : public_tests
 
     exe = executable(test['name'], test['sources'],
                      dependencies : deps,
+                     implicit_include_directories : false,
                      link_with : test_libraries,
                      include_directories : test_includes_public)
 
@@ -103,6 +104,7 @@  foreach test : internal_tests
 
     exe = executable(test['name'], test['sources'],
                      dependencies : deps,
+                     implicit_include_directories : false,
                      link_with : test_libraries,
                      include_directories : test_includes_internal)
 
@@ -117,6 +119,7 @@  foreach test : internal_non_parallel_tests
 
     exe = executable(test['name'], test['sources'],
                      dependencies : deps,
+                     implicit_include_directories : false,
                      link_with : test_libraries,
                      include_directories : test_includes_internal)