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

Add CMAKE_FIND_USE_INSTALL_PREFIX=OFF to our CMake flags #22315

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

Conversation

pipcet
Copy link

@pipcet pipcet commented Nov 18, 2024

TL;DR Can we apply this patch to a test system and see whether it makes things better or worse?

Here's the issue:

  1. start with a fresh docker container; note that it doesn't (and shouldn't) contain a "ccache" binary in the path. (I used sha256:25e51469574b174a940bc7970cba71d8f96ce228c8ddf7a60aedd61f32c87c68)
  2. ./build-package.sh -I ccache
  3. ./build-package.sh -I swiftshader
  4. /bin/sh: 1: ccache: not found

The build process assumes
there's a "ccache" binary in the path, even though the only copy of
"ccache" is that in the /data directory. The problem is that CMake's
find_program searches the install prefix unless
CMAKE_FIND_USE_INSTALL_PREFIX is set, even though
CMAKE_FIND_ROOT_PATH_MODE_PROGRAM=NEVER is also set and affects the
same directory.

Problems:

  1. CMake doesn't document whether find_program is ever validly used
    to search for target executables rather than host executables. If
    that is the case, this change might break other packages.
  2. swiftshader is broken. Its CMake code looks like this:
# Use CCache if available
find_program(CCACHE_FOUND ccache)
if(CCACHE_FOUND)
    message(STATUS "Using ccache")
    set_property(GLOBAL PROPERTY RULE_LAUNCH_COMPILE ccache)
    set_property(GLOBAL PROPERTY RULE_LAUNCH_LINK ccache)
endif()

Obviously, it's perfectly possible for ccache to be a findable program
but not reside on the PATH, but the code assumes it's always in the
cache.

(It's generally a bad idea to automatically enable ccache without an
explicit flag, but that's a swiftshader problem, and we have to live
with the broken CMakeLists.txt provided by the package maintainer)

This popped up while working on #22305, but is unrelated.

Copy link
Member

@TomJo2000 TomJo2000 left a comment

Choose a reason for hiding this comment

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

That makes sense to me.
We wouldn't want to use programs from the /data/* directory as they are cross-compiled for Termux.

We may be implicitly relying on that for some packages but that is purely speculation on my part.

@Biswa96
Copy link
Member

Biswa96 commented Nov 19, 2024

Could you please clarify whether this change might create any issues with the on-device package build?

@pipcet
Copy link
Author

pipcet commented Nov 19, 2024

Could you please clarify whether this change might create any issues with the on-device package build?

Good point. Maybe we should move this change to the section:

 	if [ "$TERMUX_ON_DEVICE_BUILD" = "false" ]; then

@TomJo2000 , does that make sense to you? I don't know anything about on-device termux builds...

@TomJo2000
Copy link
Member

Right I think that's what was nagging at the back of my mind.
During on-device builds those are valid for CMake to pick.

@TomJo2000

This comment was marked as off-topic.

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