Skip to content

Commit 3f2ef80

Browse files
simolus3simolus3
committed
Review feedback
1 parent f0e1c12 commit 3f2ef80

File tree

5 files changed

+115
-40
lines changed

5 files changed

+115
-40
lines changed

‎pkgs/hooks_runner/CHANGELOG.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,8 @@
1+
## 0.23.3-wip
2+
3+
- Add `ANDROID_NDK`, `ANDROID_NDK_HOME`, `ANDROID_NDK_LATEST_HOME` and
4+
`ANDROID_NDK_ROOT` to the environment variables allowlist.
5+
16
## 0.23.2
27

38
- Add `LIBCLANG_PATH` to the environment variables allowlist.

‎pkgs/hooks_runner/lib/src/build_runner/build_runner.dart‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -543,8 +543,20 @@ class NativeAssetsBuildRunner {
543543
///
544544
/// This allows environment variables needed to run mainstream compilers.
545545
static bool includeHookEnvironmentVariable(String environmentVariableName) {
546+
// Typically, we'd find the NDK through ANDROID_HOME alone. In some cases
547+
// where users have an NDK in nonstandard locations, these environment
548+
// variables are used as well, e.g. in
549+
// https://github.com/actions/runner-images/blob/main/images/ubuntu/Ubuntu2404-Readme.md#environment-variables-2
550+
const nonStandardNdkEnvironmentVariables = {
551+
'ANDROID_NDK',
552+
'ANDROID_NDK_HOME',
553+
'ANDROID_NDK_LATEST_HOME',
554+
'ANDROID_NDK_ROOT',
555+
};
556+
546557
const staticVariablesFilter = {
547558
'ANDROID_HOME', // Needed for the NDK.
559+
...nonStandardNdkEnvironmentVariables,
548560
'HOME', // Needed to find tools in default install locations.
549561
'LIBCLANG_PATH', // Needed for Rust's bindgen + clang-sys.
550562
'PATH', // Needed to invoke native tools.

‎pkgs/hooks_runner/pubspec.yaml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ name: hooks_runner
22
description: >-
33
This package is the backend that invokes build hooks.
44
5-
version: 0.23.2
5+
version: 0.23.3-wip
66

77
repository: https://github.com/dart-lang/native/tree/main/pkgs/hooks_runner
88

‎pkgs/native_toolchain_c/lib/src/native_toolchain/android_ndk.dart‎

Lines changed: 38 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -5,16 +5,16 @@
55
import 'dart:io';
66

77
import 'package:code_assets/code_assets.dart';
8+
import 'package:glob/glob.dart';
89
import 'package:logging/logging.dart';
9-
import 'package:path/path.dart' as p;
1010

1111
import '../tool/tool.dart';
1212
import '../tool/tool_instance.dart';
1313
import '../tool/tool_resolver.dart';
1414
import 'clang.dart';
1515

1616
final androidNdk = Tool(
17-
name: 'Android NDK',
17+
name: _AndroidNdkResolver._toolName,
1818
defaultResolver: _AndroidNdkResolver(),
1919
);
2020

@@ -37,46 +37,43 @@ final androidNdkLld = Tool(
3737
);
3838

3939
class _AndroidNdkResolver implements ToolResolver {
40-
ToolResolver installLocationResolver(ToolResolvingContext context) =>
41-
PathVersionResolver(
42-
wrappedResolver: ToolResolvers([
43-
RelativeToolResolver(
44-
toolName: 'Android NDK',
45-
wrappedResolver: PathToolResolver(
46-
toolName: 'ndk-build',
47-
executableName: Platform.isWindows
48-
? 'ndk-build.cmd'
49-
: 'ndk-build',
50-
),
51-
relativePath: Uri(path: ''),
52-
),
53-
InstallLocationResolver(
54-
toolName: 'Android NDK',
55-
paths: [
56-
if (context.environment['ANDROID_HOME'] case final androidHome?)
57-
p.join(androidHome, 'ndk/*/'),
58-
for (final ndkHomeKey in _ndkHomeEnvironmentVariables)
59-
if (context.environment[ndkHomeKey] case final home?) home,
60-
61-
if (Platform.isLinux) ...[
62-
'\$HOME/.androidsdkroot/ndk/*/', // Firebase Studio
63-
'\$HOME/Android/Sdk/ndk/*/',
64-
'\$HOME/Android/Sdk/ndk-bundle/',
65-
],
66-
if (Platform.isMacOS) ...['\$HOME/Library/Android/sdk/ndk/*/'],
67-
if (Platform.isWindows) ...[
68-
'\$HOME/AppData/Local/Android/Sdk/ndk/*/',
69-
],
70-
],
71-
),
72-
]),
73-
);
40+
static final installLocationResolver = PathVersionResolver(
41+
wrappedResolver: ToolResolvers([
42+
RelativeToolResolver(
43+
toolName: _toolName,
44+
wrappedResolver: PathToolResolver(
45+
toolName: 'ndk-build',
46+
executableName: Platform.isWindows ? 'ndk-build.cmd' : 'ndk-build',
47+
),
48+
relativePath: Uri(path: ''),
49+
),
50+
InstallLocationResolver(
51+
toolName: _toolName,
52+
paths: [
53+
if (Platform.isLinux) ...[
54+
'\$HOME/.androidsdkroot/ndk/*/', // Firebase Studio
55+
'\$HOME/Android/Sdk/ndk/*/',
56+
'\$HOME/Android/Sdk/ndk-bundle/',
57+
],
58+
if (Platform.isMacOS) ...['\$HOME/Library/Android/sdk/ndk/*/'],
59+
if (Platform.isWindows) ...[
60+
'\$HOME/AppData/Local/Android/Sdk/ndk/*/',
61+
],
62+
],
63+
),
64+
EnvironmentVariableResolver(
65+
toolName: _toolName,
66+
keys: {
67+
'ANDROID_HOME': Glob('ndk/*/'),
68+
for (final ndkHome in _ndkHomeEnvironmentVariables) ndkHome: null,
69+
},
70+
),
71+
]),
72+
);
7473

7574
@override
7675
Future<List<ToolInstance>> resolve(ToolResolvingContext context) async {
77-
final ndkInstances = await installLocationResolver(
78-
context,
79-
).resolve(context);
76+
final ndkInstances = await installLocationResolver.resolve(context);
8077

8178
return [
8279
for (final ndkInstance in ndkInstances) ...[
@@ -146,4 +143,6 @@ class _AndroidNdkResolver implements ToolResolver {
146143
'ANDROID_NDK_LATEST_HOME',
147144
'ANDROID_NDK_ROOT',
148145
];
146+
147+
static const _toolName = 'Android NDK';
149148
}

‎pkgs/native_toolchain_c/lib/src/tool/tool_resolver.dart‎

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,9 @@ abstract class ToolResolver {
2828
/// global state not available in this context. Not all resolvers adhere to that
2929
/// though, since some need to run subprocesses to resolve tools.
3030
final class ToolResolvingContext {
31+
// TODO: Expose package:file and package:process environments here and use
32+
// them in resolvers to consistently mock external state.
33+
3134
final Logger? logger;
3235
final Map<String, String> environment;
3336

@@ -200,6 +203,7 @@ class ToolResolvers implements ToolResolver {
200203
];
201204
}
202205

206+
/// A tool resolver that looks for tools in a glob of [paths] if they exist.
203207
class InstallLocationResolver implements ToolResolver {
204208
final String toolName;
205209
final List<String> paths;
@@ -262,6 +266,61 @@ class InstallLocationResolver implements ToolResolver {
262266
}();
263267
}
264268

269+
/// A tool resolver considering environment variables such as `ANDROID_HOME`.
270+
class EnvironmentVariableResolver implements ToolResolver {
271+
final String toolName;
272+
273+
/// Considered environment variables, mapped to a [Glob] identifying paths in
274+
/// the variable to consider.
275+
///
276+
/// For instance, `{'ANDROID_HOME': Glob('ndk/*/')}` would report one tool
277+
/// instance per sub-directory of `$ANDROID_HOME/ndk/` if that environment
278+
/// variable is set.
279+
///
280+
/// A null value would consider the key itself as a directory.
281+
final Map<String, Glob?> keys;
282+
283+
EnvironmentVariableResolver({required this.toolName, required this.keys});
284+
285+
@override
286+
Future<List<ToolInstance>> resolve(ToolResolvingContext context) async {
287+
final logger = context.logger;
288+
final foundTools = <ToolInstance>[];
289+
for (final MapEntry(:key, value: glob) in keys.entries) {
290+
logger?.fine('Looking for $toolName in environment variable $key');
291+
if (context.environment[key] case final found?) {
292+
final fileSystemEntities = switch (glob) {
293+
null => [Directory(found)],
294+
final glob =>
295+
await glob
296+
.list(root: found)
297+
.where(
298+
// If the path ends in /, only consider directories
299+
(entity) =>
300+
entity is Directory || !glob.pattern.endsWith('/'),
301+
)
302+
.toList(),
303+
};
304+
305+
for (final fileSystemEntity in fileSystemEntities) {
306+
if (!await fileSystemEntity.exists()) {
307+
continue;
308+
}
309+
310+
foundTools.add(
311+
ToolInstance(
312+
tool: Tool(name: toolName),
313+
uri: fileSystemEntity.uri,
314+
),
315+
);
316+
}
317+
}
318+
}
319+
320+
return foundTools;
321+
}
322+
}
323+
265324
class RelativeToolResolver implements ToolResolver {
266325
final String toolName;
267326
final ToolResolver wrappedResolver;

0 commit comments

Comments
 (0)