Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Meson build with Werror #448

Merged
merged 31 commits into from
Sep 5, 2024
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
f3a0ea5
Add Meson build with Werror
WillAyd Apr 30, 2024
8d82f7c
Merge remote-tracking branch 'upstream/main' into meson-ci-build
WillAyd Apr 30, 2024
6c7da11
Revert array_test.ccc
WillAyd Apr 30, 2024
63c2231
Try main for now
WillAyd May 1, 2024
80c5923
Force new build
WillAyd May 1, 2024
d204fb1
More updates to CI
WillAyd May 1, 2024
05089a9
Try clean warning branch
WillAyd May 15, 2024
6a29031
fix directory
WillAyd May 15, 2024
43248be
Add explicit gmock dependency
WillAyd May 15, 2024
4562863
Remove non-compiling code
WillAyd May 15, 2024
0d91852
Remove more non-compiling code
WillAyd May 15, 2024
620d6a6
Merge remote-tracking branch 'upstream/main' into meson-ci-build
WillAyd May 15, 2024
bc6291e
Merge branch 'main' into meson-ci-build
WillAyd May 15, 2024
9f52be1
Wreorder fix
WillAyd May 15, 2024
b22e567
Merge remote-tracking branch 'upstream/main' into meson-ci-build
WillAyd May 24, 2024
d0fab89
Point to Arrow main
WillAyd May 24, 2024
bf6b20f
URL fix
WillAyd May 24, 2024
f149708
path fix
WillAyd May 24, 2024
fd5fb7d
Merge branch 'main' into meson-ci-build
WillAyd Jun 18, 2024
85dbccf
include fix
WillAyd Jun 18, 2024
55b98f1
UNUSED macro
WillAyd Jun 18, 2024
61675a8
Merge remote-tracking branch 'upstream/main' into meson-ci-build
WillAyd Jun 25, 2024
aa0c74b
Remove unused parameter warnings
WillAyd Jun 28, 2024
2ea48b4
Merge remote-tracking branch 'upstream/main' into meson-ci-build
WillAyd Jun 28, 2024
0dfa1e0
Suppress arrow build warnings
WillAyd Aug 29, 2024
3472ae2
Merge remote-tracking branch 'upstream/main' into meson-ci-build
WillAyd Aug 29, 2024
40b4cd5
Fix IPC warnings
WillAyd Aug 29, 2024
23b3c9c
Merge branch 'main' into meson-ci-build
WillAyd Sep 4, 2024
e1d3306
Fix sanitizers
WillAyd Sep 4, 2024
6ae24e9
Clean C Compiler warnings
WillAyd Sep 4, 2024
dddd193
Update flatcc
WillAyd Sep 4, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions .github/workflows/build-and-test.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -122,3 +122,35 @@ jobs:
with:
name: nanoarrow-memcheck
path: build/Testing/Temporary/MemoryChecker.*.log

verify-meson:
name: meson-build
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4

- name: Install system dependencies
run: |
sudo apt-get update && sudo apt-get install -y lcov ninja-build valgrind

- name: Install meson
run: |
python3 -m pip install meson

- name: Cache Arrow C++ Build
id: cache-arrow-build
uses: actions/cache@v4
with:
path: arrow
# Bump the number at the end of this line to force a new Arrow C++ build
key: arrow-${{ runner.os }}-${{ runner.arch }}-2

- name: Build Arrow C++
if: steps.cache-arrow-build.outputs.cache-hit != 'true'
shell: bash
run: |
ci/scripts/build-arrow-cpp-minimal.sh 16.0.0 arrow

- name: Run meson testing script
run: |
PKG_CONFIG_PATH="$(pwd)/arrow/lib/pkgconfig" ci/scripts/build-with-meson.sh
66 changes: 0 additions & 66 deletions .github/workflows/meson-build.yaml

This file was deleted.

4 changes: 2 additions & 2 deletions ci/scripts/build-arrow-cpp-minimal.sh
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Original file line number Diff line number Diff line change
Expand Up @@ -45,10 +45,10 @@ ARROW_CPP_SCRATCH_DIR="arrow-cpp-build-${ARROW_CPP_VERSION}"
mkdir "${ARROW_CPP_SCRATCH_DIR}"
pushd "${ARROW_CPP_SCRATCH_DIR}"

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 \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

-DCMAKE_BUILD_TYPE=Debug \
-DARROW_JEMALLOC=OFF \
-DARROW_SIMD_LEVEL=NONE \
Expand Down
2 changes: 1 addition & 1 deletion ci/scripts/build-with-meson.sh
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ function main() {
meson wrap install nlohmann_json

show_header "Compile project with meson"
meson setup "${SANDBOX_DIR}" --pkg-config-path $PKG_CONFIG_PATH
meson setup "${SANDBOX_DIR}" --pkg-config-path $PKG_CONFIG_PATH -Dwerror=true

pushd "${SANDBOX_DIR}"

Expand Down
4 changes: 2 additions & 2 deletions src/nanoarrow/buffer_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ TEST(BufferTest, BufferTestFill) {

ArrowBufferReset(&buffer);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

@WillAyd WillAyd May 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

EXPECT_EQ(ArrowBufferAppendFill(&buffer, 0, std::numeric_limits<int64_t>::max()),
EXPECT_EQ(ArrowBufferAppendFill(nullptr, 0, std::numeric_limits<int64_t>::max()),
ENOMEM);
}

Expand Down Expand Up @@ -171,7 +171,7 @@ TEST(BufferTest, BufferTestError) {
ArrowBufferInit(&buffer);
EXPECT_EQ(ArrowBufferResize(&buffer, std::numeric_limits<int64_t>::max(), false),
ENOMEM);
EXPECT_EQ(ArrowBufferAppend(&buffer, nullptr, std::numeric_limits<int64_t>::max()),
EXPECT_EQ(ArrowBufferAppend(nullptr, &buffer, std::numeric_limits<int64_t>::max()),
ENOMEM);

ASSERT_EQ(ArrowBufferAppend(&buffer, "abcd", 4), NANOARROW_OK);
Expand Down
2 changes: 1 addition & 1 deletion src/nanoarrow/nanoarrow_testing_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -470,7 +470,7 @@ TEST(NanoarrowTestingTest, NanoarrowTestingTestFieldMetadata) {
NANOARROW_RETURN_NOT_OK(ArrowSchemaSetMetadata(schema, "\0\0\0\0"));
return NANOARROW_OK;
},
[](ArrowArray* array) { return NANOARROW_OK; }, &WriteFieldJSON,
[](ArrowArray*) { return NANOARROW_OK; }, &WriteFieldJSON,
R"({"name": null, "nullable": true, "type": {"name": "null"}, "children": []})",
[](TestingJSONWriter& writer) { writer.set_include_metadata(false); });
}
Expand Down
Loading