Skip to content

Add find-ccache.sh with find_ccache_and_export_vars #1524

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

Merged
merged 15 commits into from
Aug 16, 2024

Conversation

eramongodb
Copy link
Contributor

@eramongodb eramongodb commented Jan 26, 2024

Followup to #1522. Verified by this patch.

This PR proposes three sets of changes:

Remove ccache 3.4.3 detection

After recent updates to our EVG config to migrate to newer and recommended distros, none of the distros we currently use in our EVG config appear to provide a ccache version that fails this condition. Therefore, the workaround is removed for simplification purposes.

Remove CCache.cmake

This PR proposes moving the routines to opt-into ccache out of the CMake config and into our EVG scripts instead. CMake 3.17 introduced the CMAKE_<LANG>_COMPILER_LAUNCHER environment variables as a convenient, non-intrusive method of enabling ccache. Pre-3.17 CMake (i.e. in the build-and-test-with-toolchain tasks, which uses 3.1.0 (should we update this to 3.15?)) will simply ignore the env vars without emitting "manually-specified variables were not used by the project" warnings as would be the case when -D is used. Since most of our EVG scripts use a "latest" version of CMake (currently set to 3.25.3), most of our EVG tasks will correctly take advantage of these env vars when set.

This change is motivated by a CMake principle of keeping the config as simple as possible (this PR removes the custom MONGO_USE_CCACHE option), instead utilizing supported CMake configuration variables whenever able to minimize making assumptions about the user environment. (Ultimately, we should move the env vars and -D flags into dedicated CMake toolchain files for use on EVG.) If instead we want to keep ccache detection routines in the CMake configuration rather than using env vars (the status quo, i.e. for user convenience), this change can be reverted.

Add find-ccache.sh

The repetition of setting ccache variables to support reuse of compilation results across different build directories + ccache detection motivated the addition of a find-ccache.sh script (a la find-cmake-latest.sh). The find_ccache_and_export_vars() conveniently performs both a simple search for a ccache binary and the exporting of all necessary ccache-related environment variables (for both CMake and ccache itself).

The find_ccache() function is motivated by the realization that some of the distros we test on, despite not providing a system-installed ccache binary, do provide ccache via the MongoDB server toolchain instead (which is present on many non-Windows distros, with the caveat that there are no guarantees of stability/support for non-server teams). This function therefore helps slightly extend the set of distros where we can take advantage of ccache. (Note: extending ccache availability to Windows distros is requested in DEVPROD-4074.) The odd use of || and true in the script is to avoid potential interference by set -o errexit in parent scripts.

@eramongodb eramongodb requested a review from kevinAlbs January 26, 2024 16:45
@eramongodb eramongodb self-assigned this Jan 26, 2024
Copy link
Collaborator

@kevinAlbs kevinAlbs left a comment

Choose a reason for hiding this comment

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

(i.e. in the build-and-test-with-toolchain tasks, which uses 3.1.0 (should we update this to 3.15?))

I think "yes". The build-and-test-with-toolchain task was intended to test the minimum versions of dependencies. I think it can be updated to 3.15 to match the cmake_minimum_required (VERSION 3.15).

If instead we want to keep ccache detection routines in the CMake configuration rather than using env vars (the status quo, i.e. for user convenience), this change can be reverted.

I do not yet have a strong opinion. @vector-of-bool do you have an opinion? I can tie-break if needed.

this PR removes the custom MONGO_USE_CCACHE option

I suggest updating the NEWS file to note the upcoming removed option. This may give users a heads up to change the build configuration if they are benefitting from the automatic ccache detection. Example:

libmongoc 1.26.0 (Unreleased)
=============================

Build Configuration:

* Remove `MONGO_USE_CCACHE`. Use the environment variables `CMAKE_<LANG>_COMPILER_LAUNCHER` to specify `ccache`.


# Find, export, and set ccache env vars in one command for convenience.
# Requires base_dir as first argument.
# Returns a non-zero (false) value a ccache binary is not found.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# Returns a non-zero (false) value a ccache binary is not found.
# Returns a non-zero (false) value if a ccache binary is not found.

@eramongodb eramongodb requested a review from kevinAlbs February 9, 2024 22:40
Copy link
Contributor

@vector-of-bool vector-of-bool left a comment

Choose a reason for hiding this comment

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

Sorry for the absurd delay on this. Life, uh, gets in the way. LGTM

@eramongodb eramongodb merged commit 35f3880 into mongodb:master Aug 16, 2024
48 checks passed
@eramongodb eramongodb deleted the cdriver-ccache branch August 16, 2024 18:11
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.

3 participants