[pigeon] Recursively create output target files #4458

Nitin-Poojary · 2023-07-13T15:57:07Z

Recursively creates output target files before trying to write to it.

Pre-launch Checklist

tarrinneal · 2023-07-13T22:45:48Z

@stuartmorgan do you think this is something that can be reasonably tested?

stuartmorgan-g · 2023-07-14T11:45:49Z

I don't see why not; we have lots of tests that generate output files. All we need is one that does so with a target directory that doesn't exist.

Nitin-Poojary · 2023-07-14T13:00:43Z

I'm sorry, but I didn't quite understand your point. Could you please clarify what I need to do?

tarrinneal · 2023-07-14T21:06:04Z

I'm sorry, but I didn't quite understand your point. Could you please clarify what I need to do?

Can you add a test in the pigeon_lib_test.dart file that tests writing a file in a directory that doesn't exist?

Nitin-Poojary · 2023-07-18T08:13:12Z

I apologize, but I am having difficulty understanding the approach required to write this test. Could you please provide guidance or instructions on how to proceed?

Nitin-Poojary · 2023-07-20T13:37:34Z

I apologize, but I am having difficulty understanding the approach required to write this test. Could you please provide guidance or instructions on how to proceed?

@stuartmorgan

stuartmorgan-g

I apologize, but I am having difficulty understanding the approach required to write this test. Could you please provide guidance or instructions on how to proceed?

To test as a unit test, you would create a temp directory (see the logic in withTempFile for an example) and then call shouldGenerate on a generator with options that would write to a non-existent subdirectory of that temp directory, then check that it was created.

Alternately it could be done as an end-to-end test here by using an output path that doesn't exist.

packages/pigeon/lib/pigeon_lib.dart

flutter-dashboard · 2023-07-24T15:34:33Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

tarrinneal · 2023-07-31T15:04:25Z

packages/pigeon/tool/shared/test_suites.dart

+      '--input',
+      'pigeons/message.dart',
+      '--dart_out',
+      '$tempDir/subdirectory/message.g.dart',


could you change this to '$tempDir/subdirectory/does/not/exist/message.g.dart'

Okay, thanks I just wanted to confirm whether I have to do something like this.

…xist

…m main branch

stuartmorgan-g

LGTM with nits.

stuartmorgan-g · 2023-08-07T17:04:39Z

packages/pigeon/CHANGELOG.md

@@ -1,3 +1,7 @@
+## 10.1.6
+
+* Creates target files before trying to write to it.


How about "Fixes generation failures when an output file is in a directory that doesn't already exist."

stuartmorgan-g · 2023-08-07T17:06:04Z

packages/pigeon/lib/pigeon_lib.dart

@@ -472,6 +473,7 @@ class DartGeneratorAdapter implements GeneratorAdapter {
    final DartOptions dartOptionsWithHeader = _dartOptionsWithCopyrightHeader(
      options.dartOptions,
      options.copyrightHeader,
+      dartOutPath: options.dartOut,


How is this change related to the fix?

I made this change to investigate something, but then I forgot to revert it. Later, I mistakenly assumed that I had made the change intentionally, and I apologize for that.

stuartmorgan-g · 2023-08-07T17:06:16Z

packages/pigeon/tool/shared/test_suites.dart

@@ -366,6 +366,13 @@ Future<int> _runCommandLineTests() async {
      '--ast_out',
      tempOutput
    ],
+    // Test writing a file in a directory that doesn't exist


Nit: missing final period.

tarrinneal

Thanks for putting this together!

Nitin-Poojary · 2023-08-08T06:35:58Z

@tarrinneal @stuartmorgan I wanted to say a big thank you for your support and guidance during the review and merging of my pull request. Your help has been invaluable, and I'm grateful for the opportunity to contribute to this project.

…ages#4458)

flutter/packages@d7ee75a...ac41376 2023-08-08 engine-flutter-autoroll@skia.org Roll Flutter from ad0aa8d to 436df69 (17 revisions) (flutter/packages#4663) 2023-08-08 10687576+bparrishMines@users.noreply.github.com [webview_flutter_wkwebview] Repeatedly pump WebViews until one is garbage collected (flutter/packages#4662) 2023-08-08 erikgerman917@gmail.com [xdg_directories] Add example app (flutter/packages#4554) 2023-08-08 70025277+Nitin-Poojary@users.noreply.github.com [pigeon] Recursively create output target files (flutter/packages#4458) 2023-08-07 jpnurmi@gmail.com [path_provider] Add getApplicationCachePath() (flutter/packages#4483) 2023-08-07 10687576+bparrishMines@users.noreply.github.com [flutter_markdown] Adopt code excerpts in README (flutter/packages#4656) 2023-08-07 reidbaker@google.com [All] Expand artifact hub to all plugins (flutter/packages#4645) 2023-08-07 engine-flutter-autoroll@skia.org Roll Flutter from 2ba9f7b to ad0aa8d (31 revisions) (flutter/packages#4659) 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-flutter-autoroll Please CC flutter-ecosystem@google.com,rmistry@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

tarrinneal · 2023-08-08T22:27:10Z

@tarrinneal @stuartmorgan I wanted to say a big thank you for your support and guidance during the review and merging of my pull request. Your help has been invaluable, and I'm grateful for the opportunity to contribute to this project.

We're always happy to help community members land good pr's! Thank you for putting in the effort and time to get this through to the finish line!

github-actions bot added the p: pigeon label Jul 13, 2023

Nitin-Poojary force-pushed the pigeon/create_target_files_recursively branch from f0ef9ee to c8ae34c Compare July 13, 2023 16:21

Nitin-Poojary marked this pull request as ready for review July 13, 2023 16:36

Nitin-Poojary requested a review from tarrinneal as a code owner July 13, 2023 16:36

tarrinneal requested a review from stuartmorgan-g July 13, 2023 18:25

Nitin-Poojary force-pushed the pigeon/create_target_files_recursively branch from c8ae34c to e666bb2 Compare July 14, 2023 14:53

Nitin-Poojary force-pushed the pigeon/create_target_files_recursively branch 3 times, most recently from 49d3fd2 to fb157be Compare July 19, 2023 13:50

Nitin-Poojary force-pushed the pigeon/create_target_files_recursively branch from fb157be to 1fe6fac Compare July 20, 2023 13:56

stuartmorgan-g requested changes Jul 20, 2023

View reviewed changes

packages/pigeon/lib/pigeon_lib.dart Outdated Show resolved Hide resolved

Nitin-Poojary force-pushed the pigeon/create_target_files_recursively branch 3 times, most recently from c2e3be0 to 1d46423 Compare July 24, 2023 15:28

Nitin-Poojary force-pushed the pigeon/create_target_files_recursively branch 8 times, most recently from b5479c5 to 7cd5a2e Compare July 31, 2023 12:54

tarrinneal requested changes Jul 31, 2023

View reviewed changes

Nitin-Poojary force-pushed the pigeon/create_target_files_recursively branch 3 times, most recently from 1fba6d5 to 215531c Compare August 5, 2023 07:24

Nitin-Poojary added 7 commits August 7, 2023 19:18

create files before writing in it

2611c6e

removed unnecessary awaits

f90f49f

fixed error in test file

e51dbe3

reverted all functions signatures and used createSync() function instead

7464301

added a test to check writing to a file in a directory that doesn't e…

c659eac

…xist

added path to directory that doesn't exists in end-to-end test

fc9f638

updated the version in alignment with the changes made to the upstrea…

2cb30a0

…m main branch

Nitin-Poojary force-pushed the pigeon/create_target_files_recursively branch from 215531c to 2cb30a0 Compare August 7, 2023 13:49

stuartmorgan-g approved these changes Aug 7, 2023

View reviewed changes

Nitin-Poojary added 4 commits August 7, 2023 23:01

revised version description

88ed03c

removed unnecessary changes

83972c9

added missing period

56a5fec

fixed changelog.md version description

9da076c

tarrinneal approved these changes Aug 8, 2023

View reviewed changes

tarrinneal added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 8, 2023

auto-submit bot merged commit 7bfcc0e into flutter:main Aug 8, 2023

engine-flutter-autoroll mentioned this pull request Aug 8, 2023

Roll Packages from d7ee75ad59ad to 269118d2284e (7 revisions) flutter/flutter#132131

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 8, 2023

7bfcc0e [pigeon] Recursively create output target files (flutter/pack…

15a9221

…ages#4458)

engine-flutter-autoroll mentioned this pull request Aug 8, 2023

Roll Packages from d7ee75ad59ad to ac4137624a13 (8 revisions) flutter/flutter#132133

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 8, 2023

7bfcc0e [pigeon] Recursively create output target files (flutter/pack…

493be8d

…ages#4458)

ValentinVignal mentioned this pull request Sep 10, 2023

[pigeon] Recursively create output target files flutter/flutter#128820

Closed

		@@ -1,3 +1,7 @@
		## 10.1.6

		* Creates target files before trying to write to it.

[pigeon] Recursively create output target files #4458

[pigeon] Recursively create output target files #4458

Uh oh!

Conversation

Nitin-Poojary commented Jul 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

tarrinneal commented Jul 13, 2023

Uh oh!

stuartmorgan-g commented Jul 14, 2023

Uh oh!

Nitin-Poojary commented Jul 14, 2023

Uh oh!

tarrinneal commented Jul 14, 2023

Uh oh!

Nitin-Poojary commented Jul 18, 2023

Uh oh!

Nitin-Poojary commented Jul 20, 2023

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

flutter-dashboard bot commented Jul 24, 2023

Uh oh!

tarrinneal Jul 31, 2023

Choose a reason for hiding this comment

Uh oh!

Nitin-Poojary Jul 31, 2023

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g Aug 7, 2023

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g Aug 7, 2023

Choose a reason for hiding this comment

Uh oh!

Nitin-Poojary Aug 7, 2023

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g Aug 7, 2023

Choose a reason for hiding this comment

Uh oh!

tarrinneal left a comment

Choose a reason for hiding this comment

Uh oh!

Nitin-Poojary commented Aug 8, 2023

Uh oh!

tarrinneal commented Aug 8, 2023

Uh oh!

Uh oh!

Nitin-Poojary commented Jul 13, 2023 •

edited

Loading