Conversation
|
Reviews in this chain: |
|
There was a problem hiding this comment.
Code Review
This pull request replaces the patching of rules_boost's lzma build file with a custom build file for org_lzma_lzma. This is a solid approach to improve build reliability and cacheability, especially on Windows. The changes are well-explained in the pull request description and in the code comments. I have one suggestion for the new BUILD file to improve its maintainability by avoiding a hardcoded repository name.
a6fec9c to
d2029f7
Compare
d63555f to
636fcd7
Compare
d2029f7 to
f63c28f
Compare
636fcd7 to
076937e
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
f63c28f to
c8e8439
Compare
076937e to
df80fe6
Compare
4b62149 to
be1c72d
Compare
df80fe6 to
23649dd
Compare
|
|
||
| cc_library( | ||
| name = "lzma", | ||
| srcs = [ |
There was a problem hiding this comment.
should these be using glob?
There was a problem hiding this comment.
Glob'd what I could. Some cannot be without pulling in files we do not want
There was a problem hiding this comment.
hmm.. which files are ones we do not want?
There was a problem hiding this comment.
Mix of "causes linker errors", others are just hygeine.
-
src/xz/, src/xzdec/, src/lzmainfo/** + the *_tablegen.c files
These all define main(). Including them in a cc_library produces a link error (multiple definition of
main) for any binary that depends on :lzma. -
crc32_small.c and crc64_small.c
Both define the same symbols as crc32_fast.c and crc64_fast.c (lzma_crc32, lzma_crc64). Including both is
a link error — duplicate symbol at link time. -
The five tuklib_*.c CLI utility files (tuklib_exit.c, tuklib_mbstr_fw.c, etc.)
These don't have main() and don't conflict with anything in the library. Worst case: dead code in the
archive, and they may pull in CLI-only headers that don't compile cleanly on all platforms. Probably
harmless in practice, but formally wrong.
be1c72d to
5c8b546
Compare
| "src/liblzma/rangecoder/price_table.c", | ||
| ] + glob(["src/**/*.h"]), | ||
| hdrs = [ | ||
| "src/liblzma/api/config.h", |
There was a problem hiding this comment.
these also seems glob-able?
| "src/liblzma/lzma/lzma_encoder_optimum_normal.c", | ||
| "src/liblzma/lzma/lzma_encoder_presets.c", | ||
| "src/liblzma/rangecoder/price_table.c", | ||
| ] + glob(["src/**/*.h"]), |
There was a problem hiding this comment.
maybe exclude api headers? those api header files are double included now?
|
|
||
| cc_library( | ||
| name = "lzma", | ||
| srcs = [ |
There was a problem hiding this comment.
hmm.. which files are ones we do not want?
52fb330 to
83fffee
Compare
…build file Instead of patching rules_boost's BUILD.lzma at dependency-resolution time, declare org_lzma_lzma before com_github_nelhage_rules_boost so that boost_deps()'s maybe() skips it. A local build_file label (//thirdparty/patches:org_lzma_lzma.BUILD.bazel) forces content-based cache invalidation: changing the file triggers re-setup on all machines, including Windows CI with a persistent output base where patching an external repo's BUILD file in-place would not propagate to already-extracted directories. The custom build file also fixes Windows MSVC D8004 by using -Ipath copts (no space between -I and the path) and replaces the includes attribute with strip_include_prefix for more reliable include path handling. Topic: lzma-custom-build-file Relative: java-all-tests-bin-deploy-jar Signed-off-by: andrew <andrew@anyscale.com>
83fffee to
f3190a0
Compare
…build file (#61669) Instead of patching rules_boost's BUILD.lzma at dependency-resolution time, declare org_lzma_lzma before com_github_nelhage_rules_boost so that boost_deps()'s maybe() skips it. A local build_file label (//thirdparty/patches:org_lzma_lzma.BUILD.bazel) forces content-based cache invalidation: changing the file triggers re-setup on all machines, including Windows CI with a persistent output base where patching an external repo's BUILD file in-place would not propagate to already-extracted directories. The custom build file also fixes Windows MSVC D8004 by using -Ipath copts (no space between -I and the path) and replaces the includes attribute with strip_include_prefix for more reliable include path handling. Signed-off-by: andrew <andrew@anyscale.com> Signed-off-by: abrar <abrar@anyscale.com>

Instead of patching rules_boost's BUILD.lzma at dependency-resolution time,
declare org_lzma_lzma before com_github_nelhage_rules_boost so that
boost_deps()'s maybe() skips it. A local build_file label
(//thirdparty/patches:org_lzma_lzma.BUILD.bazel) forces content-based cache
invalidation: changing the file triggers re-setup on all machines, including
Windows CI with a persistent output base where patching an external repo's
BUILD file in-place would not propagate to already-extracted directories.
The custom build file also fixes Windows MSVC D8004 by using -Ipath copts
(no space between -I and the path) and replaces the includes attribute with
strip_include_prefix for more reliable include path handling.
Topic: lzma-custom-build-file
Relative: java-all-tests-bin-deploy-jar
Signed-off-by: andrew andrew@anyscale.com