Skip to content

[clang][python] Don't add check-clang-python to check-all if cross-compiling #111657

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.

Sign up for GitHub

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 4 commits into from
Oct 13, 2024
Merged

[clang][python] Don't add check-clang-python to check-all if cross-compiling #111657

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
afd9488
[clang][python] Don't add check-clang-python to check-all if cross-co…
asb Oct 9, 2024
8ca43d1
Fix typo of CMAKE_CROSSCOMPILING
asb Oct 13, 2024
3653541
Fix typo and refine wording when check-clang-python not added to chec…
asb Oct 13, 2024
3f21040
Add FIXME based on review discussion
asb Oct 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions clang/bindings/python/tests/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,14 @@ if(${LLVM_NATIVE_ARCH} MATCHES "^(AArch64|Hexagon|Sparc|SystemZ)$")
set(RUN_PYTHON_TESTS FALSE)
endif()

# Tests will fail if cross-compiling for a different target, as tests will try
# to use the host Python3_EXECUTABLE and make FFI calls to functions in target
# libraries.
if(CMAKE_CROSS_COMPILING)
Copy link
Contributor

@Endilll Endilll Oct 9, 2024

Choose a reason for hiding this comment

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

If there's nothing fundamental preventing this from working, I'd like to left a FIXME, too.

Copy link
Contributor Author

@asb asb Oct 9, 2024

Choose a reason for hiding this comment

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

I think fundamentally it can't really work. We could add a search for a python interpreter built for the target in the sysroot, and add logic to use it if we know we can execute it with qemu-user...but I'm not sure it's worth that kind of complexity and I personally prefer to minimise dependency on the sysroot as much as possible other than providing some needed libraries and headers. A FIXME could suggest adding some logic for specifying a guest python3_executable, but I'm not sure that's a path we'd want to commit to. So I'm a bit torn.

Copy link
Contributor

@Endilll Endilll Oct 9, 2024

Choose a reason for hiding this comment

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

Do I understand correctly that you don't have this kind of problem with "regular" tests in clang/test? If so, I think it'd be novel to say that tests for entire components are second-class citizens simply because said components are written in Python. As far as commitments go, I think our default here is to support all kinds of tests.

That said, I understand that complexity is there (it appears that nothing is easy in cross-compiling), and what you described doesn't seem to be too convoluted, given the inherent complexity of the problem. We can leave this out of scope of this PR, but I'd expect that claiming official support for cross-compiling environments is going to be blocked on this.

Copy link
Contributor Author

@asb asb Oct 9, 2024

Choose a reason for hiding this comment

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

Yes, I'm trying for now to just make changes that remove the small bits of friction for cross-compiling and then testing using qemu-user as a litmus test rather than (for now) make it "officially supported".

It is indeed correct that there's no problem with the regular tests in clang/test (other than clang/test/Driver/env.c if you need QEMU_LD_PREFIX passed through, as the test strips envvars - but of course for the lit-orchestrated tests we already have a mechanism to skip tests downstream by configuring the LIT_ARGS).

Copy link
Collaborator

@AaronBallman AaronBallman Oct 9, 2024

Choose a reason for hiding this comment

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

If I understand correctly, I think we probably do want a FIXME for clarity because we'd ultimately like the python tests to work in a cross-compile situation because I don't think there should be a distinction between running the C binding tests and the Python binding tests. Potentially we might be able to do this by hooking them in to the lit-orchestrated tests and using LIT_ARGS the same as the Clang tests?

Regardless, nothing has to be done for now beyond adding a FIXME comment; and if we decide the complexity is too great when digging into doing that effort, we can always walk back the comment.

Copy link
Contributor Author

@asb asb Oct 13, 2024

Choose a reason for hiding this comment

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

Thank you both for the comments, I've added a FIXME now that notes the issue without indicating a particular solution has been chosen.

message(WARNING "check-clang-python-tests not added to check-all as they fail in a cross-build setup")
set(RUN_PYTHON_TESTS FALSE)
endif()

if(RUN_PYTHON_TESTS)
set_property(GLOBAL APPEND PROPERTY
LLVM_ALL_ADDITIONAL_TEST_TARGETS check-clang-python)
Expand Down
Loading