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: Simplify ccache logic #577

Merged
merged 1 commit into from
Apr 7, 2024
Merged

cmake: Simplify ccache logic #577

merged 1 commit into from
Apr 7, 2024

Conversation

diizzyy
Copy link
Contributor

@diizzyy diizzyy commented Mar 31, 2024

  • Use USE_CCACHE switch, this seems to be a more common option having a quick look using Google
  • Make use of find_program() functionality introduced in CMake 3.18 to simplify code

Copy link
Member

@sjaeckel sjaeckel left a comment

Choose a reason for hiding this comment

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

DDG doesn't turn up a single hit for USE_CCACHE but G apparently finds some.

This rename falls under API breakage IMO, i.e. it must be backported to 1.3 and a warning must be added that it will change in the future.

This PR doesn't only simplify code, but also changes behavior.

I don't like the functionality change where the old solution defaults to enable its usage if ccache is found and with your change it's disabled by default. As there was no comment describing why this was done, I suspect that this was accidental!?

Also: please fix CI :)

@diizzyy
Copy link
Contributor Author

diizzyy commented Apr 3, 2024

I think the "most" correct way would be to rip it out completely and leave it up to the user compiling it or the framework to specify rather than blindly looking for a binary. https://stackoverflow.com/questions/1815688/how-to-use-ccache-with-cmake
Edit: Engrish ;-)

I can go either way :)

@sjaeckel
Copy link
Member

sjaeckel commented Apr 4, 2024

I think the "most" correct way would be to rip out of completely and leave it up to the user compiling it or the framework to specify it

Thanks, that's exactly what we should do.

Do you wanna modify this PR to simply drop it or should I?

@diizzyy
Copy link
Contributor Author

diizzyy commented Apr 4, 2024

I'll update the PR, thanks! :)

Rely on CMAKE_C_COMPILER_LAUNCHER instead of homegrown logic

See CMake's documentation for more information
https://cmake.org/cmake/help/latest/variable/CMAKE_LANG_COMPILER_LAUNCHER.html
@diizzyy
Copy link
Contributor Author

diizzyy commented Apr 5, 2024

Updated

freebsd-git pushed a commit to freebsd/freebsd-ports that referenced this pull request Apr 6, 2024
- remove built-in ccache detection, see
  libtom/libtommath#577
- put OPTIMIZED_CFLAGS and LTO in OPTIONS, on by default

PR:		278155
Reported by:	diizzy
@sjaeckel sjaeckel closed this Apr 7, 2024
@sjaeckel sjaeckel reopened this Apr 7, 2024
@sjaeckel sjaeckel merged commit 42b3fb0 into libtom:develop Apr 7, 2024
165 checks passed
@sjaeckel
Copy link
Member

"Backported" via 5410d0b

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.

2 participants