Are you good with this now @serge-sans-paille ?

@jrtc27 raises a good point - we have discussed deprecating the option but made it a hard error. Should we instead make it a soft error for now and a hard error later on?

Thanks for the review - removed those lists(). Wonder why 2to3 is so aggressive with inserting them? anyway - it seems to parse fine afterwards. but since I don't know the files this should be executed on it's hard for me to test the functionallity.

Removed unecessary lists

My motivation was that people asked to add support for new cache programs (sscache and more). And now that this is supported in upstream CMake in a nice way I think our build system is way too big and complicated to duplicate functionality that's already in upstream CMake. It's more switches to keep testing and update.

Thanks!

Thanks for the review and the explanations! I fixed your nits and landed this.

Thanks for your comments - feel free to comment on the release note, I was struggling with describing the fix well for a short release note paragraph.

• Expand on tests
• Fix crash when Attrs was null
• Added release note

Thanks!

Basically every bot broke with this change, so I reverted it for now and posted to discourse: https://discourse.llvm.org/t/llvm-ccache-build-is-deprecated/68431

Ping.

In D143557#4113334, @thetruestblue wrote:

This seems reasonable and fine to me.
Though I'd feel better if someone familiar with MSVC/Windows gave approval.

Thanks for your review @kwk - I'll merge this now and start working on the action trigger.

Expanded and fixed comments

Fix comment

Thanks for your suggestions. I have pushed a new diff with the fixes.

Incorporated feedback

clearing my queue

Hi - I finally got around to picking this one up again. This is rebased, passing all tests and now we pass the std::error_code directly to the API and changed all the callers to itaniumDemangle() and changed most of the places to actually check the error_code instead of the return const char*.

rebased and implemented some of the suggestions.

@brad is this something you still need for OpenBSD?

Clear my queue.

Thanks!

In D142726#4111037, @kwk wrote:

@thieta I hope my comments make some sense.

• Generate remote and branch names
• Make terminology a lot clearer
• Make cleanup optional

Thanks for working on this! I have a bunch of comments :)

Added warning when the iOS sdk is not found.
Reworded the commit message to reflect the current state of the patch

Add a warning when you try to pass LLVM_CCACHE_BUILD=ON to CMake

Python is already a hard requirement for many things in the LLVM ecosystem, so I don't think that is an argument. Other than that I don't know enough about Bazel to know if this is good or not.

Happy to do a deprecation warning or error.

ping on this @tstellar / @kwk

We could add deprecation warning for this something like:

I fixed the obvious errors and re-testing it and it seems to behave correctly now. Sorry for the noise.

Addressed comments

I have been using "black" to do python formatting in other places. Maybe we should make that part of the LLVM dev guide. But I think it's better if we make formatting changes separately from the functional changes.

Whoops. Bad copy and paste job. I first made it into a macro and did it for all three SDKs and then realised watchos and tvos is off by default and don't need that probing. Then I probably messed it up when removing the macro. Pretty sure I tested it in this form though ... hmm. Anyway I agree with your comments and will address and retest it tomorrow.

A long time later, I finally got time to test this since it was marked as necessary to fix before 16.x release.

Addressed comments

I think this is fine. This warning is not very useful when used in this way.

Seems fine to me. Have you tried clang-format locally since the CI job seems broken?

In D142719#4085605, @tstellar wrote:

In D142719#4085597, @thieta wrote:

I thought the required version is much less than 3.10? Why is this helping / failing?

There is some problem with the environment in the Windows builders where CMake always wants to select the x86 version of the python libraries instead of the x86_64 version. Setting the python-version to the highest supported version on the Windows builders seems to fix this.

we really should centralize the C++ standard variable in top-level cmake or llvm.

I thought the required version is much less than 3.10? Why is this helping / failing?

Took me a while to see the difference between the new and old line. Good spot.

I haven't tried the change - but the errors here makes sense. I bet those objects have references to the other targets and they will differ when we disable anything but Native. If we want to keep these checks we probably need to build all targets for phase 2 or filter the list of objects to compare.

Makes sense to me. We probably should add bolt in there as well since it's enabled by default now, cc @Amir

Good change!

Good plan. Let's merge this and we can do another with lld later.

I think we should include lld as well - now that LLD can produce production binaries on all platforms I think it makes sense to prefer that over system linker.

Thanks for making this change. I think the changes to the bump script looks fine if no processing is needed in the future.

Looks like it's ready to go to me. Maybe take a look at the comments inline in the review and mark them as done or reply to them, there were some questions in there that I couldn't see any replies to.

Oh sorry for the late reply. I think removing the getter makes sense as well - it worked better when we had the earlier revision that copied string out of the Error etc.

It still looks odd to me, but since the rest of the file has comments like that I am fine with it. But I think we should figure out if they are like that because some tool is parsing them or if it's just legacy.

If this fixes OpenBSD it looks fine I think. But I wonder if we shouldn't just do this if we are on OpenBSD, changing the SOVERSION has been fraught with problems before since people have scripts that expect certain layouts.

Is there any point in adding any tests here that can be useful? Probably not since we are working with the Windows API right? I don't have much to comment on, but it seems reasonable if it solves the problem.

I think this is ready to land @Mordante or is there anything else missing?