Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support system libs #92

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Support system libs #92

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,14 +31,14 @@ include(GNUInstallDirs)
if(BEMAN_EXEMPLAR_BUILD_TESTS)
# Fetch GoogleTest
FetchContent_Declare(
googletest
GTest
GIT_REPOSITORY https://github.com/google/googletest.git
GIT_TAG
f8d7d77c06936315286eb55f8de22cd23c188571 # release-1.14.0
EXCLUDE_FROM_ALL
)
set(INSTALL_GTEST OFF) # Disable GoogleTest installation
FetchContent_MakeAvailable(googletest)
FetchContent_MakeAvailable(GTest)
endif()

add_subdirectory(src/beman/exemplar)
Expand Down
25 changes: 25 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,6 +325,30 @@ cmake -B build -S . -DBEMAN_EXEMPLAR_BUILD_TESTING=OFF

</details>

<details>
<summary> Disable `git clone` operations </summary>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: maybe Disable pulling Google Test from GitHub would be a more clear summary here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could technically config a void proxy for git and then test it.

See: https://gist.github.com/evantoli/f8c23a37eb3558ab8765

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

configuration argument when calling `cmake`.

### Existing Binaries

In another case, again in Ubuntu, GoogleTest can be installed prebuilt
with CMake discovery support deployed in normal search paths.
Setting `-DFETCHCONTENT_TRY_FIND_PACKAGE_MODE=ALWAYS` when calling `cmake`
will force the build of this project to locate that installed library.

</details>

## Integrate beman.exemplar into your project

<details>
Expand Down Expand Up @@ -379,6 +403,7 @@ Build systems that support interoperation via `pkg-config` should be able to det

</details>


## Contributing

Please do! Issues and pull requests are appreciated.
Loading