-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 Clang Linux build to CI pipeline #10767
Conversation
✅ Deploy Preview for meta-velox canceled.
|
984a9e0
to
245f3a3
Compare
The workflow isn't triggered. Maybe a chicken and egg problem? @majetideepak FYI. |
@czentgr the job was triggerd but failed during parsing which means it doesn't show up in the check list (not a great choice on gh's part...) https://github.com/facebookincubator/velox/actions/runs/10411698998/workflow#L53 |
245f3a3
to
195045f
Compare
@assignUser Thank you! I fixed the path. I overlooked the part with the path in the doc. |
a45f534
to
f12993e
Compare
@assignUser I hope it is nothing bad but got two odd issues in my recent run: linux-adapter with clang (the one most interested in) failed with network issue (artifact read timed out).
And MacOS14 failed during upload with
I will retry later. |
f12993e
to
a5a4c98
Compare
I have deleted all active caches for the mac 14 build (on main), that should fix this issue. |
The build issue for ubuntu-clang Linux with adapters is fixed in PR #10800. |
It seems the macos14 issues is related to a bug in the artifacts action, looking into it. |
eee46a1
to
3ead727
Compare
The release build with clang15 causes one of the tests in
A SEGV at signal address 0x8. So a nullptr that tries to access at offset 0x8. The issue occurs when calling the constructor of a static object
Further investigation shows that the VTT InlineExecutor symbol for a release build is not linked. It has no address:
while for a debug build it shows:
The symbol not having an address would result in the SEGV as it results in a NULL pointer access. I don't know why this is compiled like by Clang15 and not linked properly. It seems the symbol is optimized away. It could also have something to do with the fact that it is a "cold" symbol. Because this is a test executable I've added a workaround to compile this file with no optimizations which resolves the issue. We can revisit this later - perhaps open a new issue to investigate this further at a later point in time. |
3566ec6
to
d0a535e
Compare
b6272d6
to
48287b1
Compare
48287b1
to
a0dae26
Compare
@mbasmanova has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@czentgr Would you rebase to resolve merge conflicts? |
run: | | ||
MINIO_BINARY="minio-2022-05-26" | ||
if [ ! -f /usr/local/bin/${MINIO_BINARY} ]; then | ||
wget https://dl.min.io/server/minio/release/linux-amd64/archive/minio.RELEASE.2022-05-26T05-48-41Z -O ${MINIO_BINARY} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: in a future update we should bake this into the image else we run risk of ci jobs failing when they cant download this for whatever reason..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, this was added a while back. The entire file is a copy of the original with minor changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Minio binary should be found in the image as it is part of the setup-adapter.sh install for AWS. As such the Minio binary should be found nowadays. In most (all?) cases this download doesn't occur.
We can clean it up with a subsequent PR.
a0dae26
to
3f7c934
Compare
@mbasmanova has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@czentgr CI is red. |
@mbasmanova Yes. Two flaky tests (MockSharedArbitrationTest, FilterProjectReplayerTest) failed that seem to have been fixed recently (aka within the last 24h). I can rebase again. Does this cause an issue with the fact that you already imported it? |
@czentgr I'm seeing merge conflict with "Add gcc11 to Ubuntu20.04 setup and add PkgConfig install". Would you rebase? |
3f7c934
to
c4fda34
Compare
@mbasmanova Odd you see a conflict. "Add gcc11 to Ubuntu20.04 setup and add PkgConfig install" did touch |
@mbasmanova has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
This change should not affect the fuzzer. I created an issue: #11462 |
@czentgr Thank you for creating an issue. I'll try to by-pass this failure and merge. |
@czentgr Something happening and land got stuck. Would you rebase so I can try again? |
The linux build is refactored to be able to switch between clang and gcc based builds of Velox. The gcc based linux build is run on pull and push. The clang based linux build is added to the scheduled jobs and executed on schedule only.
c4fda34
to
5b209d8
Compare
@mbasmanova Thanks! I rebased. |
@czentgr Thanks. There are Fuzzer failures. Would you take a look? |
@mbasmanova has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@mbasmanova merged this pull request in 8374802. |
@mbasmanova The fuzzer failure was some intermittend network issue. It failed to upload artifacts
The fuzzer run itself was successful. I think we are good and you merged the PR :) |
Conbench analyzed the 1 benchmark run on commit There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
The linux build is refactored to be able to switch between clang and gcc based builds of Velox.
The gcc based linux build is run on pull and push.
The clang based linux build is added to the scheduled jobs and executed on schedule
only.