Make artifact downloading more robust for flaky networks (#13984) * First version * Add a clarifying comment * Add verify to ZipDecoder.decodeBytes call * Review comments * Need OS Error for missing files * FileSystemException, not OSError.
diff --git a/packages/flutter_tools/lib/src/base/os.dart b/packages/flutter_tools/lib/src/base/os.dart index 476cae1..a1c7d8a 100644 --- a/packages/flutter_tools/lib/src/base/os.dart +++ b/packages/flutter_tools/lib/src/base/os.dart
@@ -47,8 +47,14 @@ void unzip(File file, Directory targetDirectory); + /// Returns true if the ZIP is not corrupt. + bool verifyZip(File file); + void unpack(File gzippedTarFile, Directory targetDirectory); + /// Returns true if the gzip is not corrupt (does not check tar). + bool verifyGzip(File gzippedFile); + /// Returns a pretty name string for the current operating system. /// /// If available, the detailed version of the OS is included. @@ -99,6 +105,9 @@ runSync(<String>['unzip', '-o', '-q', file.path, '-d', targetDirectory.path]); } + @override + bool verifyZip(File zipFile) => exitsHappy(<String>['zip', '-T', zipFile.path]); + // tar -xzf tarball -C dest @override void unpack(File gzippedTarFile, Directory targetDirectory) { @@ -106,6 +115,9 @@ } @override + bool verifyGzip(File gzippedFile) => exitsHappy(<String>['gzip', '-t', gzippedFile.path]); + + @override File makePipe(String path) { runSync(<String>['mkfifo', path]); return fs.file(path); @@ -179,6 +191,18 @@ } @override + bool verifyZip(File zipFile) { + try { + new ZipDecoder().decodeBytes(zipFile.readAsBytesSync(), verify: true); + } on FileSystemException catch (_) { + return false; + } on ArchiveException catch (_) { + return false; + } + return true; + } + + @override void unpack(File gzippedTarFile, Directory targetDirectory) { final Archive archive = new TarDecoder().decodeBytes( new GZipDecoder().decodeBytes(gzippedTarFile.readAsBytesSync()), @@ -186,6 +210,18 @@ _unpackArchive(archive, targetDirectory); } + @override + bool verifyGzip(File gzipFile) { + try { + new GZipDecoder().decodeBytes(gzipFile.readAsBytesSync(), verify: true); + } on FileSystemException catch (_) { + return false; + } on ArchiveException catch (_) { + return false; + } + return true; + } + void _unpackArchive(Archive archive, Directory targetDirectory) { for (ArchiveFile archiveFile in archive.files) { // The archive package doesn't correctly set isFile.
diff --git a/packages/flutter_tools/lib/src/cache.dart b/packages/flutter_tools/lib/src/cache.dart index fd9dcb9..503ffbd 100644 --- a/packages/flutter_tools/lib/src/cache.dart +++ b/packages/flutter_tools/lib/src/cache.dart
@@ -162,6 +162,9 @@ return dir; } + /// Return the top-level directory for artifact downloads. + Directory getDownloadDir() => getCacheDir('downloads'); + /// Return the top-level mutable directory in the cache; this is `bin/cache/artifacts`. Directory getCacheArtifacts() => getCacheDir('artifacts'); @@ -232,6 +235,12 @@ Directory get location => cache.getArtifactDirectory(name); String get version => cache.getVersionFor(name); + /// Keep track of the files we've downloaded for this execution so we + /// can delete them after completion. We don't delete them right after + /// extraction in case [update] is interrupted, so we can restart without + /// starting from scratch. + final List<File> _downloadedFiles = <File>[]; + bool isUpToDate() { if (!location.existsSync()) return false; @@ -246,6 +255,14 @@ location.createSync(recursive: true); await updateInner(); cache.setStampFor(name, version); + _removeDownloadedFiles(); + } + + /// Clear any zip/gzip files downloaded. + void _removeDownloadedFiles() { + for (File f in _downloadedFiles) { + f.deleteSync(); + } } /// Hook method for extra checks for being up-to-date. @@ -263,6 +280,36 @@ } Uri _toStorageUri(String path) => Uri.parse('$_storageBaseUrl/$path'); + + /// Download a zip archive from the given [url] and unzip it to [location]. + Future<Null> _downloadZipArchive(Uri url, Directory location) { + return _withDownloadFile('${_flattenName(url)}.zip', (File tempFile) async { + if (!os.verifyZip(tempFile)) { + await _downloadFile(url, tempFile); + } + _ensureExists(location); + os.unzip(tempFile, location); + }); + } + + /// Download a gzipped tarball from the given [url] and unpack it to [location]. + Future<Null> _downloadZippedTarball(Uri url, Directory location) { + return _withDownloadFile('${_flattenName(url)}.tgz', (File tempFile) async { + if (!os.verifyGzip(tempFile)) { + await _downloadFile(url, tempFile); + } + _ensureExists(location); + os.unpack(tempFile, location); + }); + } + + /// Create a temporary file and invoke [onTemporaryFile] with the file as + /// argument, then add the temporary file to the [_downloadedFiles]. + Future<Null> _withDownloadFile(String name, Future<Null> onTemporaryFile(File file)) async { + final File tempFile = fs.file(fs.path.join(cache.getDownloadDir().path, name)); + _downloadedFiles.add(tempFile); + await onTemporaryFile(tempFile); + } } bool _hasWarnedAboutStorageOverride = false; @@ -446,6 +493,32 @@ } } +// Many characters are problematic in filenames, especially on Windows. +final Map<int, List<int>> _flattenNameSubstitutions = <int, List<int>>{ + r'@'.codeUnitAt(0): '@@'.codeUnits, + r'/'.codeUnitAt(0): '@s@'.codeUnits, + r'\'.codeUnitAt(0): '@bs@'.codeUnits, + r':'.codeUnitAt(0): '@c@'.codeUnits, + r'%'.codeUnitAt(0): '@per@'.codeUnits, + r'*'.codeUnitAt(0): '@ast@'.codeUnits, + r'<'.codeUnitAt(0): '@lt@'.codeUnits, + r'>'.codeUnitAt(0): '@gt@'.codeUnits, + r'"'.codeUnitAt(0): '@q@'.codeUnits, + r'|'.codeUnitAt(0): '@pip@'.codeUnits, + r'?'.codeUnitAt(0): '@ques@'.codeUnits +}; + +/// Given a name containing slashes, colons, and backslashes, expand it into +/// something that doesn't. +String _flattenName(Uri url) { + final String urlName = url.toString(); + final List<int> replacedCodeUnits = <int>[]; + for (int codeUnit in urlName.codeUnits) { + replacedCodeUnits.addAll(_flattenNameSubstitutions[codeUnit] ?? <int>[codeUnit]); + } + return new String.fromCharCodes(replacedCodeUnits); +} + /// Download a file from the given [url] and write it to [location]. Future<Null> _downloadFile(Uri url, File location) async { _ensureExists(location.parent); @@ -453,35 +526,6 @@ location.writeAsBytesSync(fileBytes, flush: true); } -/// Download a zip archive from the given [url] and unzip it to [location]. -Future<Null> _downloadZipArchive(Uri url, Directory location) { - return _withTemporaryFile('download.zip', (File tempFile) async { - await _downloadFile(url, tempFile); - _ensureExists(location); - os.unzip(tempFile, location); - }); -} - -/// Download a gzipped tarball from the given [url] and unpack it to [location]. -Future<Null> _downloadZippedTarball(Uri url, Directory location) { - return _withTemporaryFile('download.tgz', (File tempFile) async { - await _downloadFile(url, tempFile); - _ensureExists(location); - os.unpack(tempFile, location); - }); -} - -/// Create a file with the given name in a new temporary directory, invoke -/// [onTemporaryFile] with the file as argument, then delete the temporary -/// directory. -Future<Null> _withTemporaryFile(String name, Future<Null> onTemporaryFile(File file)) async { - final Directory tempDir = fs.systemTempDirectory.createTempSync(); - final File tempFile = fs.file(fs.path.join(tempDir.path, name)); - await onTemporaryFile(tempFile).whenComplete(() { - tempDir.delete(recursive: true); - }); -} - /// Create the given [directory] and parents, as necessary. void _ensureExists(Directory directory) { if (!directory.existsSync())