flutter-intellij icon indicating copy to clipboard operation
flutter-intellij copied to clipboard

🎨 Use `tools_metadata` as a submodule in `resources/flutter`

Open AlexV525 opened this issue 2 years ago • 8 comments

https://github.com/flutter/tools_metadata is now configured as a submodule under resources/flutter instead of copy it manually.

The PR is huge because it deletes copied files. Be careful when reviewing the "Files changed" tab. 😃

Pre-launch Checklist

  • [x] I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • [x] I read the Tree Hygiene wiki page, which explains my responsibilities.
  • [x] I read the Flutter Style Guide recently, and have followed its advice.
  • [x] I signed the CLA.
  • [ ] I listed at least one issue that this PR fixes in the description above.
  • [ ] I updated/added relevant documentation (doc comments with ///).
  • [ ] I added new tests to check the change I am making, or this PR is test-exempt.
  • [x] All existing and new tests are passing.

AlexV525 avatar Aug 16 '23 15:08 AlexV525

@helin24 Could you help me verify the issue of the Kokoro build? I cannot access it since it looks like an internal infrastructure.

AlexV525 avatar Aug 17 '23 06:08 AlexV525

Updating docs references since flutter.dev just went down for a few minutes and exposed the issue.

AlexV525 avatar Aug 26 '23 07:08 AlexV525

cc @jwren @helin24 could you review this?

AlexV525 avatar Nov 01 '23 10:11 AlexV525

Also @DanTup

AlexV525 avatar Nov 01 '23 10:11 AlexV525

I don't know enough about this repo to review it so I'll defer to Jaime/Helin. The idea seems reasonable to me.

One question I'd have is about how this impacts the production builds of the plugin - will it automatically include these files in their new location (or is there some list somewhere of what gets included in the shipped plugin)? If it's automatically all included, will it include the whole tools_metadata repo (including things like the tool folder)? It's probably not a big deal (since I think it's just some additional text files), but perhaps something to be aware of.

DanTup avatar Nov 01 '23 10:11 DanTup

One question I'd have is about how this impacts the production builds of the plugin - will it automatically include these files in their new location (or is there some list somewhere of what gets included in the shipped plugin)? If it's automatically all included, will it include the whole tools_metadata repo (including things like the tool folder)?

Previous After
tools_metadata/resources tools_metadata

The resources/flutter will include the entire tools_metadata starting from this change. I didn't dig into whether irrelevant files (resources/flutter/!resources) could affect production builds though.

AlexV525 avatar Nov 01 '23 14:11 AlexV525

Thanks @AlexV525 for this PR. With less individuals working on the Flutter Plugin, and with many recent issues with the Flutter IJ build scripts, I have not had the time to verify that these changes won't have any negative consequences with the Gradle scripts, or other scripts we have that help maintain the Flutter Plugin for IntelliJ.

Leaving this PR open for the time being, thank you again for putting it together.

jwren avatar Jan 17 '24 00:01 jwren

@jwren Thanks!

With less individuals working on the Flutter Plugin, and with many recent issues with the Flutter IJ build scripts, I have not had the time to verify that these changes won't have any negative consequences with the Gradle scripts, or other scripts we have that help maintain the Flutter Plugin for IntelliJ.

I'd willing to help to verify if it works well if you can somehow provide any suggestions, and I'm fine to wait until you can figure out all fixes to build scripts. You can let me know once they are good to go.

AlexV525 avatar Jan 17 '24 01:01 AlexV525

Thank you for creating this PR, but for the time being I am closing it. PRs can't remain open for this long against the Flutter GitHub repo and with other concerns around the Flutter Plugin for IntelliJ I won't have the cycles to properly review and land this change.

jwren avatar Mar 27 '24 18:03 jwren