native icon indicating copy to clipboard operation
native copied to clipboard

[native_assets_builder] How to deal with `NativeCodeAsset`s with conflicting install names and conflicting file names

Open dcharkes opened this issue 1 year ago • 1 comments
trafficstars

It seems that renaming dylibs is non-trivial on some OSes. So, we might want to make it an error to create dylibs with the same names. Either within one hook or as a combination of multiple hooks.

  • [x] Checks in native_assets_builder
  • [x] Conflicts between build and link output in dartdev.
  • [ ] Conflicts between build and link output in Flutter.

TL;DR: So far we've been treating native code assets as being in one namespace, similar to how all native build systems work. We could consider name spacing native code assets per package (per hook), but that requires tracking dynamic linking between dylibs explicitly in the protocol.

So far, we've avoided dealing with name conflicts between native libraries. The rationale being that most native build systems do not have a way to deal with that.

In https://github.com/flutter/flutter/pull/153054, we are rewriting install names and dynamic linker import paths. We could apply similar logic to rewrite library names on all OSes (not just MacOS/iOS), to prevent name clashes between NativeCodeAssets from various packages.

Some observations:

  • File names from within one hook cannot clash. So file name collisions should only come from native libraries reported from different hooks.
  • Library install names (and correspondingly the #include statements when referring to these libraries) need to align with the file paths. So library install name collisions should also only come from combining the results of multiple hooks.
  • It's perfectly fine for a dylib in package:foo to have an #import "bar.h" from the hook from package:bar. So completely namespacing native code assets per hook might not be desirable.
    • However, if a libbar.so is produced by both package:bar and package:baz hooks, then it is ambiguous which one is imported. (Unless we add a List<NativeCodeAsset> dynamicallyLinkedTo to NativeCodeAsset to explicitly communicate in the protocol which dylibs dynamically link which dylibs.)

I am not sure whether it's worth it to start name spacing.

Thanks @mkustermann for bringing this up!

FYI @blaugold (Somewhat related to the PRs you're currently working on. But just FYI, no reason to change anything there for now. It might invalidate my assertion in https://github.com/dart-lang/native/issues/190, that we don't need to track the dynamic linking between dylibs in the BuildOutput.)

dcharkes avatar Aug 13 '24 09:08 dcharkes

It seems that renaming on Linux might be much harder than renaming on MacOS/iOS.

See the discussion on https://github.com/dart-lang/native/issues/190#issuecomment-2304312550.

dcharkes avatar Aug 22 '24 10:08 dcharkes

https://github.com/flutter/flutter/pull/158214 added duplicate code asset checks to flutter_tools. Thanks @mkustermann! 🙏

dcharkes avatar Nov 27 '24 11:11 dcharkes