Skip to content
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

[xdg_directories] Migrate to null safety #250

Closed
wants to merge 14 commits into from
5 changes: 3 additions & 2 deletions .ci/Dockerfile
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
# Last updated 10/22/2020 (to rebuild the docker image, update this timestamp)
FROM cirrusci/flutter:stable-web
# Last updated 11/26/2020 (to rebuild the docker image, update this timestamp)
FROM cirrusci/flutter:dev

RUN sudo apt-get update && \
sudo apt-get upgrade --yes && \
Expand All @@ -9,6 +9,7 @@ RUN sudo apt-get update && \
# This must occur after the install of gpg-agent
RUN wget -O - https://apt.llvm.org/llvm-snapshot.gpg.key | sudo apt-key add - && \
sudo apt-add-repository "deb http://apt.llvm.org/xenial/ llvm-toolchain-xenial-5.0 main" && \
sudo apt-add-repository "deb http://archive.ubuntu.com/ubuntu/ xenial main" && \
sudo apt-get update && \
sudo apt-get install --yes --allow-unauthenticated clang-format-5.0 && \
sudo apt-get clean --yes
Expand Down
26 changes: 14 additions & 12 deletions .cirrus.yml
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@ task:
cpu: 4
memory: 8G
upgrade_script:
- flutter channel master
- flutter channel dev
- flutter upgrade
- git fetch origin master
- git fetch origin nnbd
activate_script: pub global activate flutter_plugin_tools
matrix:
- name: analyze
Expand All @@ -31,16 +31,18 @@ task:
- ./script/incremental_build.sh java-test # must come after apk build
depends_on:
- analyze
- name: web_benchmarks_test
script:
- ./script/install_chromium.sh
- export CHROME_EXECUTABLE=$(pwd)/.chromium/chrome-linux/chrome
- flutter config --enable-web
- cd packages/web_benchmarks/testing/test_app
- flutter packages get
- cd ../..
- flutter packages get
- dart testing/web_benchmarks_test.dart
# TODO(shihaohong): Skip because new builds do not contain Chromium:
# https://github.com/cirruslabs/docker-images-flutter/issues/66
# - name: web_benchmarks_test
# script:
# - ./script/install_chromium.sh
# - export CHROME_EXECUTABLE=$(pwd)/.chromium/chrome-linux/chrome
# - flutter config --enable-web
# - cd packages/web_benchmarks/testing/test_app
# - flutter packages get
# - cd ../..
# - flutter packages get
# - dart testing/web_benchmarks_test.dart

task:
use_compute_credits: $CIRRUS_USER_COLLABORATOR == 'true'
Expand Down
4 changes: 4 additions & 0 deletions packages/xdg_directories/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## [0.2.0-nullsafety.0] - Migrated to null safety

* Migrated to null safety.

## [0.1.2] - Reduce dependencies on external libraries.

* Broaden dependencies to allow nullsafety version of process, meta, and path to be OK.
Expand Down
51 changes: 29 additions & 22 deletions packages/xdg_directories/lib/xdg_directories.dart
Original file line number Diff line number Diff line change
Expand Up @@ -13,15 +13,15 @@ import 'package:process/process.dart';

/// An override function used by the tests to override the environment variable
/// lookups using [xdgEnvironmentOverride].
typedef EnvironmentAccessor = String Function(String envVar);
typedef EnvironmentAccessor = String? Function(String envVar);

/// A testing setter that replaces the real environment lookups with an override.
///
/// Set to null to stop overriding.
///
/// Only available to tests.
@visibleForTesting
set xdgEnvironmentOverride(EnvironmentAccessor override) {
set xdgEnvironmentOverride(EnvironmentAccessor? override) {
_xdgEnvironmentOverride = override;
_getenv = _xdgEnvironmentOverride ?? _productionGetEnv;
}
Expand All @@ -31,8 +31,8 @@ set xdgEnvironmentOverride(EnvironmentAccessor override) {
///
/// Only available to tests.
@visibleForTesting
EnvironmentAccessor get xdgEnvironmentOverride => _xdgEnvironmentOverride;
EnvironmentAccessor _xdgEnvironmentOverride;
EnvironmentAccessor? get xdgEnvironmentOverride => _xdgEnvironmentOverride;
EnvironmentAccessor? _xdgEnvironmentOverride;
EnvironmentAccessor _productionGetEnv =
(String value) => Platform.environment[value];
EnvironmentAccessor _getenv = _productionGetEnv;
Expand All @@ -50,9 +50,9 @@ ProcessManager _processManager = const LocalProcessManager();

List<Directory> _directoryListFromEnvironment(
String envVar, List<Directory> fallback) {
assert(envVar != null);
assert(fallback != null);
final String value = _getenv(envVar);
ArgumentError.checkNotNull(envVar);
ArgumentError.checkNotNull(fallback);
final String? value = _getenv(envVar);
if (value == null || value.isEmpty) {
return fallback;
}
Expand All @@ -63,23 +63,30 @@ List<Directory> _directoryListFromEnvironment(
}).toList();
}

Directory _directoryFromEnvironment(String envVar, String fallback) {
assert(envVar != null);
final String value = _getenv(envVar);
Directory? _directoryFromEnvironment(String envVar) {
ArgumentError.checkNotNull(envVar);
final String? value = _getenv(envVar);
if (value == null || value.isEmpty) {
return null;
}
return Directory(value);
}

Directory _directoryFromEnvironmentWithFallback(
String envVar, String fallback) {
ArgumentError.checkNotNull(envVar);
final String? value = _getenv(envVar);
if (value == null || value.isEmpty) {
if (fallback == null) {
return null;
}
return _getDirectory(fallback);
}
return Directory(value);
}

// Creates a Directory from a fallback path.
Directory _getDirectory(String subdir) {
assert(subdir != null);
ArgumentError.checkNotNull(subdir);
assert(subdir.isNotEmpty);
final String homeDir = _getenv('HOME');
final String? homeDir = _getenv('HOME');
if (homeDir == null || homeDir.isEmpty) {
throw StateError(
'The "HOME" environment variable is not set. This package (and POSIX) '
Expand All @@ -94,7 +101,7 @@ Directory _getDirectory(String subdir) {
///
/// Throws [StateError] if the HOME environment variable is not set.
Directory get cacheHome =>
_directoryFromEnvironment('XDG_CACHE_HOME', '.cache');
_directoryFromEnvironmentWithFallback('XDG_CACHE_HOME', '.cache');

/// The list of preference-ordered base directories relative to
/// which configuration files should be searched. (Corresponds to
Expand All @@ -113,7 +120,7 @@ List<Directory> get configDirs {
///
/// Throws [StateError] if the HOME environment variable is not set.
Directory get configHome =>
_directoryFromEnvironment('XDG_CONFIG_HOME', '.config');
_directoryFromEnvironmentWithFallback('XDG_CONFIG_HOME', '.config');

/// The list of preference-ordered base directories relative to
/// which data files should be searched. (Corresponds to `$XDG_DATA_DIRS`).
Expand All @@ -131,14 +138,14 @@ List<Directory> get dataDirs {
///
/// Throws [StateError] if the HOME environment variable is not set.
Directory get dataHome =>
_directoryFromEnvironment('XDG_DATA_HOME', '.local/share');
_directoryFromEnvironmentWithFallback('XDG_DATA_HOME', '.local/share');

/// The base directory relative to which user-specific runtime
/// files and other file objects should be placed. (Corresponds to
/// `$XDG_RUNTIME_DIR`).
///
/// Throws [StateError] if the HOME environment variable is not set.
Directory get runtimeDir => _directoryFromEnvironment('XDG_RUNTIME_DIR', null);
Directory? get runtimeDir => _directoryFromEnvironment('XDG_RUNTIME_DIR');

/// Gets the xdg user directory named by `dirName`.
///
Copy link
Contributor

Choose a reason for hiding this comment

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

Based on a bug that came in recently, we should make getUserDirectory return Directory?, because I think we'll want to add logic to make it catch run failures (e.g., due to xdg-user-dir not being installed) and return null, and changing the return type later would be a breaking change.

Expand All @@ -147,7 +154,7 @@ Directory getUserDirectory(String dirName) {
final ProcessResult result = _processManager.runSync(
<String>['xdg-user-dir', dirName],
includeParentEnvironment: true,
stdoutEncoding: Encoding.getByName('utf8'),
stdoutEncoding: Encoding.getByName('utf8') ?? systemEncoding,
);
final String path = utf8.decode(result.stdout).split('\n')[0];
return Directory(path);
Expand All @@ -172,11 +179,11 @@ Set<String> getUserDirectoryNames() {
final RegExp dirRegExp =
RegExp(r'^\s*XDG_(?<dirname>[^=]*)_DIR\s*=\s*(?<dir>.*)\s*$');
for (String line in contents) {
final RegExpMatch match = dirRegExp.firstMatch(line);
final RegExpMatch? match = dirRegExp.firstMatch(line);
if (match == null) {
continue;
}
result.add(match.namedGroup('dirname'));
result.add(match.namedGroup('dirname')!);
}
return result;
}
14 changes: 7 additions & 7 deletions packages/xdg_directories/pubspec.yaml
Original file line number Diff line number Diff line change
@@ -1,16 +1,16 @@
name: xdg_directories
description: A Dart package for reading XDG directory configuration information on Linux.
version: 0.1.2
version: 0.2.0-nullsafety.0
homepage: https://github.com/flutter/packages/tree/master/packages/xdg_directories

environment:
sdk: ">=2.3.0 <3.0.0"
sdk: ">=2.12.0-0 <3.0.0"

dependencies:
meta: ">=1.2.2 <2.0.0"
path: ">=1.6.4 <2.0.0"
process: ">=3.0.12 <5.0.0"
meta: ^1.3.0-nullsafety.6
path: ^1.8.0-nullsafety.3
process: ^4.0.0-nullsafety.4

dev_dependencies:
mockito: ^4.1.1
test: ^1.15.3
mockito: ^5.0.0-nullsafety.1
test: ^1.16.0-nullsafety.13
24 changes: 11 additions & 13 deletions packages/xdg_directories/test/xdg_directories_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import 'package:xdg_directories/xdg_directories.dart' as xdg;

void main() {
final Map<String, String> fakeEnv = <String, String>{};
Directory tmpDir;
late Directory tmpDir;

String testPath(String subdir) => path.join(tmpDir.path, subdir);

Expand All @@ -29,11 +29,11 @@ void main() {
'${testPath('usr/local/test_share')}:${testPath('usr/test_share')}';
fakeEnv['XDG_DATA_HOME'] = testPath('.local/test_share');
fakeEnv['XDG_RUNTIME_DIR'] = testPath('.local/test_runtime');
Directory(fakeEnv['XDG_CONFIG_HOME']).createSync(recursive: true);
Directory(fakeEnv['XDG_CACHE_HOME']).createSync(recursive: true);
Directory(fakeEnv['XDG_DATA_HOME']).createSync(recursive: true);
Directory(fakeEnv['XDG_RUNTIME_DIR']).createSync(recursive: true);
File(path.join(fakeEnv['XDG_CONFIG_HOME'], 'user-dirs.dirs'))
Directory(fakeEnv['XDG_CONFIG_HOME']!).createSync(recursive: true);
Directory(fakeEnv['XDG_CACHE_HOME']!).createSync(recursive: true);
Directory(fakeEnv['XDG_DATA_HOME']!).createSync(recursive: true);
Directory(fakeEnv['XDG_RUNTIME_DIR']!).createSync(recursive: true);
File(path.join(fakeEnv['XDG_CONFIG_HOME']!, 'user-dirs.dirs'))
.writeAsStringSync(r'''
XDG_DESKTOP_DIR="$HOME/Desktop"
XDG_DOCUMENTS_DIR="$HOME/Documents"
Expand All @@ -48,9 +48,7 @@ XDG_VIDEOS_DIR="$HOME/Videos"
});

tearDown(() {
if (tmpDir != null) {
tmpDir.deleteSync(recursive: true);
}
tmpDir.deleteSync(recursive: true);
// Stop overriding the environment accessor.
xdg.xdgEnvironmentOverride = null;
});
Expand All @@ -76,7 +74,7 @@ XDG_VIDEOS_DIR="$HOME/Videos"
expect(xdg.cacheHome.path, equals(testPath('.test_cache')));
expect(xdg.configHome.path, equals(testPath('.test_config')));
expect(xdg.dataHome.path, equals(testPath('.local/test_share')));
expect(xdg.runtimeDir.path, equals(testPath('.local/test_runtime')));
expect(xdg.runtimeDir!.path, equals(testPath('.local/test_runtime')));
Copy link
Contributor

Choose a reason for hiding this comment

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

This ! is not correct; the API can return a null. You need to validate both that it's not null, and that it matches if it's not.


expectDirList(xdg.configDirs, <String>[testPath('etc/test_xdg')]);
expectDirList(xdg.dataDirs, <String>[
Expand Down Expand Up @@ -122,13 +120,13 @@ class FakeProcessManager extends Fake implements ProcessManager {
@override
ProcessResult runSync(
List<dynamic> command, {
String workingDirectory,
Map<String, String> environment,
String? workingDirectory,
Map<String, String>? environment,
bool includeParentEnvironment = true,
bool runInShell = false,
Encoding stdoutEncoding = systemEncoding,
Encoding stderrEncoding = systemEncoding,
}) {
return ProcessResult(0, 0, expected[command[1]].codeUnits, <int>[]);
return ProcessResult(0, 0, expected[command[1]]!.codeUnits, <int>[]);
}
}