Skip to content

Specify target_compatible_with to qt_build_test #1245

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 1 commit into from
Apr 10, 2025

Conversation

yukawa
Copy link
Collaborator

@yukawa yukawa commented Apr 1, 2025

Description

As a preparation to make unit tests run with Bazel on Windows (#1223), this commit sets target_compatible_with to qt_build_test so that the test can be excluded when the target is Android, macOS and Windows.

Issue IDs

Steps to test new behaviors (if any)

  • OS: Windows 11 24H2
  • Steps:
    1. bazelisk build //renderer/qt/... --config oss_windows --build_tests_only

As a preparation to make unit tests run with Bazel on Windows (google#1223),
this commit sets 'target_compatible_with' to 'qt_build_test' so that the
test can be excluded when the target is Android, macOS and Windows.
@@ -145,6 +145,7 @@ mozc_cc_qt_library(

build_test(
name = "qt_build_test",
target_compatible_with = ["@platforms//os:linux"],
Copy link
Collaborator

Choose a reason for hiding this comment

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

os:macos is also compatible.

It's fine to exclude Windows, but I'm wondering what error happens against this build for Windows.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here is the result. ::setenv isn't available hence failing. The build succeeds if we drop the line in question though.

PS D:\mozc\src> bazelisk --output_base=D:/x test //renderer/qt:qt_build_test --config oss_windows -c dbg --enable_runfiles
INFO: Analyzed target //renderer/qt:qt_build_test (54 packages loaded, 6962 targets configured).
ERROR: D:/mozc/src/renderer/qt/BUILD.bazel:82:19: Compiling renderer/qt/qt_server.cc failed: (Exit 1): clang-cl.exe failed: error executing CppCompile command (from target //renderer/qt:qt_server) D:\mozc\src\third_party\llvm\clang+llvm-20.1.1-x86_64-pc-windows-msvc\bin\clang-cl.exe @bazel-out/x64_windows-dbg/bin/renderer/qt/_objs/qt_server/qt_server.obj.params
renderer/qt/qt_server.cc(109,5): error: no member named 'setenv' in the global namespace
  109 |   ::setenv("QT_QPA_PLATFORM", "xcb", 1);
      |   ~~^
1 error generated.
Target //renderer/qt:qt_build_test failed to build
Use --verbose_failures to see the command lines of failed build steps.
INFO: Elapsed time: 8.440s, Critical Path: 4.30s
INFO: 118 processes: 828 action cache hit, 99 internal, 19 local.
ERROR: Build did NOT complete successfully
//renderer/qt:qt_build_test                                     FAILED TO BUILD

Executed 0 out of 1 test: 1 fails to build.

os:macos is also compatible.

Do you really want to keep maintaining this build for macOS even though it's never used for production? IIUC, we can stop running build_qt.py before running bazelisk test on macOS by excluding this from test targets.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here is the result. ::setenv isn't available hence failing. The build succeeds if we drop the line in question though.

Another negative side-effect of trying to support this scenario for Windows is that we also need to build Qt for debug builds, which increases the build cycle time, unless we switch to release build when running unit tests only on Windows.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we can stop running build_qt.py for bazelisk test, it worth excluding this test from macOS.

Also thank you for sharing the build log on Windows. If removing that line works for Windows, it'd be nice to fix it in another commit.

You don't need to modify this PR, I will merge your PR as-is (i.e. support os:linux only)

@hiroyuki-komatsu hiroyuki-komatsu merged commit a45df59 into google:master Apr 10, 2025
1 check passed
@hiroyuki-komatsu
Copy link
Collaborator

We have merged your PR.
Thank you for the contribution!

@yukawa yukawa deleted the issue_1223_qt_build_test branch April 10, 2025 08:59
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