Repository navigation
fix: stop flutter create --platforms from dropping existing platfor… - #191573
auto-submit[bot] merged 13 commits into
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Code Review
This pull request updates the flutter create command to seed the migration configuration with platforms already tracked in an existing .metadata file, preventing previously added platforms from being dropped when re-running the command. It also adds a test to verify this behavior. The review feedback suggests wrapping the .metadata parsing logic in a try-catch block to handle potentially corrupted or malformed files gracefully, preventing the tool from crashing.
bkonyi
left a comment
There was a problem hiding this comment.
Thanks for the PR! Just a couple of comments.
|
Thanks for the feedback! I will batch these fixes together in the next commit. |
|
It's ready for review! Please take a look when you have a chance, thanks! |
| // existing .metadata file. Re-running `flutter create --platforms` must | ||
| // add the newly requested platforms without dropping the previously | ||
| // added ones, and without overwriting their recorded revisions. | ||
| // See https://github.com/flutter/flutter/issues/191567. |
There was a problem hiding this comment.
I think that comment is a bit confusing.
There is also this use case:
- One adds a platform / multiple platforms, let's say:
--platforms=android,ios - One recreates specific platform folders, to start from scratch (this was also common in packages repo to migrate to newer project standards, as not everything is migrated and/or additionally adds a new platform:
--platforms=ios,linux
So I would recommend somethinng like:
// ... Re-running `flutter create --platforms` must
// add the platform without dropping or overwriting the previously
// added ones, if they didn't exist yet.
Can we test the second case, too? So an entry for ios won't get duplicated (appended)?
7060e71 to
fb40abe
Compare
|
Ah, sorry about that! I accidentally included .vscode/settings.json in the PR as I've been quite busy lately. I'll remove it shortly.
|
|
Hi @bkonyi, Sorry for the ping! The PR has been waiting for the "Flutter Roll on Borg / Google testing" approval for over 12 hours now. I've fully fixed my local environment issues and successfully ran the entire suite. The command This fully validates the idempotency fix—ensuring that running Since this fix was delegated by you, could you please take a quick moment to check Frob and click the "Run tests now" button to unblock the internal Borg CI queue? Thank you so much for your time and guidance! 🚀 |
|
@Shawn-Yu-Dev you should always check, what you have actually commited and verify your results, e.g. here in Github diff. There are now many files, which don't belong in a PR at all. See |
|
Ah, I keep overlooking this. I’ve already discarded the changes to those files. |
|
@bkonyi Thanks for the review! I'm currently a student, so I'm mostly tied up with coursework during the week and only have time to work on this on weekends. I'll try to get to it this coming weekend. Appreciate your patience!
|
3b2bba7 to
511d4f6
Compare
|
@bkonyi Thanks for the review! I've updated the PR to address this feedback. Please take another look. |
|
autosubmit label was removed for flutter/flutter/191573, because - The status or check suite Tree_analyze has failed. Please fix the issues identified (or deflake) before re-applying this label.
|
…begin-pasted-text-1-37-lines-flut/state/directions_tried.json
…begin-pasted-text-1-37-lines-flut/state/findings.jsonl
…begin-pasted-text-1-37-lines-flut/state/progress.json
…begin-pasted-text-1-37-lines-flut/state/task_spec.json
…begin-pasted-text-1-37-lines-flut/state/iteration_log.jsonl
… create --platforms
0ec4485 to
4325291
Compare
|
autosubmit label was removed for flutter/flutter/191573, because - The status or check suite Dashboard Checks has failed. Please fix the issues identified (or deflake) before re-applying this label. |
…ms in .metadata
Fixes an issue where running
flutter create --platforms=<new-platform> .overwrites the existing platform list in.metadatainstead of appending to it.Fixes #191567
Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.
List which issues are fixed by this PR. You must list at least one issue. An issue is not required if the PR fixes something trivial like a typo.
If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.