bazel: build xcb-util-cursor from source in qt-bazel#10412
Conversation
There was a problem hiding this comment.
Code Review
This pull request modifies the qt-bazel dependency to build xcb-util-cursor from source, eliminating the requirement for a system-provided library. This is achieved by adding a git_override with patch_cmds to download the source and applying a patch to define the corresponding Bazel targets. Feedback was provided regarding the curl command in MODULE.bazel, specifically noting that the --retry-all-errors flag may cause compatibility issues on older systems and recommending the addition of a checksum verification step to ensure the integrity of the downloaded asset.
| # feed a truncated/HTML response into tar (seen in CI as "xz: | ||
| # File format not recognized"). Stage to a file first for a | ||
| # clearer error if extraction fails. | ||
| "curl -sSfL --retry 5 --retry-all-errors --retry-delay 5 -o xcb-util-cursor-0.1.5.tar.xz https://xorg.freedesktop.org/archive/individual/lib/xcb-util-cursor-0.1.5.tar.xz && tar xJf xcb-util-cursor-0.1.5.tar.xz --strip-components=1 -C interface_libs/xcb xcb-util-cursor-0.1.5/cursor && rm xcb-util-cursor-0.1.5.tar.xz", |
There was a problem hiding this comment.
The curl command uses --retry-all-errors, which was introduced in version 7.71.0. This may cause build failures on older systems, such as Ubuntu 20.04 (which ships with curl 7.68.0).
Additionally, downloading external assets without verifying their integrity (e.g., via a SHA256 checksum) is a security risk and can lead to non-deterministic builds if the source is compromised or corrupted. Adding a checksum verification step ensures that the downloaded tarball is exactly what is expected.
| "curl -sSfL --retry 5 --retry-all-errors --retry-delay 5 -o xcb-util-cursor-0.1.5.tar.xz https://xorg.freedesktop.org/archive/individual/lib/xcb-util-cursor-0.1.5.tar.xz && tar xJf xcb-util-cursor-0.1.5.tar.xz --strip-components=1 -C interface_libs/xcb xcb-util-cursor-0.1.5/cursor && rm xcb-util-cursor-0.1.5.tar.xz", | |
| "curl -sSfL --retry 5 --retry-delay 5 -o xcb-util-cursor-0.1.5.tar.xz https://xorg.freedesktop.org/archive/individual/lib/xcb-util-cursor-0.1.5.tar.xz && echo '28a89960303756adcf596d9e47907f744e3d13d26a613334d9ee452273081e23 xcb-util-cursor-0.1.5.tar.xz' | shasum -a 256 -c && tar xJf xcb-util-cursor-0.1.5.tar.xz --strip-components=1 -C interface_libs/xcb xcb-util-cursor-0.1.5/cursor && rm xcb-util-cursor-0.1.5.tar.xz", |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
Can you send this to the upstream repo instead of patching it. Also I think someone added xcb to bcr so maybe that would work |
Repin qt-bazel at oharboe/qt_bazel_prebuilts#xcb-cursor-from-source HEAD (878efffc), which builds xcb-util-cursor 0.1.5 from source as a static cc_library instead of letting interface_libs/xcb dlopen libxcb-cursor.so.0 from the host. That removes the apt-install libxcb-cursor0 step gadfort hit on The-OpenROAD-Project#8532, unblocking the GUI-by-default switch in the follow-up PR. The upstream PR is The-OpenROAD-Project/qt_bazel_prebuilts#11; once that merges, this commit will be repinned to the merged SHA on the The-OpenROAD-Project remote and the oharboe fork URL will be replaced. Replaces the earlier downstream-patch approach (curl in patch_cmds, no sha256 verification) with @QuantamHD's suggestion of fixing it upstream. Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
5aa6e5e to
14f25c0
Compare
|
Good call, @QuantamHD — sent the fix upstream as The-OpenROAD-Project/qt_bazel_prebuilts#11. This PR is now just a For @gemini-code-assist's earlier sha256 / dependency-on-curl notes: the upstream PR uses an For Verified end-to-end locally with
|
|
Thank you for the update, @oharboe. This approach—submitting the fix upstream and using a temporary |
|
|
once The-OpenROAD-Project/qt_bazel_prebuilts#11 is filed, bumping qt-bazel will fix this problem. |
|
clang-tidy review says "All clean, LGTM! 👍" |
Summary
Removes the host
libxcb-cursor0runtime dependency from--//:platform=guiand:openroad-qtbuilds by patchingqt-bazel(viaMODULE.bazel) to compilexcb-util-cursor 0.1.5from source as a
cc_library, replacing the upstreamcc_import(system_provided = True).Why
On #8532, @gadfort reported:
@maliberty then asked @QuantamHD "is it possible to avoid the
need for the user to install this package separately?" — that
question never got answered and the PR has been stalled since.
This PR is the "yes" answer. After it lands, a user can build the
Qt-linked
openroadon a clean machine without apt-installinglibxcb-cursor0first. It is intentionally a standalone change soit can be reviewed and merged independently of the GUI-by-default
flip (which will follow as a stacked PR).
How
The fix is ported verbatim from the public sibling repo
The-OpenROAD-Project/bazel-orfs,where it has been in use for some time. Two pieces:
MODULE.bazel— extend the existingqt-bazelgit_overridewith
patch_cmds(downloadsxcb-util-cursor-0.1.5.tar.xzfrom
xorg.freedesktop.orgwith retry/--failso transientnetwork hiccups produce a clear error instead of a truncated
tar) and
patches.bazel/qt-bazel-xcb-cursor-from-source.patch— new file,copied verbatim from
bazel-orfs/patches/. Replaces qt-bazel'scc_import(name = "xcb_cursor_ifso", system_provided = True)with a
cc_library(name = "xcb_cursor", ...)compiled fromcursor.c / load_cursor.c / parse_cursor_file.c / shape_to_id.c, and rewiresinterface_libs/xcb:xcbtodepend on it instead of the ifso.
Sits alongside the existing
bazel/boost_context_disable_parse_headers.patchfor consistency.Test plan
bazelisk build --//:platform=cli //:openroadstill works(no behavior change for CLI users).
bazelisk build --//:platform=gui //:openroadandbazelisk build //:openroad-qtsucceed on a host that doesnot have
libxcb-cursor0installed.readelf -d bazel-bin/openroad | grep -i cursorreturnsnothing —
libxcb-cursor.so.0is no longer inDT_NEEDED.--//:platform=gui //:openroad) stays green.Follow-up
A second PR will flip
BUILD.bazel'sstring_flag(name = "platform", build_setting_default = "gui")to make the GUIbuild the default — picking up where #8532 left off. That PR is
stacked on this one because the default flip is only safe once
the cursor dependency is bundled.