Uh oh!
There was an error while loading. Please reload this page.
[build] Inline lz4 submodule into src-ThirdParty/ - #11658
Closed
jonathanpeppers wants to merge 1 commit into
Closed
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() |
jonathanpeppers
commented
Jun 15, 2026
MemberAuthor
I'm on the fence about doing this, we can discuss. |
jonathanpeppers
commented
Jun 16, 2026
MemberAuthor
Closing in favor of a simpler approach — switching the submodule to track upstream lz4/lz4 directly instead of vendoring. The dotnet/lz4 fork has zero custom changes (verified: 0 files differ from upstream at the merge parent), so there's no reason to keep a fork OR to vendor when we can just track upstream releases. |
jonathanpeppers added a commit
that referenced
this pull request
Jun 25, 2026
Mirrors #11608 (which checked in `Mono.Android.Apis.projitems` the same way). See also #11568, #11580, #11613, #11631, #11657, #11658. ## What `build-tools/scripts/Ndk.projitems.in` was a template with `@NDK_*@` placeholders that the `Get_Ndk_projitems` generator in xaprepare filled in from constants in `BuildAndroidPlatforms.cs`. Those substitution values only change on rare NDK bumps, so generating the file on every build adds no value. This PR checks in the resolved file as a static `build-tools/scripts/Ndk.projitems` and removes the generator. ## Changes - **Add** `build-tools/scripts/Ndk.projitems` — the static, checked-in file. Rather than hardcoding values, it sources them from the existing properties in `Configuration.props` (which is imported before `Ndk.targets`): - `AndroidNdkVersion` → `$(_XAAndroidNdkRelease)` - `AndroidNdkPkgRevision` → `$(_XAAndroidNdkPkgRevision)` - the per-ABI `AndroidNdkApiLevel_*` → `$(AndroidMinimumDotNetApiLevel)` So there is nothing to keep in sync on an NDK/API bump. - **`build-tools/scripts/Ndk.targets`** — point `ProjitemsFile` at the new in-tree location (`$(MSBuildThisFileDirectory)Ndk.projitems`) instead of `bin/Build$(Configuration)/Ndk.projitems`. - **Delete** `build-tools/scripts/Ndk.projitems.in`. - **`Step_GenerateFiles.cs`** — remove the `Get_Ndk_projitems` method and its dispatch entry. - **`Configurables.cs`** — update a doc comment reference from `Ndk.projitems.in` → `Ndk.projitems`. ### Simplifications (from review) - **Collapsed the dead legacy/NET API-level split.** The legacy 32-bit minimum API level used to be lower than the .NET minimum, but they are identical now. Removed the two per-ABI properties that nothing references (`AndroidNdkApiLevel_Arm`, `AndroidNdkApiLevel_X86_Legacy`). The `arm64-v8a`/`x86_64` properties are kept because shipped targets (`Build.Tasks.targets`, `Common.props.in`, `NativeAOT.targets`) still consume them by name. - **Removed the `ApiLevelNET` item metadata.** `ApiLevelNET` was the .NET (.NET 6+) Android minimum API level vs. the legacy Xamarin.Android/Mono `ApiLevel`; the two are identical now and this repo is .NET-only. Its only consumer was `src/native/common/libunwind/libunwind-xamarin.targets`, which now reads `ApiLevel`. Dropped `ApiLevelNET` from all four `AndroidSupportedTargetJitAbi` items. The NDK constants in `BuildAndroidPlatforms.cs` are intentionally left untouched — they're still consumed by `Get_XABuildConfig_cs` and the cmake presets generator. ## Verification - `dotnet build build-tools/xaprepare/xaprepare/xaprepare.csproj -c Debug` → 0 errors / 0 warnings. - Evaluated the import chain via a standalone MSBuild harness (importing the real `Configuration.props`); it produces all four `AndroidSupportedTargetJitAbi` items with `ApiLevel`=24 and the expected `AndroidRID` metadata, matching prior generator output (NDK `28c` / pkg `28.2.13676358`). - Audits clean: `git grep Ndk.projitems.in`, `git grep Ndk_projitems`, `git grep ApiLevelNET`, and the old `bin/Build$(Configuration)/Ndk.projitems` path all return 0 hits. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This was referenced Jun 25, 2026
simonrozsival pushed a commit
that referenced
this pull request
Jun 26, 2026
Many xaprepare provisioning steps have been removed over the past year (#11332, #11348, #11399, #11440, #11441, #11636 and follow-up cleanups in #11568, #11580, #11608, #11613, #11631, #11657, #11658, #11731, #11732, #11733, #11737). The supporting scaffolding around those steps was left behind. This PR removes the verified-dead pieces in two passes. ## Files removed (first pass — original audit) | File | Justification | | --- | --- | | `Application/TestAssembly.cs` | Orphan test infra; only referenced by `TestAssemblyType.cs`. | | `Application/TestAssemblyType.cs` | Only referenced by `TestAssembly.cs`. | | `Application/StepWithDownloadProgress.cs` | No subclasses remain. | | `Application/NDKTool.cs` | NDK provisioning moved to MSBuild in #11440. Last consumer was the also-dead `Configurables.NDKTools` collection (removed below). | | `ToolRunners/SnRunner.cs` | Strong-naming tool runner; never instantiated. | | `ToolRunners/SnRunner.OutputSink.cs` | Partial sibling of `SnRunner`. | | `ToolRunners/CMakeRunner.cs` | Never instantiated. | | `ToolRunners/CMakeRunner.OutputSink.cs` | Partial sibling of `CMakeRunner`. | ## Files removed (second pass — repo-wide re-audit) | File | Justification | | --- | --- | | `ToolRunners/MakeRunner.Linux.cs` | Partial of `MakeRunner`; type never instantiated. | | `ToolRunners/MakeRunner.MacOS.cs` | Partial of `MakeRunner`. | | `ToolRunners/MakeRunner.OutputSink.Unix.cs` | Partial of `MakeRunner`. | | `ToolRunners/MakeRunner.Unix.cs` | Partial of `MakeRunner`. | | `ToolRunners/MSBuildRunner.cs` | Never instantiated. | | `ToolRunners/MSBuildRunner.OutputSink.cs` | Partial sibling of `MSBuildRunner`. | | `ToolRunners/NinjaRunner.cs` | Never instantiated. | | `ToolRunners/NinjaRunner.OutputSink.cs` | Partial sibling of `NinjaRunner`. | | `Application/ScenarioNoStandardEndSteps.cs` | Abstract class with zero subclasses. | ## Cascading cleanup - `ConfigAndData/Configurables.cs` — removed the dead `NDKTools` `List<NDKTool>` collection (lines 132–145). Rest of the file unchanged. ## Removed from initial deletion list after verification - `Application/Extensions.DictionaryOfProgramVersionParser.cs` — initial name-only audit flagged it as dead, but its `Add` extension method is consumed via dictionary collection-initializer syntax in `Application/VersionFetchers.cs`. The consumer never references the static class by name, which is why the first audit missed it. The file stays. - `Scenarios/Scenario_Required.cs` — looks unreferenced by static grep, but `Scenario` subclasses are reflectively discovered via the `[Scenario]` attribute in `Context.cs` (`Utilities.GetTypesWithCustomAttribute<ScenarioAttribute> ()`). Live. The file stays. ## Verification - `git grep -n -w <TypeName>` for each deleted type now returns 0 real hits (only unrelated `"TestAssembly"` string literals in `tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests/` remain — those are assembly-name strings, not the C# type). - `dotnet build build-tools/xaprepare/xaprepare/xaprepare.csproj -c Debug` → 0 warnings, 0 errors. ## Deferred follow-up The csproj conditionally excludes `*MacOS*` files from compilation when `HostOS != Darwin`, so static dead-code analysis from a Windows/Linux host can't see whether the macOS-only consumers are themselves live. These candidates need verification on a Mac host (or a build matrix) before deletion: - `Application/PkgProgram.MacOS.cs` - `Application/HomebrewProgram.MacOS.cs` - `ToolRunners/BrewRunner.MacOS.cs` - `ToolRunners/PkgutilRunner.MacOS.cs` - `ConfigAndData/Dependencies/MacOS.cs`
jonathanpeppers added a commit
that referenced
this pull request
Jun 27, 2026
…ct (#11760) Continues the incremental dismantling of `xaprepare` (see #11568, #11580, #11608, #11613, #11631, #11657, #11658, #11731, #11732, #11733, #11737, #11740). Two more generator methods come out, and the work they did moves into a small dedicated MSBuild project. ## What changes Two xaprepare generators in `build-tools/xaprepare/xaprepare/Steps/Step_GenerateFiles.cs` are deleted: * `Get_Cmake_XA_Build_Configuration` — produced `bin/Build$(Configuration)/xa_build_configuration.cmake`, included by `src/native/CMakeLists.txt`. * `Get_Cmake_Presets` (and its `GetCmakePresetsCommon` helper) — produced `src/native/CMakePresets.json`, consumed by the `cmake` preset invocations in `src/native/native.targets`. Both are now generated by a new project: * **`src/native/cmake-config/cmake-config.csproj`** (`Microsoft.Build.NoTargets`) hosts `_GenerateCMakeFiles`, which uses the existing `ReplaceFileContents` MSBuild task to substitute the placeholders in `build-tools/scripts/xa_build_configuration.cmake.in` and `src/native/CMakePresets.json.in`. * `native-mono.csproj`, `native-clr.csproj`, and `native-nativeaot.csproj` each `<ProjectReference>` this project. ### Why a dedicated project (not just a target in `native.targets`) The three native csproj share two output files (`bin/Build$(Configuration)/xa_build_configuration.cmake` and `src/native/CMakePresets.json`). Hosting the generator in `native.targets` would either: * Race on the shared outputs under parallel msbuild (`/m`), or * Require a `'$(MSBuildProjectName)' == 'native-mono'` gate -- which breaks isolated `dotnet build native-clr.csproj` on a clean tree (the gitignored output files would never get generated). A dedicated project with three incoming `<ProjectReference>`s solves both: single producer, and project-reference ordering guarantees the outputs exist before any consumer's `cmake` preset call -- including for isolated single-csproj builds. ### Incremental correctness `_GenerateCMakeFiles` declares fully-anchored `Inputs`: * `build-tools/scripts/xa_build_configuration.cmake.in` * `src/native/CMakePresets.json.in` * `Configuration.props` (`AndroidNdkDirectory`, `NinjaPath`, `MicrosoftAndroidSdkOutDir`, `XAPackagesDir`, `AndroidMinimumDotNetApiLevel`) * `Directory.Build.props` (`TestOutputDirectory`) * `eng/Versions.props` (`MicrosoftNETCoreAppRefPackageVersion`) so editing any of those re-runs the target; a no-change rebuild skips it. (Note: `$(MSBuildAllProjects)` is intentionally **not** used -- it does not include imported `.props` files for SDK-style projects since MSBuild 16.0; verified empirically.) ### Cascading deletions in xaprepare * `Configurables.NativeSourcesDir` (was used only by `Get_Cmake_Presets`). * All eight `Configurables.Paths.NetcoreAppRuntimeAndroid*` / `CoreClrAppRuntimeAndroid*` properties (+ backing fields + the `GetNetcoreAppRuntimePath` / `GetCoreClrAppRuntimePath` helpers) -- the new MSBuild target reads `$(XAPackagesDir)` / `$(MicrosoftNETCoreAppRefPackageVersion)` directly. * The `Get_Cmake_*` entries in `Step_GenerateFiles.GetFilesToGenerate`. `BuildAndroidPlatforms.NdkMinimumAPI` / `NdkMinimumAPILegacy32` are kept -- still used by `Get_XABuildConfig_cs` (a separate generator for a future PR). ### What stays The `.in` template files are unchanged and still tracked in git -- the new MSBuild target reads them. The other generators in `Step_GenerateFiles.cs` (`Get_SourceLink_Json`, `Get_Configuration_OperatingSystem_props`, `Get_XABuildConfig_cs`, `AddOSSpecificSteps`) are out of scope and untouched. ## Verification On Windows: * `build.cmd Prepare -c Debug` * `dotnet-local.cmd build src\native\native-mono.csproj -c Debug` -> 0 errors, generates both files * `dotnet-local.cmd build src\native\native-clr.csproj -c Debug` -> 0 errors (incl. clean-tree isolated build) * `dotnet-local.cmd build src\native\native-nativeaot.csproj -c Debug` -> 0 errors * `dotnet-local.cmd build build-tools\xaprepare\xaprepare\xaprepare.csproj -c Debug` -> 0 errors Incremental behaviour verified by walking through MSBuild diagnostic output: * No-change rebuild: `Skipping target "_GenerateCMakeFiles" because all output files are up-to-date`. * `touch eng/Versions.props` + rebuild: `Building target "_GenerateCMakeFiles" completely`, outputs re-written. The generated `CMakePresets.json` and `xa_build_configuration.cmake` were diffed against the xaprepare baselines captured before the deletion; only the path separator normalisation (forward slashes throughout) differs, which both CMake and JSON accept on every platform. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
simonrozsival pushed a commit
that referenced
this pull request
Jun 30, 2026
### Context After #11636 (dotnet provisioning step removed) and #11731 (test-deps scenarios removed), `Context.AutoProvision` is `false` by default everywhere except hand-run dev provisioning, which is no longer in use. The per-OS package lists are populated at `OS.Init()` time but `EnsureDependencies` is effectively a no-op: - `OS.EnsureDependencies()` returns early when `AutoProvision` is false (the default), - nothing else in the codebase reads from the `Program` derivatives' install/uninstall paths, - the `BuildToolsInventory` writer remains driven only from `EssentialTools.MacOS.cs` (homebrew version detection). The `OS.Init() / InitializeDependencies() / EnsureDependencies()` machinery on `OS.cs` itself is intentionally **left in place** here — that's a larger refactor for a follow-up PR. This PR only strips the now-vestigial package-list data and the program/runner classes that fed it. ### Files deleted (Phase F — macOS, 4 files) - `Application/HomebrewProgram.MacOS.cs` - `Application/PkgProgram.MacOS.cs` - `ToolRunners/BrewRunner.MacOS.cs` - `ToolRunners/PkgutilRunner.MacOS.cs` ### Files deleted (Phase G — Linux, 5 files) - `Application/Program.Linux.cs` (`LinuxProgram` base — orphan after subclasses go) - `Application/Program.ArchLinux.cs` - `Application/Program.DebianLinux.cs` - `Application/Program.FedoraLinux.cs` - `Application/Program.GentooLinux.cs` ### Files deleted (Phase 3 — orphan) - `Application/IBuildInventoryItem.cs` (only implementor was `HomebrewProgram`; `BuildToolsInventory` itself stays, populated directly by `EssentialTools.MacOS.cs`). ### Files reduced to empty stubs `ConfigAndData/Dependencies/`: - `MacOS.cs` — `InitializeDependencies()` no-op (was Homebrew formula list + git fallback). - `Linux.Arch.cs` — class kept (referenced by `distroMap`); package list removed. - `Linux.Fedora.cs` — same. - `Linux.Gentoo.cs` — same. - `Linux.DebianCommon.cs` — common Debian/Ubuntu package list removed; `Flavor = "Debian"` kept. - `Linux.UbuntuCommon.cs` — `libtoolPackages` + `NeedLibtool` virtual + `InitOS` override removed (all dead). - `Linux.Debian.cs` — all per-version package lists (`packages`, `packagesPre10`, `packagesPreTrixie`, `packagesTrixieAndLater`, `packages10AndNewerBuildBots`) removed; release/codename detection (`EnsureVersionInformation`, `DebianUnstableVersionMap`, `IsDebian10OrNewer`, etc.) preserved as conservative scope. - `Linux.Ubuntu.cs` — `preCosmicPackages`, `cosmicPackages`, `preDiscoPackages` lists + `NeedLibtool` override removed; `UbuntuRelease` + `EnsureVersionInformation` preserved. - `Linux.Mint.cs` — `NeedLibtool` override removed (the property is gone from the base). `ConfigAndData/Dependencies/Windows.cs` was already a no-op stub — no edit. ### Verification Orphan audit (each `git grep -nw <Type> -- 'build-tools/xaprepare/*'` reports **0 hits**): - `HomebrewProgram`, `PkgProgram`, `BrewRunner`, `PkgutilRunner` - `ArchLinuxProgram`, `DebianLinuxProgram`, `FedoraLinuxProgram`, `GentooLinuxProgram`, `LinuxProgram` - `IBuildInventoryItem` Build: ``` dotnet build build-tools\xaprepare\xaprepare\xaprepare.csproj -c Debug Build succeeded. 0 Warning(s) 0 Error(s) ``` ### Out of scope (follow-up) - Removing the abstract `OS.InitializeDependencies()` declaration and the surrounding `EnsureDependencies()` machinery from `OperatingSystems/OS.cs`. - `VersionFetchers` / `ProgramVersionParser` / `RegexProgramVersionParser` / `SevenZipVersionParser` / `Extensions.DictionaryOfProgramVersionParser.cs` are kept — `Utilities.GetProgramVersion` still queries them from `Program.cs`, `ToolRunner.cs`, `EssentialTools.MacOS.cs`, and `OperatingSystems/MacOS.cs` (brew detection). ### Precedent #11568, #11580, #11608, #11613, #11631, #11657, #11658, #11731, #11732, #11733, #11737, #11740, #11760
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 freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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).