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.

Sign up for GitHub

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

Jump to bottom

CORE-2708 - chore: fix open source CI build #18390

Merged
merged 4 commits into from May 13, 2024
Merged

CORE-2708 - chore: fix open source CI build #18390

merged 4 commits into from May 13, 2024

Conversation

pgellert
Copy link
Contributor

@pgellert pgellert commented May 10, 2024 •
edited

The open source build is currently failing because of various errors.

  1. We need to upgrade the seastar version used by the open source build to match what we use in the vtools-based build.
  2. We need to downgrade the golang version used by kreq-gen to 1.21
  3. We need to prevent seastar from upgrading the C++ version used by the build.

The open source build is that it tries to use golang version 1.21 to build kreq-gen because that is what is available on ubuntu:mantic and fedora:38. Unfortunately, we can't upgrade to the newer OS yet because that would upgrade clang to clang-18 as well, which we are not ready for. So for the meantime, I have reverted to using go v1.21 for kreq-gen.

Fixes https://redpandadata.atlassian.net/browse/CORE-2708

Backports Required

  • none - not a bug fix
  • none - this is a backport
  • none - issue does not exist in previous branches
  • none - papercut/not impactful enough to backport
  • v24.1.x
  • v23.3.x
  • v23.2.x

Release Notes

  • none

@pgellert pgellert self-assigned this May 10, 2024
@pgellert pgellert requested review from BenPope and a team as code owners May 10, 2024 10:26
@pgellert pgellert requested review from jackietung-redpanda and removed request for a team May 10, 2024 10:26
@pgellert pgellert mentioned this pull request May 10, 2024
Closed
7 tasks
@pgellert
Copy link
Contributor Author

The oss build is still failing, but now on ubuntu: https://github.com/redpanda-data/redpanda/actions/runs/9030703077

Locally the fedora build works now with the additional fixes I've pushed, but on ubuntu there are some seastar-related errors. @graphcareful could you take a look at the errors?

BenPope
BenPope reviewed May 10, 2024
View reviewed changes
src/v/serde/serde_is_enum.h Outdated
@@ -17,7 +17,7 @@ using serde_enum_serialized_t = int32_t;

template<typename T>
inline constexpr bool serde_is_enum_v =
#if __has_cpp_attribute(__cpp_lib_is_scoped_enum)
#if __cpp_lib_is_scoped_enum >= 202011L
Copy link
Member

Choose a reason for hiding this comment

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

What changed that made this break?

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've reverted this change now. It was needed because seastar was overwriting the c++ version used to 23 instead of 20, and it looks like this fails in c++ 23.

@graphcareful
Copy link
Contributor

The oss build is still failing, but now on ubuntu: https://github.com/redpanda-data/redpanda/actions/runs/9030703077

Locally the fedora build works now with the additional fixes I've pushed, but on ubuntu there are some seastar-related errors. @graphcareful could you take a look at the errors?

If your referring to the unit test failure this is a known issue and is being looked into here: #13275 (comment)

@pgellert pgellert marked this pull request as draft May 10, 2024 15:41
@pgellert pgellert force-pushed the chore/fix-oss-build branch 2 times, most recently from 0697ee4 to 0d9ae7a Compare May 10, 2024 17:30
@vbotbuildovich
Copy link
Collaborator

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/48939#018f6364-0f3d-4556-983c-16b5c576c63c

@pgellert
kreq-gen: go back to v1.21 go
1def3fd 
This is needed to fix the failing open source build. Until we upgrade to
newer OS versions of ubuntu and fedora, we need to stick to v1.21
because the version of golang installed by install-dependencies.sh is
1.21 on both fedora:38 and ubuntu:mantic.
@pgellert
net: fix tls serial type
fc1b792 
The type changed from bytes_view to std::vector<std::byte> during the
seastar upgrade.
@pgellert
cmake: fix prevent seastar changing to c++23
370822a 
Seastar uses a cmake cache variable to configure the c++ version it
uses. The problem with this for the redpanda build is that the cache
variable overwrites the normal variable set with the simple set() call.
This leads to us building redpanda with c++23 instead of c++20 as
intended, since seastar started setting the c++ version to c++23.

This is only a problem with the open source build because here we use
FetchContent to include dependencies directly in the cmake build,
whereas the internal build uses ExternalProject which allows us to
explicitly set the cache variable `CMAKE_CXX_STANDARD` for our
dependencies.

Note that we can't yet change to compiling with C++23 because of a bug
in the llvm-project. The combination of libstdc++ >= 13, c++ >= 23 and
clang 16 has a bug that caused compilation issues on ubuntu (see
llvm-project issue 61415).

This does not cause any compilation errors on fedora because fedora has
a patch version higher gcc and libstdc++ installed which contains a fix
for the above bug.
@pgellert
Copy link
Contributor Author

I got to a passing build with this: https://github.com/redpanda-data/redpanda/actions/runs/9061716793/job/24894083129

@pgellert pgellert marked this pull request as ready for review May 13, 2024 11:20
@pgellert pgellert requested a review from BenPope May 13, 2024 11:20
BenPope
BenPope approved these changes May 13, 2024
View reviewed changes
src/v/net/probes.cc
@@ -292,7 +292,7 @@ void tls_certificate_probe::loaded(
return;
}

auto to_tls_serial = [](bytes_view b) {
auto to_tls_serial = [](const auto& b) {
Copy link
Member

Choose a reason for hiding this comment

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

nit, perhaps this could be std::span<std::byte>

Copy link
Member

Choose a reason for hiding this comment

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

tangential (sorry):

  1. Did my recent seastar change break this?
  2. Would it make sense to expedite this instead? (stalled last week on CI troubles, but I can push)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Yes I believe so
  2. I am happy with either approach

Copy link
Member

@oleiman oleiman May 13, 2024 •
edited

Choose a reason for hiding this comment

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

I'd lean towards pushing on the vtools/seastar bump since the generic lambda here is a pure stopgap. But I'll rejigger it to the "correct" type regardless.

oleiman
oleiman approved these changes May 13, 2024
View reviewed changes
Copy link
Member

@oleiman oleiman left a comment

Choose a reason for hiding this comment

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

go for it

@dotnwat dotnwat merged commit d55749d into dev May 13, 2024
14 of 21 checks passed
@dotnwat dotnwat deleted the chore/fix-oss-build branch May 13, 2024 22:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BenPope BenPope BenPope approved these changes

@oleiman oleiman oleiman approved these changes

@jackietung-redpanda jackietung-redpanda Awaiting requested review from jackietung-redpanda jackietung-redpanda is a code owner automatically assigned from redpanda-data/devprod

@graphcareful graphcareful Awaiting requested review from graphcareful

Assignees

@pgellert pgellert

Labels
area/build area/redpanda
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@pgellert @graphcareful @vbotbuildovich @BenPope @oleiman @dotnwat