WFM. Thanks!

Thank you. 16.x isn't affected.

This commit broke building against libclang-cpp.so dylib. I've submitted a fix as D146427 but it haven't received any attention so far.

Ping.

Do you have a doc link or some precedent? Something to help us understand the implications of this.

LGTM. Thanks!

First observation: third-party/unittest/UnitTestMain/CMakeLists.txt still has BUILDTREE_ONLY which makes it impossible to use this with LLVM_DISTRIBUTION_COMPONENTS. Could you adjust that as well?

I think it's good.

I haven't been able to get Halide to configure successfully yet but I already spotted something suspicious. dependencies/llvm/CMakeLists.txt does:

Ok, this turned out to be surprisingly painless, at least within our packaging.

While the general direction seems sound, I suspect this may be a real PITA given that for us:

In D140315#4077193, @yaxunl wrote:

@scchan the test expects CLANG_INSTALL_LIBDIR_BASENAME to be lib. Maybe in the failed test it was not true.

I'm sorry for reporting it this late (clang's been broken for over two weeks, so we couldn't periodically test it) but this change is causing test regressions on Gentoo/amd64:

Thanks for the superfast review! I'll push it later today.

In D141581#4072692, @kwk wrote:

In D141581#4071617, @thesamesam wrote:

This is currently holding back further testing on our end which is concerning me a bit, especially as we approach the branch point. Could you revert this please if a fix isn't imminent? Thank you!

@thesamesam what do you mean with "revert"? Just by looking at this differential I cannot see that this patch has landed. Has it?

Are you sure about this? FWIU right now the default is to "use Z3 if available". If I read the patch right, it will be "default to requiring Z3, unless explicitly disabled". Not that I mind but I suppose it's a semantic change.

In rGac1ffd3caca12c254e0b8c847aa8ce8e51b6cfbf#1164605, @fpetrogalli wrote:

Let me know if you think I should apply D141581

Ping. Please either fix this or revert and put the new version back for review.

In rGac1ffd3caca12c254e0b8c847aa8ce8e51b6cfbf#1164097, @fpetrogalli wrote:

Hi @mgorny - thank you for the heads up. Would this fix the issue? https://reviews.llvm.org/D141581

This version breaks standalone builds of clang:

It looks good to me as-is.

I think it makes sense but I'd like to get @MaskRay's opinion as well.

Our policy is to not accept patches where nobody is willing to provide a CI bot, which is the case here currently.

I'm wondering if we shouldn't just by setting CLANG_NO_DEFAULT_CONFIG=1 like in clang's test suite rather than tying to predict how different config files will affect these tests.

Thanks!

I've pushed a fix in dab67c66932b9149842f7c8431e951f952125fc0, based on @jhuber6's fix from the other diff.

Thank you. I'm going to try if the same solution helps with D139723 too.

In D140155#4004505, @doru1004 wrote:

Commit 658ed9547cdd6657895339a6c390c31aa77a5698

Also, please link commits to diffs using Differential Revision: trailer in the future.

In D139723#4004536, @doru1004 wrote:

Commit: 07ff3c5ccce68aed6c1a270b3f89ea14de7aa250

Use explicit mapping as requested.

We also have the same problem for __string unless I am mistaken, @mgorny

In D139270#4005137, @ldionne wrote:

Let's use __tuple_dir for LLVM 16 and rename it back to __tuple in LLVM 17. We also have the same problem for __string unless I am mistaken, @mgorny
can you handle that in your patch?

In D139270#4003978, @tschuett wrote:

Would __tuple_helpers or __tuple_utils be less ugly resp. obvious solutions to the challenge?

Thinking about it more, probably the cleanest long term solution would be to simply use .h suffix for files but that obviously won't solve the existing upgrade problem.

Rebased.

That question can be turned around; why should software authors need to rename their source files to work around deficiencies in package managers?

Use __tuple_dir instead of the icky __tuple2.

Package managers track all installed files, and remove files that are no longer installed by the specific package version. However, the removal happens *after* installing the new version.

Gentle ping. Do you have any specific requests on how this could be improved? I'd love to see the problem fixed before the next weekend (that's when I do snapshots for Gentoo).

@MaskRay, I've reverted your M68k test fix since this change was reverted too, to get M68k tests to pass again.

Well, I'd prefer renaming them permanently but one release would be good enough for Gentoo. Alternatively, at least for private headers we could streamline this by using a distinct prefix/suffix for directories, e.g. __tuple for a file, and __tuple_dir for directory. This would probably be cleaner than temporarily appending 2.

Well, it fixes the problem for me as well. I don't know the code but FWICS you've added the original assumption, so I guess you know best.

Regenerate files.

I've regenerated files. However, I needed to modify the generator to strip the extra 2 from public header name. That's not the prettiest solution, I admit, so I'm open to better ideas.

In D138792#3966920, @Hahnfeld wrote:

Hi, I bisected this change to lead to a couple of test failures when building with LLVM_LINK_LLVM_DYLIB. In the past, this had to do with global variable initialization order, but nothing immediately jumps to my eye in this patch. Is AARCH64_ARCH used to define global variables?

LGTM

Thanks a lot for doing this!

Closing in favor of D139107.

Replacing a file with a directory (and the other way arond) is problematic in general. E.g. if you attempt to ninja install it on top of the existing installation:

Thanks. I can confirm that the two tests pass for me with this patch.

Thanks!

Thanks!

Thank you!

@thesamesam, could you test it on PPC32, please?

@phosek, very gentle ping ;-).

Plus there's the matter of LLVMTestingSupport library.

I'll try to test this today. I suppose the main idea would be to enable LLVM_INSTALL_GTEST, then stop unpacking third-party directory when building other packages, correct? Perhaps it would make sense to update the checks like the one in clang while doing that:

LGTM