Skip to content

[native_assets_cli] Add base path for user-defines #2209

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

Merged
merged 2 commits into from
Apr 16, 2025

Conversation

dcharkes
Copy link
Collaborator

@dcharkes dcharkes commented Apr 15, 2025

Bug: #39
Closes: #2187
Closes: #2201

Relative paths in user-defines accessible as absolute paths in hooks

This PR enables resolving relative paths in user-defines as absolute paths. The input.json now is structured as follows:

{
  "user_defines": {
    "workspace_pubspec": {
      "base_path": "/private/var/folders/2y/mngq9h194yzglt4kzttzfq6800klzg/T/QqYcWN/user_defines/pubspec.yaml",
      "defines": {
        "user_define_key": "user_define_value",
        "user_define_key2": {
          "foo": "bar"
        },
        "some_file": "assets/data.json"
      }
    },
  },
}

We don't want to expose the workspace/pubspec path (except for resolving user-defines), so this can be done in the Dart API as follows:

extension type HookInputUserDefines._(HookInput _input) {
  /// The value for the user-define for [key] for this package.
  ///
  /// This can be arbitrary JSON/YAML if provided from the SDK from such source.
  /// If it's provided from command-line arguments, it's likely a string.
  Object? operator [](String key) => // already present before this PR

  /// The absolute path for user-defines for [key] for this package.key
  ///
  /// The relative path passed as user-define is resolved against the base path.
  /// For user-defines originating from a JSON/YAML, the base path is this
  /// JSON/YAML. For user-defines originating from command-line aruments, the
  /// base path is the working directory of the command-line invocation.
  ///
  /// If the user-define is `null` or not a [String], returns `null`.
  Uri? path(String key) => // added in this PR.
}

This PR does not yet add support for command-line user-defines. But the JSON has been set up so that it's extensible.

Move reading user-defines from flutter_tools/dartdev to NativeAssetsBuildRunner

This PR changes the responsibility to read the user-defines from the pubspec to the NativeAssetsBuildRunner.

  1. This ensures all SDKs have the same logic for reading user-defines
  2. This ensures all SDKs have the same caching behavior for files containing user-defines changing on disk.

This PR adds the pubspec.yaml in the HookOutput.dependencies to ensure the build runner is reinvoked if the pubspec changes.

The API for the NativeAssetsBuildRunner changes to take the pubspec.yaml uri rather than user-defines. Consequently, this PR will need a manual roll into dartdev and flutter_tools.

Copy link

github-actions bot commented Apr 15, 2025

PR Health

Breaking changes ✔️
Package Change Current Version New Version Needed Version Looking good?
native_assets_builder Breaking 0.15.0 0.16.0-wip 0.16.0 ✔️
native_assets_cli Breaking 0.15.0 0.16.0-wip 0.16.0 ✔️
native_toolchain_c Non-Breaking 0.12.0 0.13.0-wip 0.12.1 ✔️
Changelog Entry ✔️
Package Changed Files

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.

Package Leaked API symbols
License Headers ✔️
// Copyright (c) 2025, the Dart project authors. Please see the AUTHORS file
// 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.
Files
no missing headers

All source files should start with a license header.

Unrelated files missing license headers
Files
pkgs/jni/lib/src/third_party/generated_bindings.dart
pkgs/objective_c/lib/src/ns_input_stream.dart

@coveralls
Copy link

coveralls commented Apr 15, 2025

Coverage Status

coverage: 78.633% (-0.7%) from 79.346%
when pulling e98a1bb on user-defines-base-path
into 231e45f on main.

@dcharkes dcharkes force-pushed the user-defines-base-path branch from 7fe82ee to b6e81fa Compare April 15, 2025 19:24
@dcharkes dcharkes force-pushed the user-defines-base-path branch from b6e81fa to 7d2d0ae Compare April 15, 2025 19:32
@dcharkes dcharkes marked this pull request as ready for review April 15, 2025 19:40
@dcharkes dcharkes requested review from goderbauer and mosuem April 15, 2025 19:40
@goderbauer
Copy link

We don't want to expose the workspace/pubspec path (except for resolving user-defines)

We are kind of indirectly exposing the workspace/pubspec path through this: Developers can take a look at the URI returned by path and reverse-engineer it from there. Any concerns about that?

Copy link

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM modulo some nits and the top-level comment

## 0.16.0-wip

- Pass in path to pubspec to read user-defines (and read user-defines) in this
package rather than in the SDKs using this package.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean to repeat "and read user-defines" in the parentheses? Seems odd...

final pubspec => [pubspec],
},
);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wondering if its worthwhile to factor this logic out since it seems to be 1:1 repeated from above?

@dcharkes
Copy link
Collaborator Author

dcharkes commented Apr 16, 2025

Thanks @goderbauer ! 🙏

We don't want to expose the workspace/pubspec path (except for resolving user-defines)

We are kind of indirectly exposing the workspace/pubspec path through this: Developers can take a look at the URI returned by path and reverse-engineer it from there. Any concerns about that?

Yes. They can also (1) look at the outputDirectory which exposes the .dart_tool directory in the pub workspace, and (2) simply look at the JSON directly which passed as an argument to main. Hook writers can do all kinds of things which will make their hooks behave inconsistently in different runs (and consequently make caching broken). E.g. doing something on current time, or based on a file existing that's not reported as a dependency.

I see this more as kind of a best effort. We need to document clearly how caching works: It is whatever is in the input.json + any files reported as HookOutput.dependencies + the transitive Dart sources of the hook itself (and the packge_config.json and pubspec.yaml).

@auto-submit auto-submit bot merged commit 222eb98 into main Apr 16, 2025
38 checks passed
@auto-submit auto-submit bot deleted the user-defines-base-path branch April 16, 2025 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants