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

Improve detection of ccache on macOS #97810

Merged
merged 1 commit into from
Dec 13, 2024

Conversation

pafuent
Copy link
Contributor

@pafuent pafuent commented Oct 4, 2024

Before this commit, ccache where only used on Mac when OSXCROSS_ROOT was defined. Now, it could be used even when that environment variable is not defined.

@pafuent pafuent requested a review from a team as a code owner October 4, 2024 13:21
@pafuent pafuent force-pushed the getting_ccache_working_on_mac branch from 4b97022 to a778070 Compare October 4, 2024 13:24
@AThousandShips AThousandShips added this to the 4.x milestone Oct 4, 2024
@AThousandShips AThousandShips changed the title Improving detection of ccache on Mac Improve detection of ccache on Mac Oct 4, 2024
@pafuent pafuent force-pushed the getting_ccache_working_on_mac branch from a778070 to 1d3893c Compare October 17, 2024 13:34
@pafuent
Copy link
Contributor Author

pafuent commented Oct 17, 2024

Friendly remainder

@pafuent pafuent force-pushed the getting_ccache_working_on_mac branch from 1d3893c to c65b350 Compare November 12, 2024 18:25
@pafuent
Copy link
Contributor Author

pafuent commented Nov 13, 2024

Friendly remainder

@pafuent pafuent force-pushed the getting_ccache_working_on_mac branch from c65b350 to db3c8ec Compare November 29, 2024 12:35
@adamscott adamscott requested a review from Repiteo November 30, 2024 20:07
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Code looks good to me.

Note that on my macOS system, CCACHE wasn't set by brew install ccache, so I opted to prepend PATH with its binaries from /opt/homebrew as advised by the post-install message. It works without this PR in this situation, but I think this PR still makes sense as not everyone would want to modify their PATH.

@akien-mga akien-mga requested a review from bruvzg December 2, 2024 13:04
@akien-mga akien-mga modified the milestones: 4.x, 4.4 Dec 2, 2024
Copy link
Member

@bruvzg bruvzg left a comment

Choose a reason for hiding this comment

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

Seems to be working fine, CCACHE usually is not set on macOS and wrappers are used instead (will work without this PR), but you can set it to /opt/homebrew/opt/ccache/bin/ccache (for homebrew install).

Compiler version checks need adjustment ("Couldn't parse CXX environment variable to infer compiler version."), I guess doesn't like space in the executable name.

@akien-mga
Copy link
Member

Compiler version checks need adjustment ("Couldn't parse CXX environment variable to infer compiler version."), I guess doesn't like space in the executable name.

@pafuent Could you look into fixing this too?

@akien-mga akien-mga changed the title Improve detection of ccache on Mac Improve detection of ccache on macOS Dec 12, 2024
Before this commit, ccache where only used on Mac when
`OSXCROSS_ROOT` was defined. Now, it could be used even
when that envirnment variable is not defined.
@pafuent pafuent force-pushed the getting_ccache_working_on_mac branch from db3c8ec to 7c4c110 Compare December 13, 2024 12:51
@Repiteo Repiteo merged commit 9535cd0 into godotengine:master Dec 13, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Dec 13, 2024

Thanks!

@pafuent pafuent deleted the getting_ccache_working_on_mac branch December 15, 2024 02:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants