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

3 changes: 1 addition & 2 deletions ci/scripts/build-with-meson.sh
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ function main() {
mkdir "${SANDBOX_DIR}"

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 All @@ -76,7 +76,6 @@ function main() {
-Db_coverage=false

meson compile
export ASAN_OPTIONS=allocator_may_return_null=1 # allow ENOMEM tests
meson test --print-errorlogs

show_header "Run valgrind test suite"
Expand Down
14 changes: 13 additions & 1 deletion meson.build
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,17 @@ project(
# add_project_arguments(['-fvisibility=hidden'], language: 'cpp')
# endif

cc = meson.get_compiler('c')
add_project_arguments(
cc.get_supported_arguments(['-Wno-misleading-indentation']),
language : 'c'
)
cpp = meson.get_compiler('cpp')
add_project_arguments(
cpp.get_supported_arguments(['-Wno-misleading-indentation']),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

language : 'cpp'
)

nanoarrow_dep_args = []
if host_machine.system() == 'windows' and get_option('default_library') == 'shared'
add_project_arguments(['-DNANOARROW_BUILD_DLL', '-DNANOARROW_EXPORT_DLL'], language: 'c')
Expand Down Expand Up @@ -161,7 +172,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')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

gtest_dep = dependency('gtest_main')
gmock_dep = dependency('gmock')

Expand Down
4 changes: 4 additions & 0 deletions src/nanoarrow/common/array_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -98,9 +98,11 @@ TEST(ArrayTest, ArrayTestAllocateChildren) {
ArrowArrayRelease(&array);

ASSERT_EQ(ArrowArrayInitFromType(&array, NANOARROW_TYPE_STRUCT), NANOARROW_OK);
#if !defined(__SANITIZE_ADDRESS__)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

EXPECT_EQ(ArrowArrayAllocateChildren(
&array, std::numeric_limits<int64_t>::max() / sizeof(void*)),
ENOMEM);
#endif
ArrowArrayRelease(&array);

ASSERT_EQ(ArrowArrayInitFromType(&array, NANOARROW_TYPE_STRUCT), NANOARROW_OK);
Expand Down Expand Up @@ -2303,9 +2305,11 @@ TEST(ArrayTest, ArrayViewTestStruct) {
EXPECT_EQ(array_view.layout.element_size_bits[0], 1);

// Expect error for out-of-memory
#if !defined(__SANITIZE_ADDRESS__)
EXPECT_EQ(ArrowArrayViewAllocateChildren(
&array_view, std::numeric_limits<int64_t>::max() / sizeof(void*)),
ENOMEM);
#endif

EXPECT_EQ(ArrowArrayViewAllocateChildren(&array_view, 2), NANOARROW_OK);
EXPECT_EQ(array_view.n_children, 2);
Expand Down
5 changes: 2 additions & 3 deletions src/nanoarrow/common/buffer_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -156,9 +156,6 @@ TEST(BufferTest, BufferTestFill) {
}

ArrowBufferReset(&buffer);

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

TEST(BufferTest, BufferTestResize0) {
Expand All @@ -184,8 +181,10 @@ TEST(BufferTest, BufferTestResize0) {
TEST(BufferTest, BufferTestError) {
struct ArrowBuffer buffer;
ArrowBufferInit(&buffer);
#if !defined(__SANITIZE_ADDRESS__)
EXPECT_EQ(ArrowBufferResize(&buffer, std::numeric_limits<int64_t>::max(), false),
ENOMEM);
#endif

ASSERT_EQ(ArrowBufferAppend(&buffer, "abcd", 4), NANOARROW_OK);
EXPECT_EQ(ArrowBufferSetAllocator(&buffer, ArrowBufferAllocatorDefault()), EINVAL);
Expand Down
2 changes: 2 additions & 0 deletions src/nanoarrow/common/schema_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -68,9 +68,11 @@ TEST(SchemaTest, SchemaInit) {
EXPECT_EQ(schema.release, nullptr);

ASSERT_EQ(ArrowSchemaInitFromType(&schema, NANOARROW_TYPE_UNINITIALIZED), NANOARROW_OK);
#if !defined(__SANITIZE_ADDRESS__)
EXPECT_EQ(ArrowSchemaAllocateChildren(
&schema, std::numeric_limits<int64_t>::max() / sizeof(void*)),
ENOMEM);
#endif
ArrowSchemaRelease(&schema);
}

Expand Down
4 changes: 4 additions & 0 deletions src/nanoarrow/common/utils_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -150,13 +150,15 @@ TEST(AllocatorTest, AllocatorTestDefault) {

allocator.free(&allocator, buffer, 100);

#if !defined(__SANITIZE_ADDRESS__)
buffer =
allocator.reallocate(&allocator, nullptr, 0, std::numeric_limits<int64_t>::max());
EXPECT_EQ(buffer, nullptr);

buffer =
allocator.reallocate(&allocator, buffer, 0, std::numeric_limits<int64_t>::max());
EXPECT_EQ(buffer, nullptr);
#endif
}

// In a non-trivial test this struct could hold a reference to an object
Expand Down Expand Up @@ -239,13 +241,15 @@ TEST(AllocatorTest, AllocatorTestMemoryPool) {
arrow_allocator.free(&arrow_allocator, buffer, 100);
EXPECT_EQ(CustomMemoryPool::GetInstance()->bytes_allocated, allocated0);

#if !defined(__SANITIZE_ADDRESS__)
buffer = arrow_allocator.reallocate(&arrow_allocator, nullptr, 0,
std::numeric_limits<int64_t>::max());
EXPECT_EQ(buffer, nullptr);

buffer = arrow_allocator.reallocate(&arrow_allocator, buffer, 0,
std::numeric_limits<int64_t>::max());
EXPECT_EQ(buffer, nullptr);
#endif
}

TEST(DecimalTest, Decimal128Test) {
Expand Down
1 change: 1 addition & 0 deletions src/nanoarrow/device/device.c
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,7 @@ void ArrowDeviceInitCpu(struct ArrowDevice* device) {
}

struct ArrowDevice* ArrowDeviceResolve(ArrowDeviceType device_type, int64_t device_id) {
NANOARROW_UNUSED(device_id);
if (device_type == ARROW_DEVICE_CPU) {
return ArrowDeviceCpu();
}
Expand Down
8 changes: 4 additions & 4 deletions subprojects/flatcc.wrap
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,10 @@
# under the License.

[wrap-file]
directory = flatcc-0.6.1
source_url = https://github.com/dvidelabs/flatcc/archive/refs/tags/v0.6.1.tar.gz
source_filename = flatcc-0.6.1.tar.gz
source_hash = 2533c2f1061498499f15acc7e0937dcf35bc68e685d237325124ae0d6c600c2b
directory = flatcc-fd3c4ae5cd39f0651eda6a3a1a374278070135d6
source_url = https://github.com/dvidelabs/flatcc/archive/fd3c4ae5cd39f0651eda6a3a1a374278070135d6.tar.gz
source_filename = flatcc-fd3c4ae5cd39f0651eda6a3a1a374278070135d6.tar.gz
source_hash = 353cb04a619865383b87c8077eb39b63b01a3fc44f7bebd558a88f139c6b6f77
patch_directory = flatcc

[provide]
Expand Down
Loading