[build] Inline lz4 submodule into src-ThirdParty/#11658
Open
jonathanpeppers wants to merge 1 commit into
Open
Conversation
We compile only two files out of the entire `external/lz4` submodule (`lib/lz4.c` and `lib/lz4.h`), and the upstream `dotnet/lz4` fork is essentially frozen. Carrying a full git submodule (with tests, build files, docs, etc.) for two source files is overkill. Vendor `lz4.c` and `lz4.h` verbatim into `src-ThirdParty/lz4/`, matching the existing convention used by `src-ThirdParty/bionic/` and `src-ThirdParty/crc32.net/`. Repoint: * `src/native/common/lz4/CMakeLists.txt` * `tools/fastdev/xamarin.sync/CMakeLists.txt` * `tools/fastdev/fastdevtools.projitems` * `Configuration.props` (`LZ4SourceDirectory`) * `src/native/native.targets` (`_RuntimeSources` input tracking) And remove the submodule (`.gitmodules` block, dependabot ignore entry, `.lgtm.yml` exclusion, `update-tpn` skill inventory row). The existing `lz4/lz4` TPN block at `THIRD-PARTY-NOTICES.TXT` already covers this attribution and is unchanged. The managed `K4os.Compression.LZ4` NuGet package is unrelated and untouched. Follow-up to dotnet#11529, dotnet#11568, dotnet#11580, dotnet#11608, dotnet#11613, dotnet#11631. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR removes the external/lz4 git submodule and vendors the two consumed LZ4 sources (lz4.c/lz4.h) directly under src-ThirdParty/lz4, updating the native/fastdev build wiring to point at the new location.
Changes:
- Vendor
lz4.c/lz4.hintosrc-ThirdParty/lz4/and update CMake/MSBuild inputs to compile from that directory. - Repoint fastdev (
xamarin.sync) and runtime build tracking (native.targets) to the new LZ4 path. - Remove submodule/automation references (submodule entry, dependabot ignore, LGTM exclusion, update-tpn inventory).
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tools/fastdev/xamarin.sync/CMakeLists.txt | Updates LZ4_SRC_DIR default path to src-ThirdParty/lz4 (but currently uses invalid CMake syntax; see comment). |
| tools/fastdev/fastdevtools.projitems | Updates CMake flags to pass -DLZ4_SRC_DIR=../../../src-ThirdParty/lz4. |
| src/native/native.targets | Updates _RuntimeSources input tracking to match lz4.c/lz4.h at the new root. |
| src/native/common/lz4/CMakeLists.txt | Updates the runtime’s LZ4 CMake build to use ${REPO_ROOT_DIR}/src-ThirdParty/lz4. |
| src-ThirdParty/lz4/lz4.h | Adds vendored LZ4 header. |
| src-ThirdParty/lz4/lz4.c | Adds vendored LZ4 implementation. |
| Configuration.props | Updates LZ4SourceDirectory default to src-ThirdParty\\lz4. |
| .lgtm.yml | Removes exclusion for external/lz4/tests (no longer present). |
| .gitmodules | Removes external/lz4 submodule entry. |
| .github/skills/update-tpn/SKILL.md | Updates third-party inventory docs for vendored LZ4. |
| .github/dependabot.yml | Removes dependabot ignore entry for external/lz4. |
Comment on lines
46
to
48
| if(NOT DEFINED ${LZ4_SRC_DIR}) | ||
| set(LZ4_SRC_DIR, "../../../external/lz4/lib") | ||
| set(LZ4_SRC_DIR, "../../../src-ThirdParty/lz4") | ||
| endif() |
Member
Author
|
I'm on the fence about doing this, we can discuss. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
We compile only two files out of the entire
external/lz4submodule (lib/lz4.candlib/lz4.h), and the upstreamdotnet/lz4fork is essentially frozen. Carrying a full git submodule (with tests, build files, docs, etc.) for two source files is overkill.Vendor
lz4.candlz4.hverbatim intosrc-ThirdParty/lz4/, matching the existing convention used bysrc-ThirdParty/bionic/andsrc-ThirdParty/crc32.net/. Repoint:src/native/common/lz4/CMakeLists.txttools/fastdev/xamarin.sync/CMakeLists.txttools/fastdev/fastdevtools.projitemsConfiguration.props(LZ4SourceDirectory)src/native/native.targets(_RuntimeSourcesinput tracking)And remove the submodule (
.gitmodulesblock, dependabot ignore entry,.lgtm.ymlexclusion,update-tpnskill inventory row).The existing
lz4/lz4TPN block atTHIRD-PARTY-NOTICES.TXTalready covers this attribution and is unchanged. The managedK4os.Compression.LZ4NuGet package is unrelated and untouched.Follow-up to #11529, #11568, #11580, #11608, #11613, #11631.
Verification
Local build of
src/native/native-mono.csprojandsrc/native/native-clr.csproj(Debug) succeeded with 0 errors.compile_commands.jsonconfirmslz4.cis compiled fromsrc-ThirdParty/lz4/with-I.../src-ThirdParty/lz4, andlibxa-lz4-release.ais produced for all 4 ABIs (arm, arm64, x64, x86).