General pattern should be that Uris in BuildConfig/LinkConfig should be absolute

[mkustermann]

There's various kinds of Uris in the BuildConfig/LinkConfig, the current deserialization logic from json does more than json decoding, it also resolves uris relative to a base (this was done in package:cli_config before, but I kept this logic when removing this dependency in https://github.com/dart-lang/native/commit/7f206e39c3d5042658e22391726c331fc6d606ec)

The running example to explain the issue will be the paths to compiler executables (cc/ar/ld/...).

I think it's problematic to auto-resolve such uris relative to a base (the base I believe is the config.json file itself):

In terms of layering we have a) an end user (that in the future may pass --build-config=config.compiler.cc=<path>) b) flutter build (which currently discovers the compiler paths e.g. from android sdk) c) package:native_assets_builder - which is passed a CompilerConfigImpl from the layer above and will

• internally create a temporary folder
• write the config.json file into it
• invoke the build/hook.dart (currently with a currentWorkingDirectory passed from above)

Now we can see that the layers in a) and b) are determining the actual paths / uris and c) is just invoking the hook. That means nowhere in this system will we ensure that if the paths are relative, that they are relative to config.json.

Now we could solve this by updating package:native_assets_builder to re-write anything it gets from the upper layers to be actually relative to config.json, but this wouldn't work in a world where arbitrary configuration can be passed down that the package:native_assets_builder doesn't know about.

=> tl;dr: I think we should no longer try to resolve Uris encoded in config.json relative to that config.json file.

There's another question about the currentWorkingDirectory: Currently the upper layer (e.g. flutter build) passes the working directory down to the package:native_assets_builder. I'm not sure if this is the right thing to do: We want the hook/build.dart to be invoked in a similar environment - irrespective of whether it's used as part of flutter build or dart build. So the upper layer should only pass down a package config and package:native_asset_builder can then determine the working directory to use (it may be the directory where the hook/build.dart script lies (e.g. in ~/.pub-cache/*/foo-1.2/) or it even may be a temporary directory. (A hook/build.dart script should have access to it's own package, not to the root application package IMHO)

[mkustermann]

/cc @dcharkes for discussion

[dcharkes]

it also resolves uris relative to a base

Paths from the JSON are resolved relative to the JSON. Paths from commandline args are resolved relative to the current working directory. Env paths are expected to be absolute.

With you having removed the cli_config this reasoning is now broken.

Making paths in the JSON be absolute sgtm.

currentWorkingDirectory with relative paths enables dart run -Bmy_package.config.my_key=foo.txt. If we don't have a current working dir, we cannot pass in paths from command-line APIs. Maybe that user journey can be supported in another way? Maybe not. Any ideas? (Actually, this makes me think it's a bad idea to conflate command-line args and JSON config.)

[mkustermann]

With you having removed the cli_config this reasoning is now broken.

I think there were no guarantees that this ever worked with cli_config either.

A user may have a relative path in an environment variable, but the actual hook/build.dart invocation may be happening with a different working directory (depending on the bundling tool) than the user originally invoked the bundling tool in, thereby making the relative uri be resolved incorrectly.

Similarly a relative uri passed down from CCompilerConfig from the bundling tool is very likely to be always incorrect because the bundling tool doesn't know where the temp directory or the config.json file is, thereby making the relative uri in json['c_compiler']['cc']be resolved incorrectly (against config.json).

currentWorkingDirectory with relative paths enables dart run -Bmy_package.config.my_key=foo.txt. If we don't have a current working dir, we cannot pass in paths from command-line APIs. Maybe that user journey can be supported in another way? Maybe not. Any ideas? (Actually, this makes me think it's a bad idea to conflate command-line args and JSON config.)

So my gut feeling is that the invocation of hook/build.dart should happen in an as much isolated environment as possible (it may not be a hermetic, sandboxed build - but the closer we get to it the better). Building a transitive package's assets shouldn't depend on which working directory the build happened to be triggered on, which application root package there is, ... or in which directory the resulting build is later going to be executed. This way we'd avoid hook/build.dart writers start depending things they shouldn't depend on.

The dart run -Bmy_package.config.my_key=foo.txt use case is interesting. The dart run command internally does a) build and then b) runs the build. We want to make the build step (in a) ) re-usable across dart run - irrespective of which directory the dart run happens. So maybe it's not too bad to require users to write dart run -Bmy_package.config.my_key=$PWD/foo.txt

[dcharkes]

Yes for the config.json it makes sense to force absolute uris.

I think there were no guarantees that this ever worked with cli_config either.

It did when I wrote it. We used to have unit tests that invoked with command-line defines. But we recently replaced those to use config files because they broke every time we added new required config variable.

So maybe it's not too bad to require users to write dart run -Bmy_package.config.my_key=$PWD/foo.txt

That seems not user friendly, imagine requiring the same thing for commandline arguments for anything else. dart foo.dart foo.txt etc.

However, since we don't have user defines currently, and this is not very likely to be used with the compilers, we can just remove this functionality and worry about such user journey later (and do another refactoring later if we deem that user journey worth the complexity).

[mkustermann]

So maybe it's not too bad to require users to write dart run -Bmy_package.config.my_key=$PWD/foo.txt

That seems not user friendly, imagine requiring the same thing for commandline arguments for anything else. dart foo.dart foo.txt etc.

Currently the bundling tools probably will eventually run hook/build.dart in the same working directory as the user launched flutter/dart run/build. So currently the relative paths will be from highest to lowest level just be embedded in config.json and the hook can open the files. (the PR I've sent out doesn't enforce that they're absolute paths - just that we no longer meddle with the uris we get in the hook)

This would mainly start to become an issue if/when we run the hooks with a different working directory (which may make sense - to avoid hooks assuming/depending they are run from the application package)