-
Notifications
You must be signed in to change notification settings - Fork 42
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
General pattern should be that Uris in BuildConfig/LinkConfig should be absolute #1605
Comments
/cc @dcharkes for discussion |
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.
|
I think there were no guarantees that this ever worked with A user may have a relative path in an environment variable, but the actual Similarly a relative uri passed down from
So my gut feeling is that the invocation of The |
This makes a `hook/{build,link}.dart` no longer resolve uris encoded in the `config.json`. It still validates that the uris that are encoded exist when it decodes the json. The creator of `config.json` has to ensure that the uris it encodes in that config are existing file urls that the `hook/{build,link}.dart` scripts can open as-is (i.e. without resolving against any base uri). In many cases that means that an invoker should encode absolute uris in there - or if it encodes relative uris - guarantees that they are relative to the working directory the `hook/{build,link}.dart` is invoked. Issue #1605
Yes for the config.json it makes sense to force absolute uris.
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.
That seems not user friendly, imagine requiring the same thing for commandline arguments for anything else. 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). |
Currently the bundling tools probably will eventually run 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) |
This makes a `hook/{build,link}.dart` no longer resolve uris encoded in the `config.json`. It still validates that the uris that are encoded exist when it decodes the json. The creator of `config.json` has to ensure that the uris it encodes in that config are existing file urls that the `hook/{build,link}.dart` scripts can open as-is (i.e. without resolving against any base uri). In many cases that means that an invoker should encode absolute uris in there - or if it encodes relative uris - guarantees that they are relative to the working directory the `hook/{build,link}.dart` is invoked. Issue #1605
There's various kinds of
Uri
s in theBuildConfig
/LinkConfig
, the current deserialization logic from json does more than json decoding, it also resolves uris relative to a base (this was done inpackage:cli_config
before, but I kept this logic when removing this dependency in 7f206e3)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 aCompilerConfigImpl
from the layer above and willconfig.json
file into itbuild/hook.dart
(currently with acurrentWorkingDirectory
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 toconfig.json
, but this wouldn't work in a world where arbitrary configuration can be passed down that thepackage: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 thatconfig.json
file.There's another question about the
currentWorkingDirectory
: Currently the upper layer (e.g.flutter build
) passes the working directory down to thepackage:native_assets_builder
. I'm not sure if this is the right thing to do: We want thehook/build.dart
to be invoked in a similar environment - irrespective of whether it's used as part offlutter build
ordart build
. So the upper layer should only pass down a package config andpackage:native_asset_builder
can then determine the working directory to use (it may be the directory where thehook/build.dart
script lies (e.g. in~/.pub-cache/*/foo-1.2/
) or it even may be a temporary directory. (Ahook/build.dart
script should have access to it's own package, not to the root application package IMHO)The text was updated successfully, but these errors were encountered: