-
Notifications
You must be signed in to change notification settings - Fork 99
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
libc: make all builds LLVM_ENABLE_RUNTIMES #325
Conversation
@gkistanova the presubmit check failure looks related to 618344c. Can you PTAL? EDIT: posted a revert: #326 |
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.
I suspect the ASAN builds will be messed up by this change. Trying to confirm locally. Marking this as draft as such.
fb1d6fd
to
d43d4d2
Compare
Ah, the issue is that locally, I'm setting Personally, I think we may want to change how that works, such that
just works by building compiler-rt, then setting |
A quick:
later and I can build via:
perhaps I should document this... |
Seems to be OK now - I guess you addressed this with a rebase? I imagine previous failures were because the pre-commit checks were running against a base repo that didn't include #323. |
/me shrug ok then I think this is ready for review. |
if runtimes_build: | ||
projects = ['llvm', 'clang'] | ||
cmake_args.append('-DLLVM_ENABLE_RUNTIMES=libc') | ||
else: | ||
projects = ['llvm', 'libc'] | ||
|
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.
I think you still need llvm
in projects because the cmake command is rooted in the llvm
directory. See run_command(['cmake', os.path.join(source_dir, 'llvm')] + cmake_args)
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.
Have you verified that? I've been doing runtimes builds (rooted in the runtimes dir) locally without ANY value set for -DLLVM_ENABLE_PROJECTS
for building+testing libc for a while now. Even when I did core clang+llvm work, I think would use the llvm/ dir as the root and only explicitly list clang and lld in my -DLLVM_ENABLE_PROJECTS
. So I would be surprised if llvm needed to appear explicitly in -DLLVM_ENABLE_PROJECTS
when using llvm/ dir as the root for cmake (for llvm-libc, or any other sub project).
From a recent team discussion though, @lntue and @michaelrj-google mentioned that the use of llvm/ dir as the root and clang in the projects list will do a bootstrap build.
@lntue also pointed out this this is how we're testing fixed point support continuously, since the old clang most other buildbots are running are too old for fixed point support. Landing this PR right now would regress our CI coverage for fixed point.
So I think it's important to distinguish "bootstrap" builds (i.e. build clang first, then llvm-libc with that clang) from "runtimes" builds (anchored in runtimes/ for cmake). Those are two distinct things to me which are currently being conflated by the term "runtimes."
I have near term goals to move all build bots to newer versions of clang. That alone will resolve having coverage for fixed point.
It may still be handy to have "bootstrap" builds in CI, though they will take much longer to run since they need to build all of llvm and clang first.
Next steps to me then are perhaps:
- rename debian-dbg-runtimes-build to debian-dbg-bootstrap-build (rather than spin down debian-dbg-runtimes-build as I had originally intended in [libc] consider using runtimes/CMakeLists.txt as base (rather than llvm/CMakeLists.txt) llvm-project#78479 (comment). This splits the conflation of runtimes with runtimes/ cmake root from "bootstrap" builds. Probably need to update buildbot config files, move/rename dirs, and wipe build dirs.
- update this PR to move remaining builds to LLVM_ENABLE_RUNTIMES
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.
SGTM
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.
#332 as precursor so that 1 doesn't turn off any CI builds.
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.
https://lab.llvm.org/buildbot/#/builders?tags=libc now shows that libc-x86_64-debian-dbg-runtimes-build
has been renamed to libc-x86_64-debian-dbg-bootstrap-build
. I've rebased this and updated it, so this thread should be resolved. Ready for rereview.
Before renaming the existing debian-dbg-runtimes-build to debian-dbg-bootstrap-build, make sure that the new name will be recognized so that there's no downtime of CI coverage due to renaming the buildbot. Link: llvm#325 (comment)
Before renaming the existing debian-dbg-runtimes-build to debian-dbg-bootstrap-build, make sure that the new name will be recognized so that there's no downtime of CI coverage due to renaming the buildbot. Also remove llvm from the explicit project list; that's not necessary (or may have been for old hdrgen which has been removed). Link: #325 (comment)
No longer will any CI builds use LLVM_ENABLE_PROJECTS, which is deprecated as of llvm/llvm-project#117265. libc-x86_64-debian-dbg-runtimes-build can be offlined as a result of this change. Buildbots will probably need their CMakeCache.txt reset after this.
d43d4d2
to
c0ace11
Compare
PSA: I plan to land this after lunch (~2 hours from now), then be around to put out fires that may spring up this afternoon. |
projects = ['llvm', 'libc'] | ||
# TODO: remove once old hdrgen is deleted. | ||
# https://github.com/llvm/llvm-project/pull/117220 | ||
projects = ['llvm', 'clang'] |
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.
does it mean that we always build libc using clang from head?
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.
Only for the "bootstrap" builder (1 out of 13 active builders).
|
…ore assignment Fixes: llvm#325
homie... |
Fullbuilds are now failing with:
They were configured via:
|
ah, I totally missed setting the cmake root as the runtimes dir... |
No longer will any CI builds use LLVM_ENABLE_PROJECTS, which is deprecated as
of llvm/llvm-project#117265.
Buildbots will probably need their CMakeCache.txt reset after this.
Link: llvm/llvm-project#78479