Add Meson build with Werror #448

WillAyd · 2024-04-30T23:28:15Z

This should be a relatively easy way to enforce -Werror in CI; moved the Meson build from a scheduled job to be part of the normal CI runs.

Looks like there are still a few more upstream issues to be fixed before this is ready. Just posting for visibility

WillAyd · 2024-05-01T17:41:34Z

ci/scripts/build-arrow-cpp-minimal.sh

These changes are not permanent - just setting them up to see what we need for a clean build of Arrow.

WillAyd · 2024-05-15T18:14:22Z

src/nanoarrow/buffer_test.cc

@@ -141,9 +141,6 @@ TEST(BufferTest, BufferTestFill) {
  }

  ArrowBufferReset(&buffer);
-


These were removed because they won't compile with -Warray-bounds:

In file included from /usr/include/string.h:535, from /usr/include/c++/11/cstring:42, from ../src/nanoarrow/buffer_test.cc:19: In function ‘void* memcpy(void*, const void*, size_t)’, inlined from ‘void ArrowBufferAppendUnsafe(ArrowBuffer*, const void*, int64_t)’ at ../src/nanoarrow/buffer_inline.h:134:11, inlined from ‘ArrowErrorCode ArrowBufferAppend(ArrowBuffer*, const void*, int64_t)’ at ../src/nanoarrow/buffer_inline.h:143:26, inlined from ‘ArrowErrorCode ArrowBufferAppend(ArrowBuffer*, const void*, int64_t)’ at ../src/nanoarrow/buffer_inline.h:139:30, inlined from ‘virtual void BufferTest_BufferTestError_Test::TestBody()’ at ../src/nanoarrow/buffer_test.cc:174:3: /usr/include/x86_64-linux-gnu/bits/string_fortified.h:29:33: error: ‘void* __builtin_memcpy(void*, const void*, long unsigned int)’ forming offset [48, 9223372036854775806] is out of the bounds [0, 48] of object ‘buffer’ with type ‘ArrowBuffer’ [-Werror=array-bounds] 29 | return __builtin___memcpy_chk (__dest, __src, __len, | ~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~ 30 | __glibc_objsize0 (__dest)); | ~~~~~~~~~~~~~~~~~~~~~~~~~~ ../src/nanoarrow/buffer_test.cc: In member function ‘virtual void BufferTest_BufferTestError_Test::TestBody()’: ../src/nanoarrow/buffer_test.cc:170:22: note: ‘buffer’ declared here 170 | struct ArrowBuffer buffer; | ^~~~~~ cc1plus: all warnings being treated as errors

Would changing them to ArrowBufferReserve() work? (Also fine to remove if neither will compile...testing ENOMEM is hard and we mostly don't test it other places, too).

The BufferTestError test checks for ENOMEM using ArrowBufferResize - do you think it would be better to add the ArrowBufferReserve test there for consistency or do you want to keep within this test?

codecov-commenter · 2024-05-15T18:36:12Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.21%. Comparing base (9f75836) to head (f149708).
Report is 11 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #448      +/-   ##
==========================================
+ Coverage   89.18%   89.21%   +0.02%     
==========================================
  Files          90       90              
  Lines       16102    16230     +128     
==========================================
+ Hits        14361    14479     +118     
- Misses       1741     1751      +10

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

WillAyd · 2024-06-29T21:29:49Z

meson.build

@@ -30,7 +30,7 @@ project(
 )

 if meson.get_compiler('cpp').get_id() == 'gcc' or meson.get_compiler('cpp').get_id() == 'clang'
-  add_project_arguments(['-fvisibility=hidden'], language: 'cpp')
+  add_project_arguments(['-fvisibility=hidden', '-Wno-unused-parameter'], language: 'cpp')


Since these aren't done upstream in Arrow kind of hard to keep things synced; probably just easiest to ignore until we potentially drop the Arrow testing dependency

paleolimbot · 2024-07-15T14:28:37Z

ci/scripts/build-arrow-cpp-minimal.sh

-curl -L "https://www.apache.org/dyn/closer.lua?action=download&filename=arrow/arrow-${ARROW_CPP_VERSION}/apache-arrow-${ARROW_CPP_VERSION}.tar.gz" | \
+curl -L "https://github.com/apache/arrow/archive/refs/heads/main.tar.gz" | \
  tar -zxf -
 mkdir build && cd build
-cmake ../apache-arrow-${ARROW_CPP_VERSION}/cpp \
+cmake ../arrow-main/cpp \


Will the Arrow 17.0.0 release include the upstream changes required to make this work? We can't merge this particular change but we can update the version we use in CI once released.

Don't think so. I fixed a few but they keep popping up, especially after having added metal / ipc support. Can keep trying but I think unless Arrow comprehensively wanted to fix these warnings its going to be hard downstream to keep this clean

An alternate solution may be to treat Arrow as a system include in the build systems so that it doesn't show any warnings from that library. That might allow us to at least keep this clean within nanoarrow

I'll pipe the Arrow C++ removal through in the next few weeks. We've let that dependency complicate quite a lot of things and I've mentioned its hypothetical removal quite a lot of times.

WillAyd

This is going to require #595 going in first, but afterwards I think this might be ready

WillAyd · 2024-08-29T02:12:01Z

meson.build

@@ -161,7 +167,8 @@ if get_option('tests')
  # Similarly code coverage has a built in option users should use instead
  # https://mesonbuild.com/Unit-tests.html#coverage

-  arrow_dep = dependency('arrow')
+  # The system include suppresses compilation warnings from Arrow
+  arrow_dep = dependency('arrow', include_type: 'system')


Rather than chasing down the Arrow failures, I figured we can just ignore them

WillAyd · 2024-08-29T02:14:56Z

meson.build

@@ -35,6 +35,12 @@ project(
 #   add_project_arguments(['-fvisibility=hidden'], language: 'cpp')
 # endif

+cpp = meson.get_compiler('cpp')
+add_project_arguments(
+    cpp.get_supported_arguments(['-Wno-misleading-indentation']),


This warning was generated by the flatcc_generated header. There are a ton of these.

Perhaps there is a way to have it fixed on generation, but for now figure fine to ignore

This had been fixed (#420) but we revendored flatcc and regenerated the header. I think ignoring for now is fine (although I'd like to redo the fix at some point since these errors show up when compiling Python and obscure other actual warnings!)

Do you know which version of flatcc was generated? Meson still uses flatcc as a subproject which is 0.6.1. However, the generated flatcc_generated.h file in the source tree uses some symbols that don't appear in that release.

Should set the meson subproject up to use the same version as was vendored for CMake I think

I think so! We use a git hash right now because @bkietz found a bug/suboptimal behaviour of every flatbuffer implementation except flatcc that forced a change in flatcc. I think using the vendored version is safer but you could probably also use the git hash.

Ended up changing the subproject to use the same git hash. Can relook at the design in a separate PR, but the Meson subproject approach does generally make things much simpler from a build configuration standpoint

WillAyd · 2024-09-04T20:29:19Z

src/nanoarrow/common/array_test.cc

@@ -98,9 +98,11 @@ TEST(ArrayTest, ArrayTestAllocateChildren) {
  ArrowArrayRelease(&array);

  ASSERT_EQ(ArrowArrayInitFromType(&array, NANOARROW_TYPE_STRUCT), NANOARROW_OK);
+#if !defined(__SANITIZE_ADDRESS__)


I did also explicitly add in this code that takes out blocks which are known to be flagged by the sanitizers. This is a bit of "extra credit" but I think generally safer than ignoring a broad class of potential errors.

If we didn't want to use these macros I'm happy to revert. Otherwise, we could look at a suppression file instead of changes to the code:

https://clang.llvm.org/docs/AddressSanitizer.html#issue-suppression

I think this is OK. We should really have a better system for testing out-of-memory anyway (but that is a not-today battle!)

WillAyd added 6 commits April 30, 2024 19:25

Add Meson build with Werror

f3a0ea5

Merge remote-tracking branch 'upstream/main' into meson-ci-build

8d82f7c

Revert array_test.ccc

6c7da11

Try main for now

63c2231

Force new build

80c5923

More updates to CI

d204fb1

WillAyd commented May 1, 2024

View reviewed changes

WillAyd added 5 commits May 15, 2024 13:26

Try clean warning branch

05089a9

fix directory

6a29031

Add explicit gmock dependency

43248be

Remove non-compiling code

4562863

Remove more non-compiling code

0d91852

WillAyd commented May 15, 2024

View reviewed changes

Merge remote-tracking branch 'upstream/main' into meson-ci-build

620d6a6

WillAyd added 7 commits May 15, 2024 17:20

Merge branch 'main' into meson-ci-build

bc6291e

Wreorder fix

9f52be1

Merge remote-tracking branch 'upstream/main' into meson-ci-build

b22e567

Point to Arrow main

d0fab89

URL fix

bf6b20f

path fix

f149708

Merge branch 'main' into meson-ci-build

fd5fb7d

WillAyd marked this pull request as ready for review June 18, 2024 21:51

WillAyd added 5 commits June 18, 2024 18:07

include fix

85dbccf

UNUSED macro

55b98f1

Merge remote-tracking branch 'upstream/main' into meson-ci-build

61675a8

Remove unused parameter warnings

aa0c74b

Merge remote-tracking branch 'upstream/main' into meson-ci-build

2ea48b4

WillAyd commented Jun 29, 2024

View reviewed changes

paleolimbot reviewed Jul 15, 2024

View reviewed changes

Suppress arrow build warnings

0dfa1e0

WillAyd force-pushed the meson-ci-build branch from 20a76be to 0dfa1e0 Compare August 29, 2024 01:58

WillAyd added 2 commits August 28, 2024 22:04

Merge remote-tracking branch 'upstream/main' into meson-ci-build

3472ae2

Fix IPC warnings

40b4cd5

WillAyd commented Aug 29, 2024

View reviewed changes

WillAyd added 3 commits September 4, 2024 15:56

Merge branch 'main' into meson-ci-build

23b3c9c

Fix sanitizers

e1d3306

Clean C Compiler warnings

6ae24e9

WillAyd commented Sep 4, 2024

View reviewed changes

Update flatcc

dddd193

paleolimbot approved these changes Sep 5, 2024

View reviewed changes

paleolimbot merged commit 75ef830 into apache:main Sep 5, 2024
35 checks passed

WillAyd deleted the meson-ci-build branch September 5, 2024 15:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Meson build with Werror #448

Add Meson build with Werror #448

WillAyd commented Apr 30, 2024

WillAyd May 1, 2024

WillAyd May 15, 2024

paleolimbot May 15, 2024

WillAyd May 15, 2024 •

edited

Loading

codecov-commenter commented May 15, 2024 •

edited

Loading

WillAyd Jun 29, 2024

paleolimbot Jul 15, 2024

WillAyd Jul 15, 2024

paleolimbot Jul 15, 2024

WillAyd left a comment

WillAyd Aug 29, 2024

WillAyd Aug 29, 2024

paleolimbot Aug 30, 2024

WillAyd Sep 4, 2024

paleolimbot Sep 5, 2024

WillAyd Sep 5, 2024

WillAyd Sep 4, 2024

paleolimbot Sep 5, 2024

		@@ -141,9 +141,6 @@ TEST(BufferTest, BufferTestFill) {
		}

		ArrowBufferReset(&buffer);

Add Meson build with Werror #448

Add Meson build with Werror #448

Conversation

WillAyd commented Apr 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WillAyd May 15, 2024 • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented May 15, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WillAyd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WillAyd May 15, 2024 •

edited

Loading

codecov-commenter commented May 15, 2024 •

edited

Loading