[tool] when writing to openssl as a part of macOS/iOS code-signing, flush the stdin stream before closing it #150120
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
c870693
to
338c6b8
Compare
This comment was marked as resolved.
This comment was marked as resolved.
| stdin: opensslProcess.stdin, | ||
| line: signingCertificateStdout, | ||
| onError: (Object? error, _) { | ||
| throwToolExit('Failed to write to openssl process. Error: $error'); |
There was a problem hiding this comment.
Is this actionable by the user? We usually throw a tool exit if either the user provided invalid inputs or the state of their machine is in some bad state. This might legitimately be a bug in tool assumptions though, right?
Also, I wonder if the fact that we were synchronously closing this after we call .write() (but maybe before the write happened) is why this was crashing in the first place. If that is the case, this change should solve that.
There was a problem hiding this comment.
Is this actionable by the user? We usually throw a tool exit if either the user provided invalid inputs or the state of their machine is in some bad state. This might legitimately be a bug in tool assumptions though, right?
Great catch. I think I went on autopilot here. I've since replaced this with an Exception (code).
Also, I wonder if the fact that we were synchronously closing this after we call .write() (but maybe before the write happened) is why this was crashing in the first place. If that is the case, this change should solve that.
This is certainly possible. As you pointed out, Process.writeToStdinGuarded should protect us from this going forward.
| onError: (Object? error, _) { | ||
| throw Exception('Unexpected error when writing to openssl: $error'); | ||
| }, |
There was a problem hiding this comment.
If this ends up becoming a pattern (it very much probably will), we may want to consider creating like a writeToStdinUnsafe method that accepts something like a message or taskDescription. This new method would then throw a custom exception type from that.
packages/flutter_tools/test/general.shard/ios/code_signing_test.dart
Outdated
Show resolved
Hide resolved
| stdin.flush().then((_) { | ||
| completer.complete(); | ||
| }); |
There was a problem hiding this comment.
Not merging this yet because we probably should have a .onError call here.
d895b26
to
515a5c9
Compare
|
Heya @christopherfujino, I've reset your review status since I've made some non-trivial changes since your approval. For your convenience, here's the diff of what has changed. |
…lush the stdin stream before closing it (flutter#150120) Fixes flutter#100584. Might help flutter#137184.
…gning, flush the stdin stream before closing it (flutter/flutter#150120)
…gning, flush the stdin stream before closing it (flutter/flutter#150120)
…gning, flush the stdin stream before closing it (flutter/flutter#150120)
…gning, flush the stdin stream before closing it (flutter/flutter#150120)
flutter/flutter@651a17d...99bb2ff 2024-07-01 engine-flutter-autoroll@skia.org Roll Flutter Engine from 0d93da7e2fe1 to b57a044ed10f (1 revision) (flutter/flutter#151082) 2024-06-30 engine-flutter-autoroll@skia.org Roll Flutter Engine from ad1343cfb99e to 0d93da7e2fe1 (9 revisions) (flutter/flutter#151062) 2024-06-29 jason-simmons@users.noreply.github.com Reduce the depth used in a test that applies finders to deep widget trees (flutter/flutter#151049) 2024-06-29 engine-flutter-autoroll@skia.org Roll Flutter Engine from f828363d03ff to ad1343cfb99e (3 revisions) (flutter/flutter#151016) 2024-06-29 32538273+ValentinVignal@users.noreply.github.com Add test for segmented_button.0.dart (flutter/flutter#150676) 2024-06-29 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#151022) 2024-06-29 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#150827) 2024-06-28 engine-flutter-autoroll@skia.org Roll Flutter Engine from 2f7e9ab27493 to f828363d03ff (3 revisions) (flutter/flutter#151013) 2024-06-28 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 3.25.10 to 3.25.11 (flutter/flutter#151012) 2024-06-28 andrewrkolos@gmail.com [tool] when writing to openssl as a part of macOS/iOS code-signing, flush the stdin stream before closing it (flutter/flutter#150120) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC camillesimon@google.com,rmistry@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…lush the stdin stream before closing it (flutter#150120) Fixes flutter#100584. Might help flutter#137184.
…gning, flush the stdin stream before closing it (flutter/flutter#150120)
Fixes #100584. Might help #137184.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.