Skip to content

ext broken in app.gradle on tns-android@next #1183

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

Closed
edusperoni opened this issue Oct 4, 2018 · 15 comments
Closed

ext broken in app.gradle on tns-android@next #1183

edusperoni opened this issue Oct 4, 2018 · 15 comments
Labels
Milestone

Comments

@edusperoni
Copy link
Collaborator

Environment
Provide version numbers for the following components (information can be retrieved by running tns info in your project folder or by inspecting the package.json of the project:

  • CLI: 4.2.4
  • Cross-platform modules: 4.2.1
  • Android Runtime: 4.3(@next) to @next

Describe the bug

After build 4.3.0-2018-08-10-09, the ext and project.ext part of app.gradle is ignored.

To Reproduce
Easy way:

  • create an empty project
  • tns plugin add nativescript-plugin-firebase
  • set on app.gradle:
ext {
  googlePlayServicesVersion = "10.0.1"
}

this should print:

* What went wrong:
A problem occurred evaluating script.
>  googlePlayServicesVersion set too low, please update to at least 15.0.0 / 15.0.+ (currently set to 10.0.1)

But it doesn't, and it continues ignoring the ext part.

Manually setting ext in platforms/android/build.gradle works as expected.

Expected behavior

ext is not ignored.

Sample project

any project with android@4.3.0-2018-08-10-09 or above will do

Additional context

this seems to be happening since this commit:
0c83f7c

@edusperoni
Copy link
Collaborator Author

Probably related: #878

@edusperoni
Copy link
Collaborator Author

I found the culprit:
#1124
#1123

this makes it so ext properties are not loaded by the time plugin configurations are being imported. I suppose we may need some pre/post app.gradle solution.

Using this order solves the ext issue:

    setAppIdentifier()
    applyAppGradleConfiguration()
    applyPluginGradleConfigurations()

@farfromrefug
Copy link
Contributor

@edusperoni the new order is good because plugins' config should always be set before app's config.
However you are right the ext should be initialized before running applyPluginGradleConfigurations

@edusperoni
Copy link
Collaborator Author

@farfromrefug I'm aware, I was just pointing out how to make ext work again. Maybe something like ext.gradle or options.gradle that is called before the plugin configuration would solve this issue.

Either way, that PR is an undocumented breaking change.

@NickIliev NickIliev added the bug label Oct 5, 2018
@NickIliev
Copy link

Project to reproduce the issue (while using all next from 05.10) can be found here

@vtrifonov
Copy link
Contributor

@edusperoni @farfromrefug what about calling applyAppGradleConfiguration before and after applyPluginGradleConfigurations if there are plugin include.gradle files, but there might be some issues when applying something twice, so you'll need to handle those cases inside the app.gradle by using some variable to determine whether it is the first or the second apply of the file. Please, let me know what you think about this!

@farfromrefug
Copy link
Contributor

@vtrifonov seems like a source of errors to me.Why not having a function just initializing ext with the default Nativescript values?

@vtrifonov
Copy link
Contributor

@farfromrefug What do you mean by default Nativescript values? In the example the problem is that project.ext.googlePlayServicesVersion is set in the app.gradle file which is executed after applying the plugin include.gradle which checks for googlePlayServicesVersion in the project.ext, so I don't see how initialising any project.ext values can solve that problem without executing the app.gradle before the plugin gradle files.

@farfromrefug
Copy link
Contributor

@vtrifonov I thought the issue was with project.ext not existing or project.ext.googlePlayServicesVersion not existing. But now I see the issue.
Could we separate the variable declaration in another .gradle file?
Not very nice but would work:

setAppIdentifier()
    applyVariablesGradleConfiguration()
    applyPluginGradleConfigurations()
    applyAppGradleConfiguration()

@vtrifonov
Copy link
Contributor

@farfromrefug that would be the best solution, however it will be a breaking change and in addition to this we'll need to update all templates to have this file, which might be confusing for some of the users. I'm not sure whether applying the same file (app.gradle) twice (only if any include.gradle is present) could break anything.

@farfromrefug
Copy link
Contributor

@vtrifonov I totally agree with you. I have looked at the gradle to see and if there would be a way to only apply part of a gradle file (like only ext definition) but that does not seem possible.
Let's try with app.gradle include twice?

@vtrifonov
Copy link
Contributor

@farfromrefug actually we've decided to use another file, as this is the cleaner approach.

@vtrifonov vtrifonov added this to the 5.0.0 milestone Oct 11, 2018
@farfromrefug
Copy link
Contributor

@vtrifonov i am still facing an issue with this. The issue is now with the tempplugin builds. I have no way to force compileSdkVersion, buildToolsVersion,...
But my plugin requires 28 (android material components).
I tried with before-plugins.gradle but it is not use for plugins compilation.

@vtrifonov
Copy link
Contributor

@farfromrefug we are adding execution of the same before-plugins.gradle file in the tempPlugin builds, you can check the following PR in the CLI and you can try it with nativescript@rc.

@farfromrefug
Copy link
Contributor

@vtrifonov i have tested rc and it is working perfectly now! thank you!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants