Support system libs #92
Conversation
CMakeLists.txt
Outdated
| FetchContent_MakeAvailable(googletest) | ||
| endblock() | ||
| else() | ||
| find_package(GTest REQUIRED) |
There was a problem hiding this comment.
Is there never a case where we don't need google test? I'm actually hoping projects use something else mostly
There was a problem hiding this comment.
Projects that test another way wouldn't need googletest, I suppose. In particular, projects providing entirely constexpr features wouldn't benefit from a framework like googletest, so the extra dependency would get removed. Also, some libraries would be too low level to justify a dependency on a test framework that pulls in vectors and serializers and such.
I would still expect a CMake test target to exercise a "test suite" of some sort, even if that's a series of example main functions that get compiled and executed.
There was a problem hiding this comment.
entirely constexpr features wouldn't benefit from a framework
Well, in my experience with this in boost date-time I still had runtime tests along with the constexpr validation tests -- of course in that case the runtime tests came first and constexpr later. Still, with a trust-but-verify thinking I'd still want both compile and runtime covered -- but I wouldn't need/use the massive google test for simple test cases.
At the moment I'm not sure how to resolve the issue -- perhaps we leave a comment that says 'replace test framework here' -- not sure. In the end I wouldn't want to leave the impression that gtest is the sanctioned test framework.
There was a problem hiding this comment.
I think you have valid concerns, Jeff. Let's keep iterating until we have something we're happy with.
That being said, the current PR is entirely about not getting the build to hang given the dependency. Are OK branching off another discussion so we can resolve the bug noted in the meantime?
|
except for my question, I have no issues with the PR |
|
FetchContent already has this functionality built-in. I don't think we need this PR. See FETCHCONTENT_TRY_FIND_PACKAGE_MODE. |
|
Interesting -- would it be more correct to say we need a different PR? I think there's still a bit of an issue in the original cmake code. |
Can you elaborate on that? |
|
yeah, sorry -- what I mean is the file as is unconditionally requires google test -- so one might get the impression that it's a requirement for any testing when it's not. I suppose it's fine to leave for now. @camio are you suggesting we abandon this PR? |
The file as-is only pulls in google test when
Yes |
Problem ------- **Given**: A sandbox environment is an environment where dependencies are all provided but access to the internet is not. Also take as a given that the build for this project happens in an environment that meets that description. **When**: Configuring this project with any of the provided approaches. For instance: ``` cmake -B build -S . ``` **Then**: The configuration step hangs or fails, depending on the nature of the sandboxed environment. Solution -------- Alter `FetchContent_*` calls in this project to use `GTest` as the project name and nothing else. This is important to allow the `FETCHCONENT_TRY_FIND_PACKAGE_MODE` feature to work as designed. This allows the human or tool building this project to explicitly disable operations involving calls to `git` or other off-box I/O. The documentation in the README was also updated to include workflow tweaks for building against packaged versions of either existing GoogleTest source code or prebuilt GoogleTest libraries.
bretbrownjr
force-pushed
the
support-system-libs
branch
from
5bf523d to
282e91f
Compare
|
I don't believe the failing |
bretbrownjr
requested review from
camio,
dietmarkuehl,
neatudarius and
steve-downey
as code owners
Looks like the MSVC family of compiler is having trouble finding google test dependency.
I will try to investigate. btw bret can you check your email/ linkedin? @bretbrownjr Edit: confirmed MSVC family of CI fails is caused by this change. I manually triggered the workflow and it ran without fault. See: manually triggered workflow Is *-debug_deps\gtest-src\googletest\src\gtest-all.cc well formed on other presets? Maybe other compilers are just smarter and searched more path. |
| @@ -325,6 +325,30 @@ cmake -B build -S . -DBEMAN_EXEMPLAR_BUILD_TESTING=OFF | |||
|
|
|||
| </details> | |||
|
|
|||
| <details> | |||
| <summary> Disable `git clone` operations </summary> | |||
There was a problem hiding this comment.
Nit: maybe Disable pulling Google Test from GitHub would be a more clear summary here.
There was a problem hiding this comment.
It's not specifically GitHub that is the issue -- it's the off-box I/O as such.
Is the confusion that it's not obvious that git clone is being invoked by FetchContent? Possibly we need to update some comments in the CMakeLists.txt to make that more clear.
| For instance, in Ubuntu, source code for GoogleTest is provided via a | ||
| debian-style package under the directory `/usr/src/googletest`. To | ||
| use those sources instead of cloning other sources from the internet, | ||
| provide `-DFETCHCONTENT_SOURCE_DIR_GTEST=/usr/src/googletest` as a |
There was a problem hiding this comment.
Don't forget to add a check in CI that this works. There's no way to assert there's no actual fetching, but it would be nice to make sure this works with a well-formed setup.
See: https://github.com/bemanproject/exemplar/blob/main/.github/workflows/ci_tests.yml#L142
There was a problem hiding this comment.
You could technically config a void proxy for git and then test it.
There was a problem hiding this comment.
That would involve interesting CI changes in each case, possibly on another PR?
- The Ubuntu images would need the GoogleTest package installed for these flags to work as designed
- We haven't selected a package manager on Windows -- though we should add support for building against Conan and vcpkg dependencies (GoogleTest in this case) as you and I discussed in person a little bit yesterday
Given we're dealing with an existing bug, I think it's OK to resolve the bug (once we can figure out why Windows is being weird) and circle back on a regression prevention test, though maybe @bemanproject/leads want to chime in.
There was a problem hiding this comment.
That's fair, I thought it would be as easy as git clone https:github.com/google/googletest /usr/libs/googletest
I usually do prefer having all CMake parameters tested (I don't think multi platform is that important here, the goal is to show the project could build under the parameters). When they break, they break silently, which is very annoying.
There was a problem hiding this comment.
That's fair, I thought it would be as easy as git clone https:github.com/google/googletest /usr/libs/googletest
Fair. That's technically an exercise of the workflow for one of the two documented use cases. Though it's the functional equivalent of re-implementing a worse version of FetchContent, so I'd still rather hold off until we have packaged GoogleTest source to build against as such. If for no other reason, to leave time to work on more meaningful problems elsewhere in this repo.
|
Ubuntu currently will install googletest into /usr/src if you apt install
it. It should be locateable once that's done. We do a bunch of package
install already, one more should not be a problem.
…On Fri, Jan 24, 2025, 10:41 River ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In README.md
<#92 (comment)>:
> @@ -325,6 +325,30 @@ cmake -B build -S . -DBEMAN_EXEMPLAR_BUILD_TESTING=OFF
</details>
+<details>
+<summary> Disable `git clone` operations </summary>
+
+By default, while tests are enabled, this project will clone GoogleTest
+and build it from source. In many environments, it is better to use an
+already available GoogleTest instead.
+
+### Existing Source Code
+
+For instance, in Ubuntu, source code for GoogleTest is provided via a
+debian-style package under the directory `/usr/src/googletest`. To
+use those sources instead of cloning other sources from the internet,
+provide `-DFETCHCONTENT_SOURCE_DIR_GTEST=/usr/src/googletest` as a
That's fair, I thought it would be as easy as git clone https:
github.com/google/googletest /usr/libs/googletest
I usually do prefer having all CMake parameters tested (I don't think
multi platform is that important here, the goal is to show the project
could build under the parameters). When they break, they break silently,
which is very annoying.
—
Reply to this email directly, view it on GitHub
<#92 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAVNZ5WTSPH5QOCBRJGEIFT2MJNLPAVCNFSM6AAAAABUKDVQPOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDKNZSHE4TEMBYHA>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
|
Just to save people time combing through the logs. Here is a more technical and detailed bug description for the CI failing here. The failing compile command is: The error in the log is: Notably the source file being built is at: Which would lead me to believe we would expect to see an include directory like this in the compile command: See the relevant path in the googletest repo for reference. But we don't see that include path. We see these instead, presumably intended to point at the |
|
So. @camio. I think using this approach to support First, the latest commit changes the Second, Third, Fourth, the Fifth, it's worth noting that Finally, CMake is case insensitive largely, so don't get hung up on the distinction between "GTest" and "gtest" in correlating the above. Anyway, all this leads me to believe that, in the currently proposed patch:
This concludes my submission for the 2025 international CMake debugging championships. EDIT: I forgot to mention that I suspect that case insensitive filesystems could explain why this only happens on Windows, though I haven't identified the precise path or filesystem at play here. |
|
As to solutions... my personal opinion is and has been that putting To that end, we could move logic that provides The option that is the least amount of work is to go back to the previous implementation on this PR -- reintroduce a user-facing option to use FetchContent or not for GoogleTest. |
|
Oh, and as to why only failing in Windows... case insensitive filesystems? |
|
Since there are design and/or CMake practices questions now, this PR is not ready to merge. I'm moving it back into draft status. |
Problem
Given:
A sandbox environment is an environment where dependencies
are all provided but access to the internet is not. Also take as
a given that the build for this project happens in an environment
that meets that description.
When:
Configuring this project with any of the provided approaches. For
instance:
Then:
The configuration step hangs or fails, depending on the nature of
the sandboxed environment.
Solution
Alter
FetchContent_*calls in this project to useGTestas theproject name and nothing else. This is important to allow the
FETCHCONENT_TRY_FIND_PACKAGE_MODEfeature to work as designed.This allows the human or tool building this project to explicitly
disable operations involving calls to
gitor other off-box I/O.The documentation in the README was also updated to include
workflow tweaks for building against packaged versions of either
existing GoogleTest source code or prebuilt GoogleTest libraries.