Repository navigation
Conversation
github-actions
Bot
added
the
type-infra
PR HealthBreaking changes ✔️
This check can be disabled by tagging the PR with API leaks ✔️The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.
This check can be disabled by tagging the PR with Changelog Entry ✔️
Changes to files need to be accounted for in their respective changelogs. This check can be disabled by tagging the PR with |
dcharkes
left a comment
dcharkes
left a comment
There was a problem hiding this comment.
I really like the one PrebuiltLibrary object that you use from hook/build.dart hook/link.dart and tool/prebuilt.dart!
However, I'm not yet sure that we have considered enough the alternative ways of doing things to be shipping this package as Dart team endorsed, signaling it's the best way.
- Which decisions this package makes are opinionated and could be made differently?
- Is the API is flexible enough to accommodate a bunch of alternative ways to do prebuilt libraries.
One thing that is very baked in to this design is this idea that the hook downloads from a third-party CDN. I think that's not the right way.
- For small dylibs, for now the preferred way of doing things is to bundle them in the package tar (@jonasfj @sigurdm)
- This avoids having to compute hashes for downloads, pub takes care of this.
- This avoids downloading from a third-party CDN which can be down or of which we can violate fair-use-policies by pulling things very often on CI runs.
- For larger dylibs, we ideally want a larger package limit in the short term. And a way to lazily download individual artifacts with provenance checked via a pub API and the artifacts stored in the pub cache.
- This would limit bandwidth use.
- And enable sharing of binaries across projects on disk.
How to use the current package for uploading your dylibs inside the package tar?
If we add support to pub for downloading invidual binary blobs and putting them in the pub cache, how does that change the API?
- The flow for how to publish your package might change. E.g. How do you declare the individual binary blobs for
dart pub publish. - The download flow might change. E.g. We will probably have some pub-team owned package that has an async API that returns you a file path on disk in the pub cache for your downloaded artifact after you asked for it. Then pub will do the hashes checking so you can simply rely on that.
I don't feel comfortable shipping this package as dart interop team endorsed until we explore the design space of precompiled dylibs more.
| return symbols; | ||
| } | ||
|
|
||
| /// Returns the subset of [candidateSymbols] that the COFF [archive] defines. |
There was a problem hiding this comment.
Should this kind of logic be in package:native_toolchain_c somewhere?
|
I agree with @dcharkes that we should think through how this fits with With the package attestation work landing in
Concretely for this PR: could we decouple |
|
Another high level comment after discussion with @goderbauer and @liamappelbe, this package might not fit on this repo:
Maybe this package would fit better in the dart-lang/ecosystem repo. We on dart-lang/native we are actively trying to have less packages, and less maintenance burden. (We'd love for (My previous comments still apply, whether we host this in dart-lang/native or somewhere else.) |
524d272 to
1947841
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## ntc-windows-export-defined-symbols #3719 +/- ##
======================================================================
- Coverage 87.96% 87.85% -0.11%
======================================================================
Files 311 321 +10
Lines 19849 20353 +504
======================================================================
+ Hits 17461 17882 +421
- Misses 2388 2471 +83
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…ls on Windows With the native_toolchain_c fix below in the stack, LinkerOptions.treeshake no longer passes an /INCLUDE flag per symbol, so the command-line length fallback to a hand-written .def file is no longer needed.
native_toolchain_c now only exports the symbols that the input archives define on Windows. Without recorded uses, export allKnownSymbols on Windows, or bundle the prebuilt dynamic library in fetch mode if that is null too.
1947841 to
d77a88d
Compare
Without `usedSymbols`, the library is never tree-shaken: `build` bundles the dynamic library directly instead of routing a static library to the link hook. Without recorded uses (record use disabled), nothing can be tree-shaken either. Then `link` bundles the prebuilt dynamic library in the `fetch` build mode. In other build modes, it links the static library keeping all functions, except on Windows, where a DLL only exports the functions it lists, so it throws a `BuildError` suggesting to enable record use or to set `treeshake: off`. `treeshake: on` fails the build in both cases, because tree-shaking is not possible.
CI runs the tests from the workspace root, where `Directory.current` is not the package root, so the warm-up `pub get` failed to resolve the `prebuilt_code_assets` path dependency. Use `findPackageRoot` from `package:native_test_helpers` like the other packages in this repository.
…unctions The docs and messages said "no recorded uses" for two different cases: - Record use is disabled (`input.recordedUses` is `null`): it is unknown which functions the application uses. They now say "record use is disabled". - The application uses none of the functions: `link` now explicitly bundles no library and returns before setting up the linker, instead of relying on `CLinker` skipping an empty `symbolsToKeep`. This also holds with `treeshake: on`. Adds unit tests for the second case on Linux and Windows in the `auto` and `on` modes, and an integration test that a `dart build cli` app that calls none of the functions runs without the library.
| user_defines: | ||
| my_package: | ||
| # 'fetch' (default), 'build' (alias 'checkout'), or 'local' | ||
| buildMode: fetch |
There was a problem hiding this comment.
| buildMode: fetch | |
| build_mode: fetch |
What style do we want here?
4 participants
Adds
package:prebuilt_code_assets(currently at mosuem/prebuilt_code_assets) to this repository aspkgs/prebuilt_code_assets.The package covers the boilerplate that packages shipping prebuilt native libraries (like
package:icu4xandpackage:boring) currently each write by hand in their hooks:hook/build.dart: fetches the prebuilt dynamic or static library for the target from a release (e.g. GitHub Releases) or a pub-bundledprebuilt/directory, verifies its SHA-256, and caches it. Alternatively, builds from source through a toolchain-agnostic callback (CBuilder, CMake, Cargo, ...), or bundles a local binary. Selected viahooks.user_defines.<package>.buildMode(fetch/build/local).hook/link.dart: tree-shakes the static library withCLinkerdown to the functions inrecordedUses, and falls back to the prebuilt dynamic library if linking fails (e.g. no C toolchain) intreeshake: automode.runPrecompileBinariesCliandrunRegenerateHashesClifor producing release binaries and the hash manifest.Changes in this PR:
pkgs/prebuilt_code_assets, at 0.2.0-wip (see its CHANGELOG for the breaking changes since 0.1.2 on pub.dev), withresolution: workspace.native.yamlCI path filters, the labeler, the PR title prefixes, and the README.The history of the package is not preserved (single commit). It's relicensed to BSD-3-Clause with Dart project authors headers in the source repository before the import.
Stacked on #3726, with which
native_toolchain_conly exports symbols that the input archives define on Windows. Together with #3718 (merged), this lets this package drop its own COFF archive parsing and Windows command-line length workaround. Requiresnative_toolchain_c0.19.6.PR Checklist
dart tool/ci.dart --alllocally and resolved all issues identified. (Ran the--license --format --dependency-validator --workspacetasks,dart analyze --fatal-infos, and the package tests.)CHANGELOG.mdfor the relevant packages.