Separate onnxruntime_providers compilation from its archive - #32162
Edward Chen (edgchen1) merged 1 commit into
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR changes how the onnxruntime_providers library is built/consumed by introducing an object-library based path (primarily for MSVC shared builds) and updating downstream link dependencies to use a single indirection target variable.
Changes:
- Introduce
onnxruntime_providers_objobject library and set${onnxruntime_providers_target}to either the object library or a materialized static archive depending on configuration. - Update multiple CMake link lists to use
${onnxruntime_providers_target}instead of the hard-codedonnxruntime_providerstarget. - Adjust unit test utility linking to avoid copying object-library objects into an intermediate static archive.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| cmake/onnxruntime_providers_cpu.cmake | Splits providers into object library + conditional archive with ${onnxruntime_providers_target} indirection. |
| cmake/onnxruntime_unittests.cmake | Switches tests to ${onnxruntime_providers_target} and adds special handling when it’s an object library. |
| cmake/onnxruntime.cmake | Updates internal library aggregation to use ${onnxruntime_providers_target}. |
| cmake/onnxruntime_webassembly.cmake | Uses ${onnxruntime_providers_target} when bundling/linking for WebAssembly. |
| cmake/onnxruntime_training.cmake | Updates training runner deps/linking to ${onnxruntime_providers_target}. |
| cmake/onnxruntime_python.cmake | Updates pybind module linking to ${onnxruntime_providers_target}. |
| cmake/onnxruntime_providers_webgpu.cmake | Updates WebGPU provider linking to ${onnxruntime_providers_target}. |
| cmake/onnxruntime_providers_pch.cmake | Renames PCH target to onnxruntime_providers_obj. |
| cmake/CMakeLists.txt | Includes providers PCH config only if onnxruntime_providers_obj exists. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Suppressed comments (1)
cmake/onnxruntime_providers_cpu.cmake:210
LINKER_LANGUAGEis only meaningful for linkable targets (e.g., STATIC/SHARED libraries, executables). For anOBJECT_LIBRARYit is effectively ignored, so this property assignment is misleading. Prefer removing it here and (if needed) applyingLINKER_LANGUAGE CXXonly to the actual archive target (onnxruntime_providers).
set_target_properties(onnxruntime_providers_obj PROPERTIES LINKER_LANGUAGE CXX)
|
Seems I'm at an impasse with the reviewbots. I'll wait for Edward Chen (@edgchen1)'s review for now 😄 |
a69cc9d to
7b93e65
Compare
7b93e65 to
4102a14
Compare
|
Could you confirm that you manually validated the exact MSVC configuration this change targets: The PR description also appears to describe the earlier two-target design and should be updated to match the current implementation. 🤖 Written by GitHub Copilot on behalf of @edgchen1. |
On MSVC shared-library builds onnxruntime_providers is an object library
rather than a static one. With onnxruntime_ENABLE_LTO the objects carry
whole-program IR, which pushes the archive past the 4 GiB limit lib.exe
enforces on the COFF archive format (LNK1248), and no lib.exe option raises
that limit. A shared-library build only needs the objects for the final
link, so no archive is produced there.
Consumers link ${onnxruntime_providers_target} rather than either target
name, so the configuration that picks the shape lives in
onnxruntime_providers_cpu.cmake alone instead of being a condition each
consumer has to repeat.
The archive is not materialized from $<TARGET_OBJECTS:...> of an
always-present object library, which would have separated compilation from
packaging more cleanly. The Xcode generator hash-suffixes object filenames
when source basenames collide, and eight collide among the provider sources
(activations, attention, element_wise_ops, grid_sample, layer_norm,
rotary_embedding, unique, utils), while TARGET_OBJECTS expands to the
unsuffixed names, so libtool cannot find them and every iOS job fails.
In onnxruntime_unittests.cmake the workaround that keeps provider objects
out of the onnxruntime_unittest_utils archive now keys off the providers
target's actual type instead of re-testing the configuration that chose it.
4102a14 to
3532e1d
Compare
|
Confirmed, and recorded in the description. Firefox builds onnxruntime on Windows with this exact configuration, so I ran it as an A/B — same pinned revision both sides, differing only by this patch, Without the patch, x86_64 and i686 both fail: With it, no archive is produced and both link. That build is trimmed ( |
…8 under MSVC 14.51. r=firefox-build-system-reviewers,ahochheiden With onnxruntime_ENABLE_LTO the provider objects carry whole-program IR, which pushes onnxruntime_providers.lib past the 4 GiB limit lib.exe enforces on the COFF archive format. MSVC 14.51 tips it a few MB over on both x86_64 and i686, so the Windows toolchain build fails to link. The fix builds the providers as an object library rather than an archive on MSVC shared-library builds, which only need the objects for the final link. Cherry-picked from microsoft/onnxruntime#32162, not yet merged. The hunks for onnxruntime_unittests.cmake, onnxruntime_providers_webgpu.cmake and onnxruntime_providers_pch.cmake are omitted: they do not apply to the pinned revision and cover code this build does not configure. Differential Revision: https://phabricator.services.mozilla.com/D320657
|
closing and reopening to restart CI builds |
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
Supersedes #30511, which GitHub closed automatically when I force-pushed a commit that had lost its parent, and will not let me reopen. The review discussion there is still readable; both of Edward Chen (@edgchen1)'s comments on it are addressed here.
The change
On MSVC shared-library builds the provider sources go into an object library,
onnxruntime_providers_obj; everywhere else they go into the static libraryonnxruntime_providersas before. Only one of the two exists per configuration, and consumers link${onnxruntime_providers_target}, so the condition lives inonnxruntime_providers_cpu.cmakealone.onnxruntime_unittests.cmakekeys off the target's actualTYPErather than re-testing it.The archive is skipped there because with
onnxruntime_ENABLE_LTOthe objects carry whole-program IR, pushing it past the 4 GiB limitlib.exeenforces on the COFF archive format (LNK1248). Nolib.exeoption raises that limit, and a shared-library build only needs the objects for the final link.Why compilation and packaging are not separate targets
An earlier revision always created the object library and materialized the archive from
$<TARGET_OBJECTS:onnxruntime_providers_obj>. That does not survive the Xcode generator, and failed every iOS job:Xcode flattens a target's objects into one directory and CMake hash-suffixes the filenames when source basenames collide, but
TARGET_OBJECTSexpands to the unsuffixed names. Sources with unique basenames in the same target were written unsuffixed, which is the control. Ninja is unaffected.Two further constraints:
PRIVATElink is recorded as$<LINK_ONLY:>, andinstall(EXPORT)then rejectsonnxruntime_providersfor requiring a target outside the export set.onnxruntime_INTERNAL_LIBRARIESis consumed both as a link list and as a list of archive files — the Apple static-framework prelink inonnxruntime.cmake, andbundle_static_library()inonnxruntime_webassembly.cmake, which collects onlySTATIC_LIBRARYdependencies and would otherwise drop the providers silently. That is also why the condition stays keyed on MSVC.Verification
Firefox builds onnxruntime on Windows with
cl.exefrom MSVC 14.51.36231 and this exact configuration, so it serves as an A/B — same pinned revision both sides, differing only by this patch:Without the patch, x86_64 and i686 both fail, with
onnxruntime_ENABLE_LTO=ONandonnxruntime_BUILD_SHARED_LIB=ONin the generated cache:That is 4,296,923,348 bytes, roughly 1.9 MB over; the 32-bit build reports 4,300,722,339. The margin is small, consistent with where this surfaced — the same build linked fine until the toolchain moved to 14.51.
With the patch,
onnxruntime_providers.libis never created, the objects build underonnxruntime_providers_obj, and both DLLs link.That configuration is trimmed (
--disable_rtti,--disable_exceptions, lite proto,MinSizeRel), so a stock shared+LTO build overflows by at least as much. It also setsonnxruntime_BUILD_UNIT_TESTS=OFFwith no training or webgpu, so those paths rely on CI here. On Linux the shared, static and forced-object-library configurations all configure and build, with no provider objects in any intermediate archive.