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.

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

CMake: add TESTING_USE_NETWORK configure option #4220

Merged
merged 2 commits into from
Aug 13, 2024

Conversation

mwtoews
Copy link
Member

@mwtoews mwtoews commented Aug 12, 2024

This changes how CMake configures a build, where by default USE_NETWORK=OFF assumes use of the network is not permitted, which disables fetching GTest (if needed) and running network-dependant tests. The user (or packager) would need to opt-in with USE_NETWORK=ON to enable these features, which were the default for previous CMake-based PROJ builds.

Update: This PR now adds CMake TESTING_USE_NETWORK configure option to permit or disable network use, with a default ON to assume the network can be used to fetch GTest dependency (if needed) and run network-dependant tests.

This also finally cherry-picks #4153 to document test dependencies, which incorporate new requirements from #4131.

Closes #4219

@rouault
Copy link
Member

rouault commented Aug 12, 2024

where by default USE_NETWORK=OFF

I would rather use ON by default, as packagers' constraints are not necessarily the ones of "regular" PROJ users/developers. If we keep the default to OFF, we should change our CI configurations (at least some of them) to set it to ON to avoid reducing our test coverage.

And the option should perhaps be renamed to something like TESTING_USE_NETWORK to limit the risk of confusion with PROJ_NETWORK

@mwtoews
Copy link
Member Author

mwtoews commented Aug 12, 2024

As for default ON/OFF, I'm open for opinions. I'd agree there are likely more that would prefer it to be ON, as PROJ is a web-enabled tool built on CURL.

I'll tune the CI options next (tomorrow), just testing to see what happens with changing the defaults. The result of a default OFF is a few CMake configure warnings, and fewer tests, but no failures. For example:

...
-- Looking for GTest
CMake Warning at test/unit/CMakeLists.txt:92 (message):
-- Looking for GTest - not found
  Tests that require GTest will be skipped; either install GTest dependency,
  or if possible, set USE_NETWORK=ON to fetch content from GitHub
...

A suggested rename to TESTING_USE_NETWORK seems ok, as it applies only to testing-related aspects. But if I'd go with that if we decide to keep default ON, otherwise it's a long option that a user would need to type to enable ON if that were not the default.

@mwtoews
Copy link
Member Author

mwtoews commented Aug 12, 2024

I see the 👍 votes above to keep the default behavior that CMake can use the network during configure and test. Let me know if there are any objections, otherwise I'll re-write this PR using a default TESTING_USE_NETWORK=ON

(i.e. only users/packagers that do not permit network access would need to specify TESTING_USE_NETWORK=OFF during CMake configure)

@mwtoews mwtoews changed the title Add USE_NETWORK CMake configure option CMake: add TESTING_USE_NETWORK configure option Aug 13, 2024
@mwtoews mwtoews marked this pull request as ready for review August 13, 2024 10:37
@mwtoews
Copy link
Member Author

mwtoews commented Aug 13, 2024

Some other notes on this PR:

  • Generally tests (via ctest) should always be run, but the number of tests may vary between 44 to 68 depending if the network is available, if GTest is available, if Python with YAML is available. There is one bonus test add if pytest is available, which self-tests the CLI tester.
  • Warnings are shown with cmake configure in a few scenarios, e.g. is no network is detected but GTest is needed
  • Option RUN_NETWORK_DEPENDENT_TESTS has existed for a while, but this is moved outside an if-block.
  • If 'curl' or 'ping' are not available then assume there is no network (Debian-like users can get these with apt install -y iputils-ping curl)
  • If no network is detected (missing curl/ping), but it does work, then RUN_NETWORK_DEPENDENT_TESTS=ON can be specified to run nkg (the only network-dependant test)
  • Many different combinations of configurations were tested locally, but I don't feel these test scenarios need to be replicated in CI.

@rouault rouault added this to the 9.5.0 milestone Aug 13, 2024
@rouault rouault merged commit 64ece97 into OSGeo:master Aug 13, 2024
22 of 23 checks passed
@mwtoews mwtoews deleted the cmake-use-network branch August 13, 2024 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change network dependence on configure / build time
2 participants