Skip to content

Adopt upstream llvm-project monorepo directory layout#13049

Merged
hahnjo merged 4 commits into
root-project:masterfrom
hahnjo:llvm-dir-layout
Jun 25, 2023
Merged

Adopt upstream llvm-project monorepo directory layout#13049
hahnjo merged 4 commits into
root-project:masterfrom
hahnjo:llvm-dir-layout

Conversation

@hahnjo
Copy link
Copy Markdown
Member

@hahnjo hahnjo commented Jun 20, 2023

This will be required for future LLVM upgrades that rely on a common cmake/ directory next to clang/ and llvm/.


To have any chance at reviewing this change, the mechanical moving is in the first commit and the other two commits are the actual changes (build system, then llvm-diff workflow). I will squash the commits on merge because the state after the first commit obviously won't build.

@hahnjo hahnjo self-assigned this Jun 20, 2023
@hahnjo hahnjo added the clean build Ask CI to do non-incremental build on PR label Jun 20, 2023
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jun 20, 2023

Test Results

       10 files         10 suites   2d 7h 3m 20s ⏱️
  2 470 tests   2 470 ✔️ 0 💤 0
23 167 runs  23 167 ✔️ 0 💤 0

Results for commit 7a1b7a2.

♻️ This comment has been updated with latest results.

@hahnjo hahnjo force-pushed the llvm-dir-layout branch from ecbcd67 to a0b160e Compare June 20, 2023 11:29
@root-project root-project deleted a comment from phsft-bot Jun 20, 2023
@phsft-bot

This comment was marked as outdated.

@root-project root-project deleted a comment from phsft-bot Jun 20, 2023
@phsft-bot

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

Copy link
Copy Markdown
Member

@vgvassilev vgvassilev left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me!

@Axel-Naumann
Copy link
Copy Markdown
Member

Before I review and out of curiosity, did you consider interpreter/llvm, interpreter/clang, interpreter/cling, interpreter/cmake, etc? Is that worse because the top-most directory doesn't correspond to upstream anymore?

If so, would interpreter/llvm-project/llvm be better than interpreter/llvm/llvm, because it's signalling that this is from upstream?

@hahnjo
Copy link
Copy Markdown
Member Author

hahnjo commented Jun 20, 2023

Before I review and out of curiosity, did you consider interpreter/llvm, interpreter/clang, interpreter/cling, interpreter/cmake, etc? Is that worse because the top-most directory doesn't correspond to upstream anymore?

Yes, and two consequences thereof: (a) I want to avoid a conflict further down the line if LLVM ever puts a CMakeLists.txt in their top-level directory, and (b) we had the proposal at some point to replace our copy in interpreter/llvm/ by a submodule, which requires a directory on its own.

If so, would interpreter/llvm-project/llvm be better than interpreter/llvm/llvm, because it's signalling that this is from upstream?

Yes, I'm open to that - I thought about it, but it requires changing a few more paths to interpreter/llvm/ROOT/ and I was lazy. Btw do we still need those headers with C++ modules? core/clingutils/CMakeLists.txt even copies some headers from the build system...

@hahnjo
Copy link
Copy Markdown
Member Author

hahnjo commented Jun 21, 2023

Btw do we still need those headers (in interpreter/llvm/ROOT/) with C++ modules? core/clingutils/CMakeLists.txt even copies some headers from the build system...

ie what breaks with #13054 ?

@Axel-Naumann
Copy link
Copy Markdown
Member

it requires changing a few more paths

Never mind then, I don't have a strong enough opinion on this to trigger extra work.

@hahnjo
Copy link
Copy Markdown
Member Author

hahnjo commented Jun 21, 2023

I'm willing to spend that extra work if it's the better layout - it's likely to survive quite some time, so I think it's better not to compromise right now...

@hahnjo hahnjo force-pushed the llvm-dir-layout branch from a0b160e to d081664 Compare June 21, 2023 15:47
@phsft-bot

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

@Axel-Naumann

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

@hahnjo

This comment was marked as outdated.

@hahnjo

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

@hahnjo
Copy link
Copy Markdown
Member Author

hahnjo commented Jun 22, 2023

Now everything seems clean, and I will proceed to merge this during the weekend - incremental builds across this change won't work very nicely, I will post a note on Mattermost after it's in.

hahnjo added 4 commits June 24, 2023 09:28
This will be required for future LLVM upgrades that rely on a common
cmake/ directory next to clang/ and llvm/.

This commit is the result of the following commands:
 $ mkdir interpreter/llvm-project/
 $ git mv interpreter/llvm/src/tools/clang/ interpreter/llvm-project/clang/
 $ git mv interpreter/llvm/src/ interpreter/llvm-project/llvm/
 $ git mv interpreter/llvm/llvm-project.tag interpreter/llvm-project/llvm-project.tag
@hahnjo hahnjo force-pushed the llvm-dir-layout branch from d081664 to 7a1b7a2 Compare June 24, 2023 07:31
@phsft-bot
Copy link
Copy Markdown

Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac11/noimt, mac12arm/cxx20, windows10/cxx14
How to customize builds

@phsft-bot
Copy link
Copy Markdown

Build failed on ROOT-debian10-i386/soversion.
Running on pcepsft11.dyndns.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Errors:

  • [2023-06-24T07:33:19.572Z] CMake Error at /home/sftnight/build/workspace/root-pullrequests-build/rootspi/jenkins/root-build.cmake:1142 (message):

@phsft-bot
Copy link
Copy Markdown

Build failed on mac12arm/cxx20.
Running on macphsft26.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

@hahnjo hahnjo merged commit 7a1cc8f into root-project:master Jun 25, 2023
@hahnjo hahnjo deleted the llvm-dir-layout branch June 25, 2023 11:40
maksgraczyk pushed a commit to maksgraczyk/root that referenced this pull request Jun 28, 2023
…3049)

This will be required for future LLVM upgrades that rely on a common
cmake/ directory next to clang/ and llvm/.

The bulk of this change is the result of the following commands:
 $ mkdir interpreter/llvm-project/
 $ git mv interpreter/llvm/src/tools/clang/ interpreter/llvm-project/clang/
 $ git mv interpreter/llvm/src/ interpreter/llvm-project/llvm/
 $ git mv interpreter/llvm/llvm-project.tag interpreter/llvm-project/llvm-project.tag
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

clean build Ask CI to do non-incremental build on PR pr:squash on merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants