refactor depfile usage and update linux rule#42487
refactor depfile usage and update linux rule#42487jonahwilliams merged 16 commits intoflutter:masterfrom
Conversation
fluttergithubbot
added
the
tool
Codecov Report
@@ Coverage Diff @@
## master #42487 +/- ##
==========================================
- Coverage 60.75% 59.64% -1.12%
==========================================
Files 194 195 +1
Lines 18885 18905 +20
==========================================
- Hits 11474 11276 -198
- Misses 7411 7629 +218
Continue to review full report at Codecov.
|
| if (assets == null) { | ||
| throwToolExit('Error building assets', exitCode: 1); | ||
| // Work around for flutter_tester placing kernel artifacts in odd places. | ||
| if (applicationKernelFilePath != null) { |
There was a problem hiding this comment.
flutter_tester is currently the only use of this logic. In the long term, we should clean this up to use a tempDir or something similar.
There was a problem hiding this comment.
Is there an issue open for this?
|
This change will also allow us to output a depfile for dart2js, which should cut down on memory usage. |
| environment.projectDir.path, | ||
| 'linux', | ||
| 'flutter', | ||
| 'ephemeral', |
There was a problem hiding this comment.
Can we stick
s.path.join(
environment.projectDir.path,
'linux',
'flutter',
'ephemeral')into a local?
| fs.file(input).copySync(destinationFile.path); | ||
| } | ||
| final Depfile depfile = Depfile(inputs, outputs); | ||
| depfile.writeToFile(environment.buildDir.childFile('linux_engine_sources.d')); |
There was a problem hiding this comment.
Does it matter if the dep file is re-written on every build, even if nothing changes?
There was a problem hiding this comment.
No, we don't stat/diff the bytes of the depfile at all, we only read the contents. Changing the order/rewriting shouldn't trigger rebuilds.
| /// Parse the depfile contents from [file]. | ||
| /// | ||
| /// If the syntax is invalid, returns an empty [Depfile]. | ||
| factory Depfile.parse(File file) { |
There was a problem hiding this comment.
It's tested indirectly via integration tests, but I've added some unit tests to cover it
jonahwilliams
requested review from
blasten,
dnfield,
jmagman and
zanderso
and removed request for
dnfield and
zanderso
| } | ||
|
|
||
| // If we depend on a file that doesnt exist on disk, kill the build. | ||
| // If we depend on a file that doesnt exist on disk, mark the build as |
There was a problem hiding this comment.
What happens if the user listed a file by accident, and the fix is to remove the file? How do they find out?
There was a problem hiding this comment.
The user here would be the tools team, so ideally we'd have a test to cover it. Long term, I think we should surface the reasons why targets are re-run.
There was a problem hiding this comment.
Would it be useful to do a printTrace here with some diagnostic info?
There was a problem hiding this comment.
From the description, what is an example of a fairly dynamic rule where a required file doesn't exist at the moment of running the target?
There was a problem hiding this comment.
Anything using a depfile, which will be lots of rules
| if (missingInputs.isNotEmpty) { | ||
| throw MissingInputException(missingInputs, target.name); | ||
| _dirty = true; | ||
| invalidatedReasons.add(InvalidedReason.inputChanged); |
There was a problem hiding this comment.
does inputChanged mean that a required file is missing? The comment on this file says: An input file has an updated hash..
There was a problem hiding this comment.
Added InvalidatedReason.inputMissing with a description
…lutter into match_unpack_directories
| final List<String> colonSeparated = contents.split(': '); | ||
| if (colonSeparated.length != 2) { | ||
| printError('Invalid depfile: ${file.path}'); | ||
| return const Depfile(<File>[], <File>[]); |
There was a problem hiding this comment.
In which instance returning an empty Depfile makes the build successful later?
There was a problem hiding this comment.
In the case where (for some reason, human intervention) an invalid depfile is written or edited - we don't force the user to flutter clean, instead we allow a new one be written which hopefully doesn't have the same issue
| // Expand escape sequences, so that '\ ', for example,ß becomes ' ' | ||
| .map<String>((String path) => path.replaceAllMapped(_escapeExpr, (Match match) => match.group(1)).trim()) | ||
| .where((String path) => path.isNotEmpty) | ||
| .toSet() |
There was a problem hiding this comment.
are there duplicated items in the list? If so, would you mind added a comment?
There was a problem hiding this comment.
Also added a test case for this
| final String outputPath = fs.path.join( | ||
| outputPrefix, | ||
| fs.path.relative(input.path, from: basePath), | ||
| ); | ||
| final File destinationFile = fs.file(outputPath); | ||
| if (!destinationFile.parent.existsSync()) { | ||
| destinationFile.parent.createSync(recursive: true); | ||
| } | ||
| final File inputFile = fs.file(input); | ||
| inputFile.copySync(destinationFile.path); | ||
| inputs.add(inputFile); | ||
| outputs.add(destinationFile); |
| if (fs.isFileSync(entityPath)) { | ||
| final String outputPath = fs.path.join( | ||
| outputPrefix, | ||
| fs.path.relative(entityPath, from: basePath), | ||
| ); | ||
| final File destinationFile = fs.file(outputPath); | ||
| if (!destinationFile.parent.existsSync()) { | ||
| destinationFile.parent.createSync(recursive: true); | ||
| } | ||
| final File inputFile = fs.file(entityPath); | ||
| inputFile.copySync(destinationFile.path); | ||
| inputs.add(inputFile); | ||
| outputs.add(destinationFile); | ||
| continue; |
There was a problem hiding this comment.
nit: this code might need some comments.
There was a problem hiding this comment.
I tried to add some more context here, PTAL
| if (assets == null) { | ||
| throwToolExit('Error building assets', exitCode: 1); | ||
| // Work around for flutter_tester placing kernel artifacts in odd places. | ||
| if (applicationKernelFilePath != null) { |
There was a problem hiding this comment.
Is there an issue open for this?
| import '../../src/common.dart'; | ||
| import '../../src/testbed.dart'; | ||
|
|
||
| void main() { |
There was a problem hiding this comment.
Some negative tests here would be good.
There was a problem hiding this comment.
Added test cases for malformed file, duplicates, and whitespace
|
Filled #42957 for the tester issue |
| // Put every file on right-hand side on the separate line | ||
| .replaceAllMapped(_separatorExpr, (Match match) => '${match.group(1)}\n') | ||
| .split('\n') | ||
| // Expand escape sequences, so that '\ ', for example,ß becomes ' ' |
| outputs.add(destinationFile); | ||
| continue; | ||
| } | ||
| // If ths artifact is the directory cpp_client_wrapper, recursively |
|
I have now this error: |
7 participants
Description
Pulls the logic for depfile reading and writing into a single class. Uses it to make the linux unpack command smarter.
Updates BuildSystem to not consider missing inputs a fatal error, instead it will rerun as normal. This allows easier usage of depfiles for fairly dynamic rules.
Remove dead code from build bundle.
Fixes #42411
Fixes #42399
Fixes #42697
Fixes #42411
Fixes #42954