build: use clang for public build #2730

ivotron · 2021-10-21T05:02:58Z

Tweaks the installed dependencies and cmake config of public build so that it uses clang. A Dockerfile is also included that defines a fedora:34 image with llvm-12 and libstdc++-11.2.1. This combination is known to work well for seastar (see scylladb/scylladb@3089558).

A PR on our avro fork is required in order to have Avro compiled correctly. There are still some compilation issues that need to be resolved related to seastar/core/sstring.hh used in v::random::generators. Thus, this PR is marked as draft until these get resolved.

fixes #1772

ivotron · 2021-10-21T05:04:30Z

The compilation error mentioned in the OP is:

[1/566] Linking CXX executable bin/zstd_tests_rpunit
[2/566] Linking CXX executable bin/basic_fast_random_test_rpunit
[3/566] Linking CXX executable bin/zstd_stream_rpbench
[4/566] Linking CXX executable bin/test_bytes_rpunit
[5/566] Linking CXX executable bin/rpc_rpunit
[6/566] Linking CXX executable bin/hashing_bench_rpbench
[7/566] Linking CXX executable bin/roundtrip_demo_types_rpunit
[8/566] Linking CXX executable bin/demo_client
[9/566] Linking CXX executable bin/model_rpunit
[10/566] Linking CXX executable bin/test_configuration_rpunit
[11/566] Building CXX object src/v/utils/tests/CMakeFiles/utils_unit_rpunit.dir/fragmented_vector_test.cc.o
FAILED: src/v/utils/tests/CMakeFiles/utils_unit_rpunit.dir/fragmented_vector_test.cc.o 
/usr/bin/clang++ -DBOOST_TEST_DYN_LINK -DFMT_LOCALE -DFMT_SHARED -DSEASTAR_API_LEVEL=6 -DXXH_PRIVATE_API -I../src/v -Isrc/v -isystem deps_install/include -isystem deps_install/include/hdr -fPIC -fcolor-diagnostics -O3 -DNDEBUG -fPIE -std=c++20 -U_FORTIFY_SOURCE -DSEASTAR_SSTRING -Werror=unused-result "-Wno-error=#warnings" -std=c++20 -MD -MT src/v/utils/tests/CMakeFiles/utils_unit_rpunit.dir/fragmented_vector_test.cc.o -MF src/v/utils/tests/CMakeFiles/utils_unit_rpunit.dir/fragmented_vector_test.cc.o.d -o src/v/utils/tests/CMakeFiles/utils_unit_rpunit.dir/fragmented_vector_test.cc.o -c ../src/v/utils/tests/fragmented_vector_test.cc
In file included from ../src/v/utils/tests/fragmented_vector_test.cc:11:
In file included from ../src/v/random/generators.h:16:
In file included from deps_install/include/seastar/core/sstring.hh:25:
In file included from /usr/lib/gcc/x86_64-redhat-linux/11/../../../../include/c++/11/algorithm:61:
In file included from /usr/lib/gcc/x86_64-redhat-linux/11/../../../../include/c++/11/bits/stl_algobase.h:66:
/usr/lib/gcc/x86_64-redhat-linux/11/../../../../include/c++/11/bits/stl_iterator_base_funcs.h:183:2: error: cannot decrement value of type 'fragmented_vector<long, 1024>::const_iterator'
        --__i;
        ^ ~~~
/usr/lib/gcc/x86_64-redhat-linux/11/../../../../include/c++/11/bits/stl_iterator_base_funcs.h:206:12: note: in instantiation of function template specialization 'std::__advance<fragmented_vector<long, 1024>::const_iterator, long>' requested here
      std::__advance(__i, __d, std::__iterator_category(__i));
           ^
/usr/lib/gcc/x86_64-redhat-linux/11/../../../../include/c++/11/bits/stl_algobase.h:1463:9: note: in instantiation of function template specialization 'std::advance<fragmented_vector<long, 1024>::const_iterator, long>' requested here
          std::advance(__middle, __half);
               ^
/usr/lib/gcc/x86_64-redhat-linux/11/../../../../include/c++/11/bits/stl_algobase.h:1499:19: note: in instantiation of function template specialization 'std::__lower_bound<fragmented_vector<long, 1024>::const_iterator, long, __gnu_cxx::__ops::_Iter_less_val>' requested here
      return std::__lower_bound(__first, __last, __val,
                  ^
../src/v/utils/tests/fragmented_vector_test.cc:74:25: note: in instantiation of function template specialization 'std::lower_bound<fragmented_vector<long, 1024>::const_iterator, long>' requested here
        auto it2 = std::lower_bound(other.begin(), other.end(), val);
                        ^
1 error generated.
[12/566] Linking CXX executable bin/demo_server
[13/566] Building CXX object src/v/model/tests/CMakeFiles/model_thread_rpunit.dir/record_batch_test.cc.o
[14/566] Building CXX object src/v/model/tests/CMakeFiles/model_thread_rpunit.dir/record_batch_reader_test.cc.o
[15/566] Building CXX object src/v/storage/opfuzz/CMakeFiles/v_storage_opfuzz.dir/opfuzz.cc.o
[16/566] Building CXX object src/v/storage/CMakeFiles/v_storage.dir/log_manager.cc.o
[17/566] Building CXX object src/v/rpc/test/CMakeFiles/rpcgenerator_cycling_rpunit.dir/rpc_gen_cycling_test.cc.o
[18/566] Building CXX object src/v/utils/tests/CMakeFiles/utils_single_thread_rpunit.dir/input_stream_fanout_test.cc.o
[19/566] Building CXX object src/v/raft/tests/CMakeFiles/test_raft_rpunit.dir/mux_state_machine_test.cc.o
[20/566] Building CXX object src/v/raft/tests/CMakeFiles/test_raft_rpunit.dir/manual_log_deletion_test.cc.o
ninja: build stopped: subcommand failed.

possibly related to the above: scylladb/seastar#634

shortcut-integration · 2021-10-21T05:06:47Z

This pull request has been linked to Shortcut Story #5493: build: move public build to clang.

install-dependencies.sh

BenPope

Looks pretty good, just a couple of comments.

Will this have the github workflow reenabled?

BenPope · 2021-10-29T09:39:52Z

build.sh

@@ -24,4 +24,4 @@ cmake -DCMAKE_BUILD_TYPE=Release \
 -DDEPOT_TOOLS_DIR="$root/depot_tools" \
 "$@"

-(cd $root/build && ninja && ctest --output-on-failure -R _rpunit)
+(cd $root/build && ninja && LD_LIBRARY_PATH=$PWD/deps_install/lib ctest --output-on-failure -R _rpunit)


I don't know why this is needed, the dependencies should be found by the RPATH.

yeah, i'll double check that it's needed. i recall it was required when i was working with this earlier but then a bunch of changes went in and didn't check if removing it would still make it

BenPope · 2021-10-29T09:41:44Z

build.sh

@@ -24,7 +24,7 @@ cmake -DCMAKE_BUILD_TYPE=Release \
 -GNinja \
 -DCMAKE_C_COMPILER=$CC \
 -DCMAKE_CXX_COMPILER=$CXX \
- -DDEPOT_TOOLS_DIR="$root/depot_tools" \
+ -DDEPOT_TOOLS_DIR="/opt/depot_tools" \


nit: Having the root configurable would allow building outside of a containerised env.

ok, i'll add a variable

tools/docker/README.md

Signed-off-by: Ivo Jimenez <[email protected]>

For public build, ignore errors coming from warnings for seastar compilation Signed-off-by: Ivo Jimenez <[email protected]>

Signed-off-by: Ivo Jimenez <[email protected]>

Adds a Dockerfile of an image that has all compilation dependencies. This installs clang-12, libstdc++11.2.1 on fedora:34 Signed-off-by: Ivo Jimenez <[email protected]>

Signed-off-by: Ivo Jimenez <[email protected]>

emaxerrno · 2021-11-01T17:50:42Z

Looks pretty good, just a couple of comments.

Will this have the github workflow reenabled?

@ivotron even if is just the build part is valuable.

ivotron · 2021-11-01T18:03:12Z

all looks good, the only problem now is that the vm on github is not powerful enough for building in a reasonable amount of time, and also looks like there are some unit test errors popping up due to the same (all tests pass locally). so I've added the required tweaks to the workflow but haven't enabled it. I think we should merge and then add an issue for CI-ing the public build

ivotron · 2021-11-01T18:03:51Z

Looks pretty good, just a couple of comments.
Will this have the github workflow reenabled?

@ivotron even if is just the build part is valuable.

do you mean, just having the build work fine, even if this can't run fine in github actions?

BenPope

Looking pretty good

BenPope · 2021-11-01T21:24:19Z

.github/workflows/redpanda-build.yml

- key: ${{ matrix.container }}-ccache-${{ steps.prepare_cache_variables.outputs.timestamp }}
- restore-keys: ${{ matrix.container }}-ccache-
+ path: /dev/shm/redpanda
+ key: ccache-${{github.ref}}


Will this fallback to dev if this fails?

2021-11-01T01:22:13.6661837Z Cache not found for input keys: ccache-refs/pull/2730/merge, ccache

Maybe it's not necessary/there's no key for dev: https://docs.github.com/en/actions/advanced-guides/caching-dependencies-to-speed-up-workflows#matching-a-cache-key

yeah, it'll check whether there's one for dev when an entry for the current pr's gitref is not present

BenPope · 2021-11-01T21:31:15Z

install-dependencies.sh

-
-rm -rf $root/depot_tools
+mkdir -p /opt/
+rm -rf /opt/depot_tools


Should this file use $DEPOT_TOOLS_DIR?

oh, right. gonna change

tools/docker/README.md

BenPope · 2021-11-01T21:39:41Z

build.sh

+fi
+
+ccache -p # print the config
+ccache -s # print the stats before reusing


2021-11-01T01:22:14.2219376Z max cache size 200.0 MB

Is this sufficient?

i don't know tbh, i haven't profiled after the changes in this pr

Hmm, I'm not convinced it's working properly, it seems quite small:

files in cache 732 cache size 29.7 MB max cache size 5.0 GB

build.sh

Add a DEPOT_TOOLS_DIR variable that contains the path to where it is placed and have it be `/opt/depot_tools` by default, to make it consistent with the way other dependencies are installed for the public build. Signed-off-by: Ivo Jimenez <[email protected]>

For the public build, change message shown when installing dependencies. Signed-off-by: Ivo Jimenez <[email protected]>

Public build assumes clang-12. Also adds a reference to instructions for using the pre-baked docker image with the frozen toolchain Signed-off-by: Ivo Jimenez <[email protected]>

Point to newest version on vectorizedio fork. Signed-off-by: Ivo Jimenez <[email protected]>

Make use of the container image introduced recently, which uses clang to build redpanda. Signed-off-by: Ivo Jimenez <[email protected]>

Use git ref as part of the key and remove timestamp. The current limit from Github is 5GB for all cached files across a repository. With this change, there is only one ccache file uploaded per branch, instead of one for each distinct build. This results in having one for the main branch, as well as one per PR. Signed-off-by: Ivo Jimenez <[email protected]>

Signed-off-by: Ivo Jimenez <[email protected]>

The host doesn't have ccache installed anymore Signed-off-by: Ivo Jimenez <[email protected]>

emaxerrno · 2021-11-02T02:15:48Z

Looks pretty good, just a couple of comments.
Will this have the github workflow reenabled?

@ivotron even if is just the build part is valuable.

do you mean, just having the build work fine, even if this can't run fine in github actions?

No. I agree with your assessment

BenPope

Would it be possible to get a one-off run of this workflow?

ivotron · 2021-11-02T20:06:21Z

Would it be possible to get a one-off run of this workflow?

some tests are failing when executed by the workflow: https://github.com/vectorizedio/redpanda/actions/runs/1406061645. i haven't looked deeper but it seems like the constrained environment might be the root cause

BenPope

LGTM, the build seems to do the correct thing.

BenPope · 2021-11-02T21:14:26Z

Should the ducktape tests be run?

unicomp21 · 2021-11-03T11:53:19Z

@ivotron @dotnwat @BenPope is all this running successfully inside a docker container? I might be wrong, my perception is there are differences when running inside a docker container. For community support, I'm inclined to think a docker container is needed? To ensure a common point of reference for everyone?

BenPope · 2021-11-03T11:56:46Z

@ivotron @dotnwat @BenPope is all this running successfully inside a docker container? I might be wrong, my perception is there are differences when running inside a docker container. For community support, I'm inclined to think a docker container is needed? To ensure a common point of reference for everyone?

Yes, see the instructions: https://github.com/vectorizedio/redpanda/pull/2730/files#diff-5701757eb2f837a5f4ae8d1e1989a60593fa5d6b2b15580b704a641c5ad6a0c2

To test this PR:

sudo sysctl -w fs.aio-max-nr=10485760
docker build \
  -t vectorized/redpanda-toolchain \
  -f tools/docker/Dockerfile \
  .
docker run --ipc=host --rm -ti -v $PWD:$PWD:Z -w $PWD vectorized/redpanda-toolchain ./build.sh

I'm not sure what changed when, and the current state of the builder image, but you might get away with:

docker run --pull=always --ipc=host --rm -ti -v $PWD:$PWD:Z -w $PWD vectorized/redpanda-toolchain ./build.sh

ivotron · 2021-11-03T17:41:44Z

Should the ducktape tests be run?

yes, moving to buildkite or self-hosted github runner would help #2852

github-actions bot added the area/build label Oct 21, 2021

ivotron requested review from BenPope and dotnwat October 21, 2021 05:05

ivotron mentioned this pull request Oct 25, 2021

cannot build on ubuntu 20.04 #2760

Closed

ivotron commented Oct 27, 2021

View reviewed changes

install-dependencies.sh Show resolved Hide resolved

BenPope reviewed Oct 29, 2021

View reviewed changes

ivotron force-pushed the compile-with-clang branch from d534cbf to 2b3c743 Compare October 31, 2021 05:30

github-actions bot added area/docs area/k8s area/redpanda area/rpk labels Oct 31, 2021

ivotron force-pushed the compile-with-clang branch from 2b3c743 to 731728b Compare November 1, 2021 00:44

github-actions bot removed area/redpanda area/k8s area/docs area/rpk labels Nov 1, 2021

ivotron force-pushed the compile-with-clang branch from 731728b to 4020671 Compare November 1, 2021 00:47

ivotron added 6 commits October 31, 2021 17:52

cmake/oss: fix typo on config for v8

ac99c28

Signed-off-by: Ivo Jimenez <[email protected]>

cmake: ignore warnings when compiling seastar

90d0a6f

For public build, ignore errors coming from warnings for seastar compilation Signed-off-by: Ivo Jimenez <[email protected]>

build: move to use clang

1f81767

Signed-off-by: Ivo Jimenez <[email protected]>

build: add dependencies to build with clang

52b71e2

Signed-off-by: Ivo Jimenez <[email protected]>

build: add toolchain image for public build

d286f3c

Adds a Dockerfile of an image that has all compilation dependencies. This installs clang-12, libstdc++11.2.1 on fedora:34 Signed-off-by: Ivo Jimenez <[email protected]>

build: fix CCACHE_DIR check for public build

1f4c448

Signed-off-by: Ivo Jimenez <[email protected]>

ivotron force-pushed the compile-with-clang branch 5 times, most recently from 31dc10d to 878715c Compare November 1, 2021 01:21

ivotron marked this pull request as ready for review November 1, 2021 01:22

ivotron requested a review from bmansheim as a code owner November 1, 2021 01:22

ivotron requested review from dotnwat and BenPope and removed request for dotnwat November 1, 2021 19:33

BenPope reviewed Nov 1, 2021

View reviewed changes

ivotron added 8 commits November 1, 2021 15:54

build: change message install-dependencies shows

3cd4b49

For the public build, change message shown when installing dependencies. Signed-off-by: Ivo Jimenez <[email protected]>

readme: update 'build manually' instructions

96d0a73

Public build assumes clang-12. Also adds a reference to instructions for using the pre-baked docker image with the frozen toolchain Signed-off-by: Ivo Jimenez <[email protected]>

schemaregistry: bump up version of avro

22acdcf

Point to newest version on vectorizedio fork. Signed-off-by: Ivo Jimenez <[email protected]>

gha: use clang in redpanda workflow

e5a23ab

Make use of the container image introduced recently, which uses clang to build redpanda. Signed-off-by: Ivo Jimenez <[email protected]>

gha: set fs.aio-max-nr on host machine

210e311

Signed-off-by: Ivo Jimenez <[email protected]>

gha: move ccache command invocation to container

4534358

The host doesn't have ccache installed anymore Signed-off-by: Ivo Jimenez <[email protected]>

ivotron force-pushed the compile-with-clang branch from 878715c to 4534358 Compare November 1, 2021 22:54

ivotron mentioned this pull request Nov 2, 2021

add public build to CI #2852

Closed

BenPope reviewed Nov 2, 2021

View reviewed changes

BenPope approved these changes Nov 2, 2021

View reviewed changes

ivotron merged commit 690d09f into redpanda-data:dev Nov 3, 2021

ivotron deleted the compile-with-clang branch November 3, 2021 17:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: use clang for public build #2730

build: use clang for public build #2730

ivotron commented Oct 21, 2021 •

edited

Loading

ivotron commented Oct 21, 2021

shortcut-integration bot commented Oct 21, 2021

BenPope left a comment

BenPope Oct 29, 2021

ivotron Oct 31, 2021

BenPope Oct 29, 2021

ivotron Oct 31, 2021

emaxerrno commented Nov 1, 2021

ivotron commented Nov 1, 2021

ivotron commented Nov 1, 2021

BenPope left a comment

BenPope Nov 1, 2021

ivotron Nov 1, 2021

BenPope Nov 1, 2021

ivotron Nov 1, 2021

ivotron Nov 1, 2021

BenPope Nov 1, 2021

ivotron Nov 1, 2021

BenPope Nov 2, 2021 •

edited

Loading

emaxerrno commented Nov 2, 2021

BenPope left a comment

ivotron commented Nov 2, 2021 •

edited

Loading

BenPope left a comment

BenPope commented Nov 2, 2021

unicomp21 commented Nov 3, 2021

BenPope commented Nov 3, 2021 •

edited

Loading

ivotron commented Nov 3, 2021

build: use clang for public build #2730

build: use clang for public build #2730

Conversation

ivotron commented Oct 21, 2021 • edited Loading

ivotron commented Oct 21, 2021

shortcut-integration bot commented Oct 21, 2021

BenPope 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

emaxerrno commented Nov 1, 2021

ivotron commented Nov 1, 2021

ivotron commented Nov 1, 2021

BenPope 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

BenPope Nov 2, 2021 • edited Loading

Choose a reason for hiding this comment

emaxerrno commented Nov 2, 2021

BenPope left a comment

Choose a reason for hiding this comment

ivotron commented Nov 2, 2021 • edited Loading

BenPope left a comment

Choose a reason for hiding this comment

BenPope commented Nov 2, 2021

unicomp21 commented Nov 3, 2021

BenPope commented Nov 3, 2021 • edited Loading

ivotron commented Nov 3, 2021

ivotron commented Oct 21, 2021 •

edited

Loading

BenPope Nov 2, 2021 •

edited

Loading

ivotron commented Nov 2, 2021 •

edited

Loading

BenPope commented Nov 3, 2021 •

edited

Loading