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

Apply fetch tag logic to upgrade command, build ios framework. #52250

Closed
wants to merge 1 commit into from

Conversation

dnfield
Copy link
Contributor

@dnfield dnfield commented Mar 9, 2020

Applies logic from #52141 to upgrade and build ios-framework. Both of these depend on tags being correct to run properly.

This is expected to not regress benchmarks, since analyze, run, and other build commands that do not depend on tags are not affected.

/cc @zanderso @jonahwilliams @jmagman

@fluttergithubbot fluttergithubbot added the tool Affects the "flutter" command-line tool. See also t: labels. label Mar 9, 2020
@dnfield dnfield requested review from jmagman and zanderso March 9, 2020 17:37
@dnfield dnfield mentioned this pull request Mar 9, 2020
Copy link
Member

@jmagman jmagman left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -232,6 +232,7 @@ class BuildIOSFrameworkCommand extends BuildSubCommand {
void produceFlutterPodspec(BuildMode mode, Directory modeDirectory, { bool force = false }) {
final Status status = globals.logger.startProgress(' ├─Creating Flutter.podspec...', timeout: timeoutConfiguration.fastOperation);
try {
_flutterVersion.fetchTagsAndUpdate();
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 anywhere we request gitTagVersion where we wouldn't want to fetch the tags first?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. We get the flutter version for basically every command, which does this work. If we do this for every anayzer or build invocation it is a measurable performance hit.

Copy link
Member

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

Why does build-ios-framework need this?

For that matter, why does anyone that isn't using a weird developer workflow on master need this?

@jmagman
Copy link
Member

jmagman commented Mar 9, 2020

Why does build-ios-framework need this?

flutter build ios-framework --cocoapods refuses to run unless you are on dev/beta/stable so it won't generate a file pointing to unsigned binaries and won't generate a bogus -pre version.

@dnfield
Copy link
Contributor Author

dnfield commented Mar 9, 2020

There are a lot of ways to fail to pull down tags

@jonahwilliams
Copy link
Member

Presumably users on the normal upgrade flow would already have the right tags to know what version they're on?

@jonahwilliams
Copy link
Member

I think I'm misunderstanding the purpose of this change. Can we chat about the offline (online) for a few minutes?

@dnfield
Copy link
Contributor Author

dnfield commented Mar 27, 2020

before trying to land this, I'll try to fix the tag logic so it only runs on branches other than dev/beta/stable

@zanderso
Copy link
Member

zanderso commented Apr 2, 2020

Hi @dnfield what's the status of this PR?

@jmagman
Copy link
Member

jmagman commented Apr 2, 2020

Now that the fetch tag logic has been changed to only run on master, we can probably close this. build ios-framework --cocoapods doesn't even run on master, though if you leave that flag off it will. What do you think, @dnfield?

@dnfield
Copy link
Contributor Author

dnfield commented Apr 2, 2020

SGTM

@dnfield dnfield closed this Apr 2, 2020
@dnfield dnfield deleted the apply_version_to_more branch April 2, 2020 18:18
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 31, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
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

6 participants