-
Notifications
You must be signed in to change notification settings - Fork 9.7k
[firebase_core] Automate version retrieval #1733
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good for the Android side; a few nits
@@ -8,7 +8,7 @@ | |||
|
|||
public class FlutterFirebaseAppRegistrar implements ComponentRegistrar { | |||
private static final String LIBRARY_NAME = "flutter-fire-core"; | |||
private static final String LIBRARY_VERSION = "0.4.0+3"; | |||
private static final String LIBRARY_VERSION = BuildConfig.FLUTTER_FIRE_VERSION; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you need to declare it here; you can just use BuildConfig.VERSION_NAME
below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
android { | ||
compileSdkVersion 28 | ||
|
||
defaultConfig { | ||
minSdkVersion 16 | ||
testInstrumentationRunner "androidx.test.runner.AndroidJUnitRunner" | ||
buildConfigField "String", "FLUTTER_FIRE_VERSION", "\"${flutterFireVersion()}\"" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I would call this VERSION_NAME; FLUTTER_FIRE is a bit non standard compared to what other SDKs are doing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
489879d
to
604ef79
Compare
* @return String representing version of the plugin. | ||
*/ | ||
def flutterFireVersion() { | ||
File pubspec = new File('../../pubspec.yaml') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may be cleaner:
File pubspec = new File(project.projectDir.parentFile, 'pubspec.yaml')
project.projectDir = <pluginRoot>/android
parentFile = <pluginRoot>
Otherwise should probably use File.separator
for Windows support instead of /
@Override | ||
public List<Component<?>> getComponents() { | ||
return Collections.<Component<?>>singletonList( | ||
LibraryVersionComponent.create(LIBRARY_NAME, LIBRARY_VERSION)); | ||
LibraryVersionComponent.create( | ||
"flutter-fire-core", BuildConfig.VERSION_NAME.replaceAll("\\+", "-"))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this replace all need to be done at runtime - can we move the logic to build time inside of the flutterFireVersion()
method?
@@ -18,4 +18,11 @@ A new flutter plugin project. | |||
s.dependency 'Firebase/Core' | |||
s.ios.deployment_target = '8.0' | |||
s.static_framework = true | |||
|
|||
s.prepare_command = <<-CMD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CocoaPods already provides the versions via an NSBundle
for each podspec, as shown in my other PR - not sure if code-gen is needed if we already have the version at runtime
@@ -18,4 +18,11 @@ A new flutter plugin project. | |||
s.dependency 'Firebase/Core' | |||
s.ios.deployment_target = '8.0' | |||
s.static_framework = true | |||
|
|||
s.prepare_command = <<-CMD | |||
PUBSPEC_VERSION=`cat ../pubspec.yaml | grep version: | sed 's/version: //g'` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's possible to read the yaml file via the default Ruby yaml library that comes with OSX
require 'yaml'
pubspec = YAML.load_file(File.join('..', 'pubspec.yaml'))
# ...
Pod::UI.puts "This plugin version is '#{pubspec['version']}'"
This PR currently breaks plugin integrations as shown in #34385. |
This reverts commit 1e01c9e.
* Automate version retrieval
* Revert "[firebase_core] Automate version retrieval (flutter#1733)" This reverts commit 1e01c9e. * Update versions and CHANGELOG to call this version 0.4.0+5
Description