Skip to content

Use ErrorHandlingFileSystem.deleteIfExists when deleting .plugin_symlinks #151073

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Jul 2, 2024
Merged

Use ErrorHandlingFileSystem.deleteIfExists when deleting .plugin_symlinks #151073

merged 4 commits into from
Jul 2, 2024

Conversation

andrewkolos
Copy link
Contributor

Fixes #137168.

Pre-launch Checklist

@andrewkolos andrewkolos added the tool Affects the "flutter" command-line tool. See also t: labels. label Jun 30, 2024
@andrewkolos andrewkolos changed the title Fix fse from deleting symlinks Use ErrorHandlingFileSystem.deleteIfExists when deleting .plugin_symlinks Jun 30, 2024
@andrewkolos andrewkolos marked this pull request as ready for review July 1, 2024 21:15
andrewkolos
andrewkolos commented Jul 1, 2024
View reviewed changes
packages/flutter_tools/test/general.shard/plugins_test.dart
@@ -1752,6 +1754,73 @@ The Flutter Preview device does not support the following plugins from your pubs
);
});
});

testUsingContext('exits tool when deleting .plugin_symlinks fails', () async {
Copy link
Contributor Author

@andrewkolos andrewkolos Jul 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm somewhat concerned about having tests like these. The existence of this test implies that every single file operation throughout the entire tool should have one or more tests for possibility of the operation failing. Do tests like these add more value than they produce noise?

More pointedly, I only wrote this test to satisfy our testing requirement, not because I think it's a good test to have.

Copy link
Contributor

Choose a reason for hiding this comment

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

The existence of this test implies that every single file operation throughout the entire tool should have one or more tests for possibility of the operation failing. Do tests like these add more value than they produce noise?

I think there's a difference between testing every single operation to verify it behaves as expected and adding a test for a specific operation that was known to be problematic to verify someone doesn't undo it.

That having been said, I don't care that much, and your cache_test update would have made the bot happy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think there's a difference between testing every single operation to verify it behaves as expected and adding a test for a specific operation that was known to be problematic to verify someone doesn't undo it.

I suppose.

🤷 I do think the test was worth writing just to verify the change worked, and I therefore think it's useful for future archeologists to verify that the change was tested before being landed. So I think I'll keep writing these.

christopherfujino
christopherfujino reviewed Jul 1, 2024
View reviewed changes
packages/flutter_tools/lib/src/base/error_handling_io.dart Outdated
"unable to. This may be due to the file and/or project's location on a read-only "
'volume. Consider relocating the project and trying again',
'Unable to delete file or directory at "${entity.path}". '
'This may be due it and/or the project being in a read-only '
Copy link
Contributor

Choose a reason for hiding this comment

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

s/due it and/or the project/due to the file and/or project

christopherfujino
christopherfujino approved these changes Jul 1, 2024
View reviewed changes
Copy link
Contributor

@christopherfujino christopherfujino left a comment

Choose a reason for hiding this comment

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

nit about awkward grammar in the toolexit, but otherwise LGTM

christopherfujino
christopherfujino reviewed Jul 1, 2024
View reviewed changes
packages/flutter_tools/test/general.shard/plugins_test.dart
@@ -3,10 +3,12 @@
// found in the LICENSE file.

import 'dart:convert';
import 'dart:io';
Copy link
Contributor

Choose a reason for hiding this comment

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

nit. do you really need to import dart:io here, or would package:flutter_tools/src/base/io.dart be sufficient?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried to fix this locally, but it appears io.dart doesn't expose PathNotFoundException?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh, ok, this LGTM then

@andrewkolos andrewkolos added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 2, 2024
@auto-submit auto-submit bot merged commit af913a7 into flutter:master Jul 2, 2024
129 checks passed
@andrewkolos andrewkolos added the cp: stable cherry pick this pull request to stable release candidate branch label Jul 2, 2024
flutteractionsbot pushed a commit to flutteractionsbot/flutter that referenced this pull request Jul 2, 2024
@andrewkolos
Use ErrorHandlingFileSystem.deleteIfExists when deleting .plugin_sy…
d98c9f5 
…mlinks (flutter#151073)

Fixes flutter#137168.
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jul 2, 2024
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 2, 2024
@engine-flutter-autoroll
af913a7 Use ErrorHandlingFileSystem.deleteIfExists when deleting .p…
ac7c1b9 
…lugin_symlinks (flutter/flutter#151073)
hello-coder-xu added a commit to hello-coder-xu/flutter that referenced this pull request Jul 3, 2024
@hello-coder-xu
Merge branch 'master' into fix/pageController_get_value_error
8798d1a 
* master: (88 commits)
  Fix scheduler event loop being stuck due to task with Priority.idle (flutter#151168)
  Fix result propagation in RenderSliverEdgeInsetsPadding.hitTestChildren (flutter#149825)
  docImports for flutter_test (flutter#151189)
  Interactable ScrollView content when settling a scroll activity (flutter#145848)
  [flutter_tools] Update the mapping for the Dart SDK internal URI (flutter#151170)
  Roll pub packages (flutter#151129)
  Fix typo (flutter#151192)
  [tool] Fix `stdin.flush` calls on processes started by `FakeProcessManager` (flutter#151183)
  Roll Flutter Engine from 433d360eee11 to 44278941443e (4 revisions) (flutter#151186)
  Use `ErrorHandlingFileSystem.deleteIfExists` when deleting .plugin_symlinks (flutter#151073)
  ScrollEndNotification example: auto-scroll based on RenderSliver constraints and geometry (flutter#143538)
  Roll Packages from 412ec46 to d2705fb (13 revisions) (flutter#151169)
  docimports for painting (flutter#151143)
  docimports for scheduler (flutter#151126)
  `dismissible.dart` code cleanup (flutter#150276)
  docimports for physics (flutter#151125)
  docimports for services (flutter#151134)
  docimports for cupertino (flutter#151149)
  docimports for gestures (flutter#151123)
  Docimports for foundation (flutter#151119)
  ...
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Jul 3, 2024
@engine-flutter-autoroll
Roll Flutter from 99bb2ff to af913a7 (30 revisions) (#7047)
ddc2caa 
flutter/flutter@99bb2ff...af913a7

2024-07-02 [email protected] Use `ErrorHandlingFileSystem.deleteIfExists` when deleting .plugin_symlinks (flutter/flutter#151073)
2024-07-02 [email protected] ScrollEndNotification example: auto-scroll based on RenderSliver constraints and geometry (flutter/flutter#143538)
2024-07-02 [email protected] Roll Packages from 412ec46 to d2705fb (13 revisions) (flutter/flutter#151169)
2024-07-02 [email protected] docimports for painting (flutter/flutter#151143)
2024-07-02 [email protected] docimports for scheduler (flutter/flutter#151126)
2024-07-02 [email protected] `dismissible.dart` code cleanup (flutter/flutter#150276)
2024-07-02 [email protected] docimports for physics (flutter/flutter#151125)
2024-07-02 [email protected] docimports for services (flutter/flutter#151134)
2024-07-02 [email protected] docimports for cupertino (flutter/flutter#151149)
2024-07-02 [email protected] docimports for gestures (flutter/flutter#151123)
2024-07-02 [email protected] Docimports for foundation (flutter/flutter#151119)
2024-07-02 [email protected] docimports for semantics (flutter/flutter#151132)
2024-07-02 [email protected] [flutter_driver] add allocator mtl to memory event allowlist. (flutter/flutter#151153)
2024-07-02 [email protected] Roll Flutter Engine from 40c087b31515 to 433d360eee11 (7 revisions) (flutter/flutter#151165)
2024-07-02 [email protected] Refactor BuildInfo to always require packageConfigPath (flutter/flutter#150559)
2024-07-02 [email protected] Roll Flutter Engine from d3c5bd66a78f to 40c087b31515 (1 revision) (flutter/flutter#151156)
2024-07-02 [email protected] Roll Flutter Engine from fc5bc14e6091 to d3c5bd66a78f (1 revision) (flutter/flutter#151155)
2024-07-02 [email protected] Fix: `CupertinoActionSheet` should take up max height when actions section is short (flutter/flutter#150708)
2024-07-02 [email protected] Roll Flutter Engine from 3456fee1a6b9 to fc5bc14e6091 (8 revisions) (flutter/flutter#151150)
2024-07-02 [email protected] [tool] remove some temporary printTrace calls (flutter/flutter#151074)
2024-07-01 [email protected] Implementing a few switch statements (flutter/flutter#150946)
2024-07-01 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Upgrade template Gradle, App AGP, Module AGP, and Kotlin versions, and tests (#150969)" (flutter/flutter#151147)
2024-07-01 [email protected] Upgrade template Gradle, App AGP, Module AGP, and Kotlin versions, and tests (flutter/flutter#150969)
2024-07-01 [email protected] Roll Flutter Engine from b57a044ed10f to 3456fee1a6b9 (5 revisions) (flutter/flutter#151127)
2024-07-01 [email protected] Read AndroidManifest.xml and emit manifest-aar-impeller-(enabled|disabled) analytics (flutter/flutter#150970)
2024-07-01 [email protected] More docimports for animation library (flutter/flutter#151011)
2024-07-01 [email protected] Bump dartdoc to 8.0.10 (flutter/flutter#151107)
2024-07-01 [email protected] Fix missing `[` in docs (flutter/flutter#151091)
2024-07-01 [email protected] Roll pub packages (flutter/flutter#151028)
2024-07-01 [email protected] Fix teardown of a FocusScopeNode in material/app_test (flutter/flutter#151115)

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 [email protected],[email protected],[email protected] 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
TahaTesser pushed a commit to TahaTesser/flutter that referenced this pull request Jul 8, 2024
@andrewkolos @TahaTesser
Use ErrorHandlingFileSystem.deleteIfExists when deleting .plugin_sy…
bd7392c 
…mlinks (flutter#151073)

Fixes flutter#137168.
victorsanni pushed a commit to victorsanni/flutter that referenced this pull request Jul 8, 2024
@andrewkolos @victorsanni
Use ErrorHandlingFileSystem.deleteIfExists when deleting .plugin_sy…
557064e 
…mlinks (flutter#151073)

Fixes flutter#137168.
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 6, 2024
@engine-flutter-autoroll
af913a7 Use ErrorHandlingFileSystem.deleteIfExists when deleting .p…
ec91f33 
…lugin_symlinks (flutter/flutter#151073)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@christopherfujino christopherfujino christopherfujino approved these changes

Assignees
No one assigned
Labels
autosubmit Merge PR when tree becomes green via auto submit App cp: stable cherry pick this pull request to stable release candidate branch tool Affects the "flutter" command-line tool. See also t: labels.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[flutter_tools] PathNotFoundException on delete operation during flutter pub get
2 participants
@andrewkolos @christopherfujino