libacl import options to address -fPIC and other similar issue - #515
Nikhil2206 wants to merge 2 commits into
Conversation
f09b1c6 to
29046b4
Compare
|
Documentation preview for this pull request is available at: |
| handle their native build systems/autoconf quirks); becomes something this | ||
| repo now owns and must patch/update over time; still need libc/system | ||
| headers (e.g. `sys/capability.h`) to match target consistently. | ||
|
|
There was a problem hiding this comment.
Regarding the libcap. There is https://registry.bazel.build/modules/libcap/2.27.bcr.2/overview. Can we use it instead of pulling a deb? I'm not entirely sure it's the same thing or it's a different implementation
Maybe we can also add the acl lib to BCR?
There was a problem hiding this comment.
I looked again. The BCR has the same libcap we're using, but a very old version. We can raise a PR in BCR to add a newer version
There was a problem hiding this comment.
we could think of BCR approach, but this might look overkill for baselib purpose because
- we likely need to maintain that per lib,
- could have delayed merges, maintainer may or may not like this
- harder to audit for a safety case, and re-verification is needed on every module version bump, on someone else's release cadence.
i think keeping build and config controll to baselib look relatively safer
| this repo; slower to iterate; still needs a strategy per distro if | ||
| AutoSD/Elektrobit need different library builds (e.g. musl vs glibc, | ||
| different ABI). | ||
|
|
There was a problem hiding this comment.
Another con here:
- users of baselibs may use their own toolchain that could be lacking the appropriate sysroot configuration
There was a problem hiding this comment.
@nradakovic
could you please share your openion on this topic,
specifically option 4 using build from source approach, we are likely to use, I am currently verifying it in reference integration
aaf57cd to
b5a4612
Compare
b5a4612 to
492a72d
Compare
492a72d to
8ef98c5
Compare
|
|
||
| # Bazel-native equivalent of upstream acl's `./configure --enable-nls`: gates | ||
| # whether include/misc.h's `_()` macro (see @acl-src//:acl) routes through gettext(). | ||
| bool_flag( |
There was a problem hiding this comment.
Why is this flag needed? Do we really want to give a user a way to control it?
| build --per_file_copt=external/.*google_benchmark.*@-Wno-error | ||
| # acl is vendored third-party source (see third_party/acl/acl.BUILD); it does not | ||
| # meet this repo's strict warning bar (e.g. -Wcast-qual), so demote its warnings. | ||
| build --per_file_copt=external/.*acl-src.*@-Wno-error |
There was a problem hiding this comment.
We should find another way to solve this as acl is non-dev-dependency. @nradakovic, any options here other that creating a yet another transition?
| "//conditions:default": "@acl-deb//:acl", | ||
| }), | ||
| actual = "@acl-src//:acl", | ||
| visibility = ["@score_baselibs//score/os:__pkg__"], |
There was a problem hiding this comment.
These two aliases looks wrong.
The score/os is the only user of libacl and here it links statically to it. What is purpose of acl_shared then?
My expectation nobody is linking to the static lib other then for purposes of constructing a shared library. I.e. score/os users should link to the shared lib only
| name = "acl-deb-aarch64", | ||
| # acl is vendored from upstream source instead of a prebuilt, arch-pinned Ubuntu .deb | ||
| # (see third_party/acl/acl.BUILD and score/os/BUILD's `:acl` target) to avoid the | ||
| # recurring -fPIC/arch breakage tracked by SWP-278650 (eclipse-score/baselibs#172, #496, #78). |
There was a problem hiding this comment.
Avoid mentioning any internal tickets in code.
There was a problem hiding this comment.
I was trying it but, it got missed somehow
|
|
||
| third_party/acl vendors the plain POSIX ACL portion of libacl's source (excluding | ||
| acl_delete_def_file_at and perm_copy_fd/perm_copy_file) from acl 2.4.0 [2], | ||
| compiled directly as a score_baselibs cc_library instead of installing a prebuilt, |
There was a problem hiding this comment.
Mentioning ubuntu deb packages looks wrong here
|
@4og sure I will clean up the nitpicking things mentioned altogether once we reached decision |
8ef98c5 to
547cc81
Compare
This is Just draft PR to compare and finalized the approach to import libacl in baselib repo
Replace prebuilt acl
.debwith a hermetic, source-builtlibaclProblem:
score_baselibsconsumedaclas an architecture-pinned Ubuntu.deb(libacl1-dev). This repeatedly broke on non-Ubuntu targets and onaarch64(missing-fPIC, arch/toolchain mismatches) — tracked as SWP-278650 (#172, #496, #78).Fix: Vendor upstream
acl-2.4.0source and compile it with this repo's own toolchain instead, so PIC/ABI can never drift from the rest of the build, on any target this repo supports (Ubuntu/AutoSD/Elektrobit, x86_64/aarch64) — no per-distro.debneeded.What's included
MODULE.bazel: replaced the twodeb()rules (acl-deb,acl-deb-aarch64) with a singlearchive()rule (acl-src) pulling the sha256-pinnedacl-2.4.0tarball.acl.BUILD/acl_sources.bzl:cc_library(:acl)compiling only the plain POSIX ACL subset actually needed byacl_impl.cpp(excludes the GPL-2 CLI tools and the unused_at/perm_copy_*files).config.h: hand-written stand-in for acl's autoconf-generatedconfig.h, scoped to this repo's one supported target class (glibc, 64-bit, little-endian Linux).ENABLE_NLSis the one genuine configure-time option (gatesgettext()ininclude/misc.h), so it's exposed as a real Bazel flag (bool_flag+config_setting,--@score_baselibs//third_party/acl:enable_nls) instead of being hardcoded.check_config_drift.py(config_drift_test): fails the build if (a) a vendored source references an autoconf-style macro (HAVE_*,ENABLE_*, ...) not accounted for inconfig.h, or (b) a vendored file's header claims the GPL rather than the LGPL — both catch drift automatically on futureaclversion bumps.cc_shared_library(:acl_shared): buildslibaclas a real shared object, giving consumers a straightforward LGPL-2.1 §6(b) compliance path (swap the.sowithout relinking). Static linking (:acl) is now visibility-restricted to the one wrapper that needs it at compile time (score/os:acl); every other consumer must go through:acl_shared+dynamic_deps.score/os/test:acl_testandscore/os/utils/acl:unit_testwere switched to dynamic linking as the first (and currently only) consumers.NOTICE/acl-lgpl.txt: added LGPL-2.1 attribution and license text for the vendored code..bazelrc: demoted-Werrorforexternal/.*acl-src.*, since the vendored upstream source doesn't meet this repo's strict warning bar (e.g.-Wcast-qual).libacl_import_options.md: design doc comparing this approach (build-from-source) against a hermetic sysroot alternative, grounded in the actual implementation.Validation
Built and tested on both
bl-x86_64-linuxandbl-aarch64-linux(aarch64 via qemu); verified vialdd/runfiles inspection that the dynamic-linked test targets pull inlibacl_shared.soat runtime with no static duplication; confirmed the visibility restriction blocks unauthorized static consumers andconfig_drift_test/GPL-check both pass.