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

new(ci): use zig compiler instead of relying on centos7. #3307

Merged
merged 2 commits into from
Oct 9, 2024

Conversation

FedeDP
Copy link
Contributor

@FedeDP FedeDP commented Aug 28, 2024

What type of PR is this?

/kind cleanup

Any specific area of the project related to this PR?

/area CI

What this PR does / why we need it:

This PR drops centos7 from our CI by instead relying on zig compiler to provide glibc 2.17 compatible builds for us.

Which issue(s) this PR fixes:

Fixes #3270

Special notes for your reviewer:

Linked libs PRs:

Linked upstream PR:

TODO:

Tue Sep  3 10:46:35 2024: Loaded event sources: syscall
Tue Sep  3 10:46:35 2024: Enabled event sources: syscall
Tue Sep  3 10:46:35 2024: Opening 'syscall' source with Kernel module
Trace/breakpoint trap
  • fix test-dev-packages-arm64 -> it seems like a Trace/breakpoint trap is caught related to stats_writer line: output_fields["falco.host_boot_ts"] = machine_info->boot_ts_epoch; -> bca6f31
  • fix test-dev-packages-arm64 -> it seems like a Trace/breakpoint trap is caught related to k8saudit opening
  • libelf is built bundled

Does this PR introduce a user-facing change?:

new(ci): use `zig` compiler instead of relying on centos7.

@poiana
Copy link
Contributor

poiana commented Aug 28, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: FedeDP

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@FedeDP
Copy link
Contributor Author

FedeDP commented Aug 28, 2024

/milestone TBD

@poiana poiana added this to the TBD milestone Aug 28, 2024
@FedeDP
Copy link
Contributor Author

FedeDP commented Aug 28, 2024

So, we need to disable the build with shared libelf, because zig statically links all specified libraries; see: ziglang/zig#7094
Indeed the CI complains it cannot find system libelf.
Since this was an 🌶️ topic for the CNCF, i am going to hold this PR; at the same time, i think we should actually find a way to drop centos:7 while maintaining same glibc requirements.

Anyway, even trying with BUNDLED_LIBELF, gives a build error...

/hold

cmake -B build -S . \
-DCMAKE_BUILD_TYPE=${{ inputs.build_type }} \
-DUSE_BUNDLED_DEPS=On \
-DUSE_BUNDLED_LIBELF=On \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Enabled bundled libelf to build with zig. Of course this is not desiderable.

@FedeDP FedeDP force-pushed the new/use_zig_drop_centos7 branch 2 times, most recently from e6e593c to 8925c1f Compare August 28, 2024 14:38
@FedeDP
Copy link
Contributor Author

FedeDP commented Aug 28, 2024

Now failing the build on

ld.lld: error: undefined symbol: arc4random_buf

referenced by ares_rand.c:270

That's probably because c-ares does some magic to detect that symbol: https://github.com/c-ares/c-ares/blob/main/CMakeLists.txt#L456; that symbol was added in glibc2.36; most probably it is detecting the symbol even if zig is not actually supporting it since we are targeting a build with glibc-2.17.

@FedeDP
Copy link
Contributor Author

FedeDP commented Aug 29, 2024

I tried to build c-ares from scratch (ie: without Falco involved at all) with zig and it worked fine with the same exports we are doing in the CI.

@FedeDP
Copy link
Contributor Author

FedeDP commented Aug 29, 2024

Update: i saw that updating c-ares to 1.33.1 (latest release) fixed the issue. I am going to bump it in libs (if possible, ie: if grpc does not complain :/ )

Bump PR in libs: falcosecurity/libs#2034

I now bumped this one to use head of the libs PR with the updated c-ares. 🤞

@FedeDP
Copy link
Contributor Author

FedeDP commented Aug 29, 2024

So, i now bumped this branch to use libs chore/zig_build branch HEAD; that is a branch i pushed upstream with all zig related fixes on it:

On that branch, i was able to build a full BUNDLED_DEPS version of sinsp-example!

Moreover, i found out that CMAKE_{C,CXX}_COMPILER truncates CC and CXX strings at first whitespace, leading with cmake using zig as compiler for both, instead of zig cc and zig c++.
I created 2 small wrapper scripts in CI to workaround this issue:

cat > zig-linux-${{ inputs.arch }}-${ZIG_VERSION}/zig-cc << EOF
#!/bin/bash
zig cc -target ${{ inputs.arch }}-linux-gnu.2.17 "$@"
EOF
chmod +x zig-linux-${{ inputs.arch }}-${ZIG_VERSION}/zig-cc
echo "CC=zig-cc" >> $GITHUB_ENV

// same for zig-c++

@FedeDP FedeDP force-pushed the new/use_zig_drop_centos7 branch from 70e175a to 73b8ed5 Compare August 29, 2024 13:49
@FedeDP
Copy link
Contributor Author

FedeDP commented Aug 30, 2024

To be able to build Falco, for now, i had to disable _FORTIFY_SOURCE, see my commit message: 0bb6f49

I opened an upstream issue to track the problem: ziglang/zig#21252 and a PR: ziglang/zig#21253

@FedeDP FedeDP force-pushed the new/use_zig_drop_centos7 branch 4 times, most recently from 903ec35 to 1bf6742 Compare August 30, 2024 14:26
@FedeDP FedeDP force-pushed the new/use_zig_drop_centos7 branch from 850a25e to 5851b6a Compare September 30, 2024 08:45
@poiana poiana added size/XL and removed size/M labels Sep 30, 2024
leogr
leogr previously approved these changes Sep 30, 2024
@FedeDP
Copy link
Contributor Author

FedeDP commented Sep 30, 2024

TODO:

Copy link

This PR may bring feature or behavior changes in the Falco engine and may require the engine version to be bumped.

Please double check userspace/engine/falco_engine_version.h file. See versioning for FALCO_ENGINE_VERSION.

/hold

@leogr
Copy link
Member

leogr commented Sep 30, 2024

This PR may bring feature or behavior changes in the Falco engine and may require the engine version to be bumped.

Please double check userspace/engine/falco_engine_version.h file. See versioning for FALCO_ENGINE_VERSION.

/hold

False positive

@FedeDP
Copy link
Contributor Author

FedeDP commented Sep 30, 2024

Yep, tomorrow will rebase on latest libs master that fixes arm64 test-dev-packages and unhold this one :)

@FedeDP FedeDP force-pushed the new/use_zig_drop_centos7 branch from 98d8dfa to be9c09d Compare October 1, 2024 06:33
@FedeDP
Copy link
Contributor Author

FedeDP commented Oct 1, 2024

/unhold
Ready!

@FedeDP
Copy link
Contributor Author

FedeDP commented Oct 1, 2024

📢 it might happen that master build fails because the cached key (ie: zig version) is no more available and zig moved to a newer master version, thus we cannot find the correct zig version to be downloaded.
In that case, i'll update zig to latest in libs and restart the CI here.

@FedeDP
Copy link
Contributor Author

FedeDP commented Oct 1, 2024

/hold
until next week.

@FedeDP
Copy link
Contributor Author

FedeDP commented Oct 7, 2024

I'll keep this on hold until we release Falco 0.39.1 to avoid issues with the new CI.

@FedeDP
Copy link
Contributor Author

FedeDP commented Oct 9, 2024

/unhold
Let's see if master CI complains (i guess so :D )

@poiana poiana merged commit c55adf3 into master Oct 9, 2024
36 checks passed
@poiana poiana deleted the new/use_zig_drop_centos7 branch October 9, 2024 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

CI pipeline uses CentOS 7 which is EOL
4 participants