Updating GoogleTest to v1.14.0?

[kloczek]

@DavidSpickett

Continuing to hammer this point in this tone is not going to help matters and only serves to discourage those who might generously offer their time to improve it.

I’m only forming very simple questions.
Lack of straight answer or avoiding answer is as well almost straight on my questions.

So what is the problem wit use regular gtest?

[pogo59]

I have been trying to answer your questions. Perhaps I need to be more explicit.

@kloczek

So why someone choose to break gtest API?

First off, we’re not “breaking” the gtest API. We’re extending it to support string-handling classes that LLVM defines for its own use. Extending gtest in this way requires modifying gtest source. That’s how gtest is set up; don’t blame us for doing things the way gtest wants us to.

@kloczek

Why EVERY other project which is using gtest was able to use standard gtest and LLVM not?

I would be astonished if EVERY project using gtest used the public version with no customizations or extensions. Gtest is specifically intended to work in many environments and some of them would require modifying the headers that gtest provides in the “custom” subdirectory.

@kloczek

did someone discuss some API extensions with gtest maintainer?

GoogleTest was originally incorporated into LLVM by Google employees, who made the first modifications. You could take this question up with them.

As it stands, we have (with the new patch) a copy of GoogleTest with very straightforward and isolated changes to make it work much better with our project. If you can do better, please provide a patch of your own.

[kloczek]

@pogo59

GoogleTest was originally incorporated into LLVM by Google employees, who made the first modifications. You could take this question up with them.

OK. However probably it was at the time when gtest was not ready to be standalone project with stable API/ABI so maybe it would be good to review this part and check is it possible to adapt LLVM and/or gtest official code to drop chunk of the current LLVM code

I’m asking because it would allow decrease slightly LLVM internal entropy to reducing overhead to maintain the LLVM project in the future.

[kloczek]

@pogo59

First off, we’re not “breaking” the gtest API. We’re extending it to support string-handling classes that LLVM defines for its own use. Extending gtest in this way requires modifying gtest source. That’s how gtest is set up; don’t blame us for doing things the way gtest wants us to.

Yet another thought.
If above is true and LLVN changes are about extending ONLY gtest API IMO chances that some LLVM specyfice gtest will be accepted into gtest upstream IMO chances on such incorporations should be relatively high.
Am I right?

[pogo59]

@kloczek

I’m asking because it would allow decrease slightly LLVM internal entropy to reducing overhead to maintain the LLVM project in the future.

As a maintainer of a downstream fork of LLVM, I am very aware of these issues (check out my talk https://www.youtube.com/watch?v=INCi9gOVMug&index=11&list=PL_R5A0lGi1AA4Lv2bBFSwhgDaHvvpVU21).

FTR, LLVM’s googletest has had very low maintenance cost over the years.

@kloczek

IMO chances that some LLVM specyfice gtest will be accepted into gtest upstream IMO chances on such incorporations should be relatively high.

Once the current patch is in, and we have some assurance that it works well for us, I could imagine proposing an extension point to upstream googletest.

[jrtc27]

@kloczek

I’m only forming very simple questions.
Lack of straight answer or avoiding answer is as well almost straight on my questions.

Be careful, this can lead you down the path towards so-called sealioning.

[mehdi_amini]

@kloczek

I’m asking because it would allow decrease slightly LLVM internal entropy to reducing overhead to maintain the LLVM project in the future.

I don’t think anyone here would object to this, whatever custom code we have for gtest getting upstreamed seem fairly obviously a good thing to do I think.

@kloczek

I’m only forming very simple questions.
Lack of straight answer or avoiding answer is as well almost straight on my questions.

So what is the problem wit use regular gtest?

The best way to figure out is to try to replace the gtest we have with a straight copy from upstream, and see what breaks!
Inspecting the diff and figuring out why each change is there (possibly using git log / git blame to find the revision that introduced the change) is another way to figure out by yourself.

@kloczek

If above is true and LLVN changes are about extending ONLY gtest API IMO chances that some LLVM specyfice gtest will be accepted into gtest upstream IMO chances on such incorporations should be relatively high.

You’re asking the wrong people: this is not a question for LLVM, this is a question for the googletests maintainers. If you want to give a try and create extension points upstream and adjust LLVM accordingly, feel free to give it a try!
It seems to me though to be fairly orthogonal to the question of folks who “just need a new version right now” in this thread.

Ultimately, even if we get it down to “no change from upstream”, deciding to remove it from tree would still be another discussion of course.

[kloczek]

@mehdi_amini

You’re asking the wrong people: this is not a question for LLVM, this is a question for the googletests maintainers.

I’ve not wrote that extension so asking me to explain gtest maintainer why it needs some API extension is a bit misunderstanding of the issue vs. competence context.
Still no one have been trying to explain why gtest needs that extension.
Someone have been trying to LLVV gtest extension from that angle?

My understanding is that gtest adaptation is relatively old so maybe it it will make sense to have look is it possible to use regular current gtest?

If it is possible that will be one step up on ladder of reduce LLVM internal entropy …

The best way to figure out is to try to replace the gtest we have with a straight copy from upstream, and see what breaks!

Agree …

[mehdi_amini]

@kloczek

Still no one have been trying to explain why gtest needs that extension.

I would say you haven’t really tried to answer this yourself, even though people have been pointing you to some elements (here and here!)

Have you looked at the diff and tried to understand it?

@kloczek

If it is possible that will be one step up on ladder of reduce LLVM internal entropy …

Do you have any evidence that there is even a problem to solve here, or are you just operating with some sort of intuition?
I spent more time answering you in this thread than looking at our gtest implementation… in 10y of working on LLVM! I suspect it’s the same for most people around: nobody will even know or notice that gtest is in the repo when working on LLVM.

In any case, if you feel this is a problem to solve: please read the code and work on it, it’s not gonna be useful to just continue to question people here at this point.

[pogo59]

@kloczek

My understanding is that gtest adaptation is relatively old so maybe it it will make sense to have look is it possible to use regular current gtest?

I believe that is exactly what @zeroomega did. So, continuing to raise the same questions on this thread has zero to negative value.

Your next productive steps would probably be to look at the specific edits to see what was done. The README.LLVM files have one-liners describing the motivation for these edits. If you want to propose different solutions to eliminate the edits, then file an issue or propose a patch showing how to achieve that while still supporting all the existing Clang/LLVM unit tests.

[petrhosek]

I think we need to look at every change separately and figure out what’s the right solution:

• Support for std::begin/std::end in gmock-matchers.h is something that could probably be directly upstreamed since it is a generally useful addition.
• Support for raw_os_ostream in gtest-message.h would likely need a new extension point which should be doable but likely needs to be discussed with GoogleTest maintainers.
• For custom printers in include/gtest/internal/custom/gtest-printers.h, I’m not sure if those need to be there since according to GoogleTest documentation they could live next to the type.

We shouldn’t block the GoogleTest update on figuring this out though, each one of these could be pursued separately.

[pogo59]

I’ve added issue #66268 for the first point.

It’s not clear to me that the second (gtest-message.h) and third (gtest-printers.h) points are separate, but then I haven’t tried to understand those changes in detail. I’ve added issue #66273 for this point.

I agree that we don’t want to block the current update in progress on these issues. They can be pursued separately.

Looking at the gist again, it seems like we might have had to replace the upstream CMakeLists.txt to make it play nicely with the rest of the LLVM project. If that’s true, it’s another potential issue to resolve. Again, should not block the current update.