Remove package:cli_config & package:args dependencies in package:native_asset_cli
#1598
Conversation
| // for details. All rights reserved. Use of this source code is governed by a | ||
| // BSD-style license that can be found in the LICENSE file. | ||
|
|
||
| String getConfigArgument(List<String> arguments) { |
There was a problem hiding this comment.
We could actually decide that instead of making this based on arguments we set a DART_BUILD_HOOK_CONFIG environment variable containing the configuration.
|
environment variables are passed through: Also, this is the way that the tests run on the Dart CI if I'm not mistaken. You might want to try to change the DEPS file to point to your branch on GitHub and run the pkg bots to verify if this doesn't break the tests on the Dart CI. |
|
I was wondering why CI tests are not running here. It was a test to see if something breaks :)
I'll update the |
We don't have infra to create a Dart SDK CL from GitHub actions here. And there are security concerns about a Dart SDK bot being triggered from GitHub. (Although now that third party contributors PRs no longer automatically run on the CI on GitHub, maybe that concern is gone.) I would have for us to have an automatic roll bot!
Are you saying you want to use package
👍 |
|
So the original idea was the following
Now we don't have to implement this right now. This can be added later on.
I don't know if it would make sense to put this json-patching mechanism into
It would be a general mechanism (as described above) but we would be in control to what extent we allow patching the Since we don't have this general patching mechanism right now, the bundling tools don't offer those flags yet, I'd just do the patching manually for the allowlist (which we should have anyway - see a) above) of things we know about for now (which seems like |
This will first require addressing #39. Currently, the only way is using environment vars. @mosuem is using this in his icu4x package for building from source or not. However, that might simply be inspecting the environment variable directly instead of using the
How would we know which out of all environment variables to patch into the config.json? The idea of the We can do this for keys we know, but we would mandate users using
Right, we don't want to allow overriding the output dir, and currently one can.
That might not work, because some config keys combine json/env-vars/cmd-args instead of override. That's currently done by passing a param to
This is tracked in #39.
SGTM. As noted above this doesn't work with arbitrary environment keys that we don't know about, but we can worry about that later. |
|
Note to self, requires landing #1592 first (which bumps the SDK constraint to a version that will only provide json and not yaml configs). |
…:native_asset_cli`
We don't want to blindly override anything in
Maybe we don't want to allow usage of environment variables at all. I find it already very weird, that our dart-lang/sdk test runner is setting If we look at how flutter tool is implemented: There the flutter building/bundling tool is responsible for locating the right c compiler (e.g. from android ndk) and setting up the right
We want control, for example we may decide environment variables that start with |
mkustermann
force-pushed
the
remove-cli
branch
from
e62668b to
292a570
Compare
PR HealthBreaking changes ✔️
Changelog Entry ✔️
Changes to files need to be accounted for in their respective changelogs. 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.
License Headers ✔️
All source files should start with a license header. Unrelated files missing license headers
Package publish validation ✔️
Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation. |
I guess in your preferred would it should have been Anyway, LGTM to this PR if it's green on a manual roll into the Dart SDK on the pkg bots. |
|
I verified the Though for now I'm not sure we should move this exact logic into
The |
|
Thanks, @dcharkes . There's more refactorings I'm planning of doing, so I think we can wait with rolling this right now. Once there's meaningful changes that warrant rolling, we should publish a new version and |
You mean the CI? Flutter tools passes the c compiler in in the build config.
That's a good idea.
That doesn't work for the ENV_SCRIPT and ENV_SCRIPT_ARGUMENTS. And for hermitic builds I don't believe the engprod team would be happy with adding the compilers to the path (which is what I requested initially). This is why we settled on env vars. |
I want to avoid risky changes blocking rolls of features others might work on this repo (Moritz or me), so I prefer more often rolls. Rolling into Flutter is more painful and requires publishing indeed. Rolling into the Dart SDK gives us at least 50% coverage of the embedders. |
These variables aren't setup in Dart CI infrastructure (neither in luci configuration, nor recipes, ...). Instead our test runner was modified to inject those variables when it runs tests (see pkg/test_runner/lib/src/configuration.dart). So we can probably convince engprod that we want extra things on
Fair enough, I'll roll it. |
Originally the idea that probably led to this
package:cli_configdependency was my thought that we should allow end-users to be able to override specific build config defines. For example to be able to sayBut the idea was that the bundling tool will be responsible for changing the
config.jsonby patching in those key/value pairs into the json tree before giving it to thehook/build.dart(thehook/build.dartisn't invoked by end users and therefore doesn't have to deal with the complexity - it can be encapsulated by the bundling tool). This would also allow an end user to make specific settings for a particular package and only that package will get it's config.json patched (not other packages)