build: unvendor gtest

[tobtoht]

This does not need to exist in our source tree.

• gtest is packaged for all relevant distributions
• gtest is available in depends
• updating vendored code is messy and hard to review

[iamamyth]

I cannot be certain as to the original intent of vendoring the library, but my best guess would be a concern regarding version mismatches and the attendant behavior changes: With the vendored version, if a test fails on one platform and succeeds on another, the behavior change likely doesn't owe to differing gtest versions; how much would relying on platform libraries alter that probability calculation?

[tobtoht]

We currently only run tests in this CI job.

The job uses the installed GTest library from Ubuntu, rather than the vendored library. The CMake is configured such that it prefers a platform library if available. I agree that using an unpinned library for (automated) tests is a risk.

The builds in this job are development builds, not release builds. They use different tools and library versions from release builds. The code we test is not equal to the code we ship! We currently do not know if tests would have failed had they been built in a release environment.

What I want to move towards with this PR and #9271 is a way to test release builds with a pinned version of GTest. So that we have a CI job that runs a Guix release build and then runs tests with the GTest as defined in depends.

Now, this PR is mostly concerned with maintainability. I could of course configure CMake to not prefer system libraries as an alternative solution. But, the issue remains that vendored code of this order of magnitude is messier to update than a package definition in depends.

[iamamyth]

I think your strategy for changing CI and testing release builds makes sense. I tend to dislike vendored libraries, so, absent a prior setup, would support your proposal without comment. However, I also think it important to consider the rationale for any decision prior to changing it, so that the change has a solid foundation.

Doing my best to infer the reasoning for the original decision, one concern lingers: When you do not control the execution environment, the test behavior itself can change, which introduces interference. This potential problem manifests in two scenarios:

1. Developers running tests in a "simplified" local environment (e.g. a cmake+make build on a native Linux OS). It would be helpful to support this workflow, as builds take a while, containers require extra resources, etc. That said, you could add support for it later, or simply warn when the system default gtest differs from the expected. In other words, "support" here would only require a) a way to run the same workflow; and b) reasonable confidence that large amounts of time won't be lost due to broken assumptions about the test library.
2. Users running tests on their system to diagnose problems (in particular, releases build the test suite, though maybe don't bundle it, and users can run parts of it). I like this feature, particularly when dealing with C++, as so many things depend on the execution platform, which can be hard for developers to reproduce. I think statically linking the gtest library for release builds achieves the desired result.

[tobtoht]

The vendored GTest library has existed since one of the first commits in this repository: 296ae46 ("moved all stuff to github"). I don't think we'll ever know precisely why it was vendored.

Later that year, in #180, the behavior was changed to prefer using an "external gtest" if available. There are no illuminating comments on that PR explaining the change.

The test suite has preferred to use the system library for over a decade and this has (to my knowledge) not resulted in any bug reports. Tests are known to succeed with the latest version of GTest. Unvendoring GTest would not break a local development workflow.

Instead of always using the system library (as this PR proposes), we could opt to add it as a submodule instead. This way updating the library doesn't involve copy and pasting a bunch of code and it would guarantee that the same GTest version is used in all testing environments.

[iamamyth]

Given that releases presently don't include the test suite, I'm OK with just using the system library, as I agree with your assessment that a submodule would work, if warranted, at a later date. It would be helpful, in my view, to consider generating a separate release artifact which includes the tests with gtest statically linked, but that's outside the scope of your work here.

[iamamyth]

Looks like there are some merge conflicts.

[iamamyth]

Looks like there's a test failure in net_server if I'm reading the output correctly; is that expected/fixed by a different PR or introduced by this one?

[tobtoht]

Happens occasionally, needs further investigation. See also, this run for an unrelated PR.

[tobtoht]

Force pushed without changes to trigger another run, so we can be sure no other problems emerge.

[tobtoht]

It would be helpful, in my view, to consider generating a separate release artifact which includes the tests with gtest statically linked

For testing release builds we would use the GTest from #9271, which is built statically. So this is possible with the current approach.

[iamamyth]

On second thought, I prefer the submodule option:

1. The minimum version available across hosts doesn't support basic features, such as GTEST_SKIP (not available on Debian 10).
2. Retrieving accurate documentation for really old gtest versions proves to be quite a chore, and having the relevant commit info makes it a bit easier.

[tobtoht]

Ok, I'll open another PR for that shortly. Would it be okay if we move forward with this PR, and then close #9271 in favor of the submodule PR?

[iamamyth]

Would it be okay if we move forward with this PR, and then close #9271 in favor of the submodule PR?

Yea, I'm fine with this PR, it still net improves the situation.

[tobtoht]

I decided to wrap this into #9715 to avoid adding gtest packages to CI, only to remove them there.