dart-lang / build

A build system for Dart written in Dart
https://pub.dev/packages/build
BSD 3-Clause "New" or "Revised" License
778 stars 205 forks source link

--dart-define-from-file not available in build_runner #3574

Open edeetee opened 11 months ago

edeetee commented 11 months ago

Problem

In some of my code generation, I use environment variables. Those environment variables are stored in an env.json, as per flutter recommendations. I can't load my environment variables into build runner, which is causing problems. What should I do?

jakemac53 commented 11 months ago

Can you elaborate a bit more on exactly why you need this? In general we don't support environment defines, and doing so in a proper way (for invalidation) would be very challenging and a lot of work.

natebosch commented 11 months ago

In some of my code generation, I use environment variables.

Can these use cases be moved to read from builder options defined in the build config? https://pub.dev/packages/build_config#configuring-builders-applied-to-your-package

edeetee commented 11 months ago

Thanks for having a look at my use case :)

I'm using a few build_runner libraries, including riverpod and ferry_generator. The problems I have are as a consumer of these libraries, intersecting with my own tooling.

I have a custom Assert annotation to make it a compiler error to forget to add an api key. Perhaps there is an variable I can use to ignore this on a build_runner compile?

Here is the assert class:

class Assert {
  const Assert(bool test, [Object? message]) : assert(test, message);
}

And the api key that causes a compiler error with build_runner:

@Assert(STREAM_CHAT_API_KEY.length != 0, "STREAM_CHAT_API_KEY is empty")
const String STREAM_CHAT_API_KEY =
    String.fromEnvironment("STREAM_CHAT_API_KEY");
jakemac53 commented 11 months ago

Perhaps there is an variable I can use to ignore this on a build_runner compile?

The builders you use would have to be the ones to provide that configuration I think? I am curious exactly how the error surfaces. I haven't seen a situation like this before where an annotation has an assert in its constructor.

edeetee commented 11 months ago

The Assert annotation has the value of failing if I've forgotten to add the key in CI/CD. I had the problem once of setting up a new build on iOS and forgetting to give it the env vars. Ended up pushing a prod version that couldn't access the api, not very good!

Heres the failing log output for build_runner:

 *  Executing task: /Users/edwardtaylor/opt/flutter/bin/flutter pub run build_runner build 

Deprecated. Use `dart run` instead.
[INFO] Generating build script completed, took 218ms
[INFO] Reading cached asset graph completed, took 68ms
[INFO] Checking for updates since last build completed, took 777ms
[INFO] Running build completed, took 15ms
[INFO] Caching finalized dependency graph completed, took 48ms
[SEVERE] freezed on lib/util/env.dart (cached):

Could not resolve annotation for `String STREAM_CHAT_API_KEY`.
[SEVERE] riverpod_generator on lib/util/env.dart (cached):

Could not resolve annotation for `String STREAM_CHAT_API_KEY`.
[SEVERE] Failed after 72ms

 *  The terminal process "/Users/edwardtaylor/opt/flutter/bin/flutter 'pub', 'run', 'build_runner', 'build'" terminated with exit code: 1. 
 *  Terminal will be reused by tasks, press any key to close it. 
jakemac53 commented 11 months ago

The Assert annotation has the value of failing if I've forgotten to add the key in CI/CD.

Yeah I get what you are doing with it, and it's a creative solution. I just haven't seen it before :). I am a bit surprised it doesn't show up as an analysis error when you run the analyzer normally as well, but maybe you are providing environment variables to the analyzer somehow?

Could not resolve annotation for String STREAM_CHAT_API_KEY.

Thanks, this was enough to get me closer to the actual issue. It looks like this is the toString for the UnresolvedAnnotationException from source_gen.

There is a general option throwOnUnresolved to the various methods here, but it defaults to true. It looks like GeneratorForAnnotation doesn't pass anything, which is likely what these builders are using.

Would you mind running the same build with --verbose so I can confirm with a stack trace the exact origin of the failure?

We likely want to add a way to configure this, or possibly even change the default. cc @natebosch

We could also look into actually supporting environment variables but I don't personally have a lot of time to spend on that and it would be fairly involved.

edeetee commented 11 months ago

Yea I just found it online somewhere, I'm don't know how dart lets you extend from a function but I'm not complaining!

I'm running vscode, I set my env vars for my editor in .vscode/settings.json. Possibly the analyzer is picking it up there?

{
"dart.flutterRunAdditionalArgs": [
        "--web-port", "8080",
        "--web-hostname", "0.0.0.0",
        "--web-launch-url", "http://localhost:8080/",
        "--dart-define-from-file", ".env.json"
        ],
}

And here's the whole output with --verbose:

➜ dart pub run build_runner build --verbose
FINE: Pub 3.1.0
MSG : Deprecated. Use `dart run` instead.
FINE: Package Config up to date.
FINE: Package Config up to date.
[INFO] Entrypoint:Generating build script...
[INFO] Entrypoint:Generating build script completed, took 202ms

[INFO] BuildDefinition:Initializing inputs
[INFO] BuildDefinition:Reading cached asset graph...
[INFO] BuildDefinition:Reading cached asset graph completed, took 71ms

[INFO] BuildDefinition:Checking for updates since last build...
[INFO] BuildDefinition:Checking for updates since last build completed, took 774ms

[INFO] Build:Running build...
[INFO] Build:Running build completed, took 13ms

[INFO] Build:Caching finalized dependency graph...
[INFO] Build:Caching finalized dependency graph completed, took 51ms

[SEVERE] freezed on lib/util/env.dart (cached):

Could not resolve annotation for `String STREAM_CHAT_API_KEY`.
package:source_gen/src/type_checker.dart 114:7   TypeChecker._computeConstantValue
package:source_gen/src/type_checker.dart 139:21  TypeChecker._annotationsWhere
dart:core                                        Iterable.isEmpty
package:source_gen/src/type_checker.dart 73:20   TypeChecker.firstAnnotationOf
package:source_gen/src/type_checker.dart 80:7    TypeChecker.hasAnnotationOf
dart:_internal                                   WhereIterator.moveNext
package:freezed/src/parse_generator.dart 29:37   ParserGenerator.generate
package:source_gen/src/builder.dart 355:23       _generate

[SEVERE] riverpod_generator on lib/util/env.dart (cached):

Could not resolve annotation for `String STREAM_CHAT_API_KEY`.
package:source_gen/src/type_checker.dart 114:7             TypeChecker._computeConstantValue
package:source_gen/src/type_checker.dart 139:21            TypeChecker._annotationsWhere
dart:core                                                  Iterable.isEmpty
package:source_gen/src/type_checker.dart 95:20             TypeChecker.firstAnnotationOfExact
package:source_gen/src/library.dart 67:34                  LibraryReader.annotatedWithExact
dart:async                                                 _SyncStarIterator.moveNext
package:riverpod_generator/src/parse_generator.dart 23:37  ParserGenerator.generate
package:source_gen/src/builder.dart 355:33                 _generate

[SEVERE] Build:
Failed after 82ms
IO  : Writing 8723 characters to text file /Users/edwardtaylor/.pub-cache/log/pub_log.txt.

Yea I was hoping that env vars would be something that would come for free with dart stuff, it's a pretty edge use case so don't think its worth that much time.

jakemac53 commented 11 months ago

Ah, so in this case freezed/riverpod do use the TypeChecker directly (example), so you could ask those packages to have a mode where they pass throwOnUnresolved: false. Or we could consider changing the default.

natebosch commented 11 months ago

We could also look into actually supporting environment variables but I don't personally have a lot of time to spend on that and it would be fairly involved.

Yeah - I think we'd have to get the config through to the analyzer, which would necessitate a public API for that in build_resolvers. We'd also need to invalidate the entire build (or anything that used the resolver at least) any time the environment changes at all - I think we'd have to be pessimistic because there won't be any way to tell which uses of the analysis context were impacted by the environment.

Or we could consider changing the default.

It might be a good idea - I don't know if we should consider it breaking. Turning an error into a non-error probably won't cause much churn for existing usage.

It looks like the current default was the backwards compatible one, and I don't see any discussion about whether it made more sense as the default. https://github.com/dart-lang/source_gen/pull/249

I can imagine that the throwing default behavior was most useful when the unresolved annotation is the one that the builder wants to read. When we rolled that out it was typical to have only the annotations for one type of codegen applied to that library, and it would be more confusing if nothing was output than to see the error. It might be more common now to have multiple annotations for different purposes - and it might not make sense for one unresolved annotation to interfere with the others.

rubenferreira97 commented 2 months ago

I was looking for a method to enforce the presence of environment variables and validate their values in my Dart program during compilation (interestingly, arrived at the same solution). I encountered the same problem as the OP.

lib/env.dart

class Assert {
  const Assert(bool condition, [Object? message]) : assert(condition, message);
}

@Assert(bool.hasEnvironment('MAPS_API_KEY'))
const mapsKey = String.fromEnvironment('MAPS_API_KEY');

@Assert(bool.hasEnvironment('HTTP_PROTOCOL'))
@Assert(httpProtocol == 'http' || httpProtocol == 'https') // this works, awesome :)
const httpProtocol = String.fromEnvironment('HTTP_PROTOCOL');

@Assert(bool.hasEnvironment('HTTP_HOST'))
const httpHost = String.fromEnvironment('HTTP_HOST');

@Assert(bool.hasEnvironment('HTTP_PORT'))
const httpPort = int.fromEnvironment('HTTP_PORT');

debug.env

MAPS_API_KEY=MY_API_KEY
HTTP_PROTOCOL=http
HTTP_HOST=192.168.1.2
HTTP_PORT=80

As a quick workaround I am disabling build_runner for this file:

build.yaml

targets:
  $default:
    sources:
      exclude:
        - lib/env.dart

But it would be awesome if we could inject environment variables in build_runner like: dart run build_runner build --dart-define-from-file=debug.env dart run build_runner build --dart-define=API_URL=https://api.example.com


Side Note: It would also be great if RegExp had const capabilities so we could validate inputs like:

@Assert(RegExp(r'^\d+$').hasMatch(httpPort))
const httpPort = String.fromEnvironment('HTTP_PORT');