Skip to content
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.

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

[Windows] Add version info migration #123414

Merged
merged 6 commits into from Mar 29, 2023

Conversation

loic-sharma
Copy link
Member

@loic-sharma loic-sharma commented Mar 24, 2023

Windows apps can set version information but this requires a manual migration for projects created before Flutter 3.3. This change automates that migration.

Addresses #121724

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • 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.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@flutter-dashboard flutter-dashboard bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Mar 24, 2023
@loic-sharma loic-sharma force-pushed the windows_version_migration branch 3 times, most recently from 071d32f to a184698 Compare March 24, 2023 18:26
import '../../cmake_project.dart';

const String _cmakeFileBefore =
'# Apply the standard set of build settings. This can be removed for applications\r\n'
Copy link
Member

Choose a reason for hiding this comment

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

So we're sure this will end in \r\n because when you use git on Windows, and it checks out https://github.com/flutter/flutter/blob/master/packages/flutter_tools/templates/app_shared/windows.tmpl/CMakeLists.txt.tmpl, it will auto convert \n -> \r\n, or is there some other OS-level magic? Or are there some configurations of git that might lead to this file having only \n between lines?

Copy link
Member

Choose a reason for hiding this comment

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

Or, do we know this must have \r\n because otherwise windows cmake will explode?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question. Somehow this has \r\n line endings on my computer, let me investigate how that happens.

Copy link
Member Author

Choose a reason for hiding this comment

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

It turns out it's because I've enabled autocrlf globally on git. This converts my Flutter SDK's template files from LF to CRLF on checkout. It's likely most users have LF line endings, so we should support that as well.

Copy link
Member

Choose a reason for hiding this comment

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

I'm gonna be out for a week, however this change LGTM provided we're confident it will work no matter which settings they have in their windows git config :)


if (!_resourceFile.existsSync()) {
logger.printTrace('windows/runner/Runner.rc file not found, skipping version migration');
return;
Copy link
Member

Choose a reason for hiding this comment

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

to clarify, we only want to do any migrations if BOTH of these files exist?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup. I'd like to make this migration conservative and only run on projects that are clearly safe. If one of these files is missing, it indicates the app has done non-trivial changes to its runner.

That said, this migration should be safe even if one file is missing: each file's changes are backwards compatible and would work even if the other file's migration wasn't applied.

Copy link
Member

Choose a reason for hiding this comment

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

Is there a docs page we can point people to when this fails, so they can do it by hand if desired? If so, in addition, log something like "For more information see: URL".

Copy link
Member Author

Choose a reason for hiding this comment

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

I added some more information to the trace to make it more actionable. This failure scenario seems unlikely (it wouldn't be unreasonable for flutter build windows to break if the runner's CMakeLists.txt file was missing), so I don't think it warrants a dedicated docs page.

import '../../base/project_migrator.dart';
import '../../cmake_project.dart';

const String _cmakeFileBefore =
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be preferable to put these long hardcoded strings into their own file(s)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Prior art also hardcodes strings inside the migration itself:

const String thinBinaryBuildPhaseOriginal = '''
3B06AD1E1E4923F5004D2608 /* Thin Binary */ = {
isa = PBXShellScriptBuildPhase;
alwaysOutOfDate = 1;
buildActionMask = 2147483647;
files = (
);
inputPaths = (
);
''';
const String thinBinaryBuildPhaseReplacement = r'''
3B06AD1E1E4923F5004D2608 /* Thin Binary */ = {
isa = PBXShellScriptBuildPhase;
alwaysOutOfDate = 1;
buildActionMask = 2147483647;
files = (
);
inputPaths = (
"${TARGET_BUILD_DIR}/${INFOPLIST_PATH}",
);
''';

I can move this to another file if folks feel strongly about this.

Copy link
Member

Choose a reason for hiding this comment

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

I weakly would prefer it inlined to make it easier to grep for a string and then see the usage within the tool.

Copy link
Contributor

@yaakovschectman yaakovschectman left a comment

Choose a reason for hiding this comment

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

LGTM then

}
}

String _replaceFirstWindows(String originalContents, String before, String after) {
Copy link
Member

Choose a reason for hiding this comment

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

Even though it's private, consider adding a doc comment stating what this does. The name sounds like it might have to do with replacing window resources of some sort (if such a thing exists).

Do we already have a _replaceFirst? I'd be tempted to just name it that, since that's what it does. That it handles windows line-endings is more of an implementation detail.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated! Let me know if you have additional feedback here!

Copy link
Member

@cbracken cbracken left a comment

Choose a reason for hiding this comment

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

Looks good modulo minor nits!

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 30, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 30, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 30, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 30, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 31, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 31, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 31, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 31, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 31, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 31, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 31, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 1, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 1, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 1, 2023
exaby73 pushed a commit to NevercodeHQ/flutter that referenced this pull request Apr 17, 2023
[Windows] Add version info migration
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App tool Affects the "flutter" command-line tool. See also t: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants