Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@jmagman
Copy link
Member

@jmagman jmagman commented Nov 13, 2020

Description

Create Flutter.xcframework in the out/ios_* directories. It only contains the Flutter.framework in the same out directory, and will allow us to start testing XCFrameworks in the tool with --local-engine flags.

This doesn't publish Flutter.xcframework as a downloadable engine artifact.

Related Issues

Local engine support part of flutter/flutter#60109, which is a dependency to support macOS ARM simulators flutter/flutter#64502

Tests

Adopted the generated Flutter.xcframework in the Scenario app to prove it's working.

Checklist

  • I read the [contributor guide] and followed the process outlined there for submitting PRs.
  • I signed the [CLA].
  • I read and followed the [C++, Objective-C, Java style guides] for the engine.
  • I read the [tree hygiene] wiki page, which explains my responsibilities.
  • I updated/added relevant documentation.
  • All existing and new tests are passing.
  • I am willing to follow-up on review comments in a timely manner.

Reviewer Checklist

  • I have submitted any presubmit flakes in this PR using the [engine presubmit flakes form] before re-triggering the failure.

Breaking Change

Did any tests fail when you ran them? Please read [handling breaking changes].

  • No, no existing tests failed, so this is not a breaking change.
  • Yes, this is a breaking change. If not, delete the remainder of this section.

@jmagman jmagman self-assigned this Nov 13, 2020
@google-cla google-cla bot added the cla: yes label Nov 13, 2020
@jmagman jmagman marked this pull request as ready for review November 13, 2020 22:02
Copy link
Member

@gaaclarke gaaclarke left a comment

Choose a reason for hiding this comment

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

LGTM I just had a few python nits, also I'd just make sure that --help works correctly. It would be nice to have a very simple test for create_xcframework.py

# Copyright 2013 The Flutter Authors. All rights reserved.
# Use of this source code is governed by a BSD-style license that can be
# found in the LICENSE file.

Copy link
Member

Choose a reason for hiding this comment

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

nit: google python style guide requires some future imports:
https://google.github.io/styleguide/pyguide.html#from-__future__-imports

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure what the minimum python version we support (I have 2.7.16 on my machine), I ran it through futurize.

from __future__ import absolute_import
from __future__ import division
from __future__ import print_function

import argparse
import errno
import os
import shutil
import subprocess
import sys

This matches the other places we use from __future__ import

from __future__ import absolute_import
from __future__ import division
from __future__ import print_function
import subprocess
import sys
import git_revision
import os

"""Get the Git HEAD revision of a specified Git repository."""
from __future__ import absolute_import
from __future__ import division
from __future__ import print_function
import sys
import subprocess
import os
import argparse

output_dir = os.path.abspath(args.location)
output_xcframework = os.path.join(output_dir, '%s.xcframework' % args.name)

try:
Copy link
Member

Choose a reason for hiding this comment

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

nit: if !os.path.exists(output_dir)

'-quiet',
'-create-xcframework']

[command.extend(['-framework', os.path.abspath(framework)]) for framework in args.frameworks]
Copy link
Member

Choose a reason for hiding this comment

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

nit: typically list comprehensions are used for constructing lists, here you are constructing a list but ignoring the result. A for loop would be idiomatic python here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the idiomatic tips, my python-foo is zero.

@jmagman
Copy link
Member Author

jmagman commented Nov 16, 2020

It would be nice to have a very simple test for create_xcframework.py

@gaaclarke Can you point me to an existing example? The only one I see is ./tools/gn_test.py but I don't see where it's actually hooked up to run. Thanks for your help with this!

@gaaclarke
Copy link
Member

I'm not sure there is an example for you. All tests are run through testing/run_tests.py, you'd want to add the execution of your test there. I didn't see an equivalent test in there already. Even just running it through a linter would be better than nothing, just to make sure its has valid python code in it.

@jmagman
Copy link
Member Author

jmagman commented Nov 16, 2020

Even just running it through a linter would be better than nothing, just to make sure its has valid python code in it.

Created flutter/flutter#70654.

@jmagman
Copy link
Member Author

jmagman commented Nov 16, 2020

I'm not sure there is an example for you.

Scenarios.app won't build if the Flutter.xcframework is missing. That's about the same coverage we had with any of the other framework build subscripts, like install_framework_headers.py.

Copy link
Member

@gaaclarke gaaclarke left a comment

Choose a reason for hiding this comment

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

lgtm!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants