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

Getting Started with Gradle: Avoid using internal APIs #6

Closed
aalmiray opened this issue Sep 19, 2018 · 9 comments
Closed

Getting Started with Gradle: Avoid using internal APIs #6

aalmiray opened this issue Sep 19, 2018 · 9 comments

Comments

@aalmiray
Copy link
Contributor

aalmiray commented Sep 19, 2018

The current example shows the usage of internal Gradle APIs to compute the value of a platform Script variable. It would be better to use public APIs or a plugin to resolve this value as Gradle may remove/change internal APIs at any given time.

@aalmiray
Copy link
Contributor Author

aalmiray commented Sep 19, 2018

A possible solution would be to use https://github.com/google/osdetector-gradle-plugin from Google, which builds on top of https://github.com/trustin/os-maven-plugin.

This plugin resolves linux and windows as expected but uses osx for MacOSX. The JavaFX 11 artifacts expect mac, thus the minimum changes needed for it to work are

buildscript {
    repositories {
        maven { url 'https://plugins.gradle.org/m2/' }
    }

    dependencies {
        classpath 'com.google.gradle:osdetector-gradle-plugin:1.6.0'
    }
}

apply plugin: 'com.google.osdetector'

ext.platform = osdetector.os != 'osx' ? osdetector.os : 'mac'

Or alternatively since Gradle 2.1

plugins {
    id 'com.google.osdetector' version '1.6.0'
}

ext.platform = osdetector.os != 'osx' ? osdetector.os : 'mac'

Also, do not use script variables, prefer the usage of project variables, that is,

def platform = 'mac' // don't !!
project.ext.platform = 'mac' // :-)

@abhinayagarwal
Copy link
Collaborator

Using a plugin to replace the internal API and the ugly if..else is definitely a +1

@mkroening
Copy link
Contributor

There is also org.gradle.nativeplatform.platform.OperatingSystem, which is however still incubating.

@aalmiray
Copy link
Contributor Author

Google's ossdetector plugin is used by everyone building gRPC projects, with both Maven and Gradle. That's a sizeable chunk.

@mkroening
Copy link
Contributor

I like it!
Note that for gradle 2.1+ this is sufficient to apply the plugin:

plugins {
  id "com.google.osdetector" version "1.6.0"
}

@tiainen
Copy link
Contributor

tiainen commented Sep 20, 2018

My opinion is that having a dependency to a gradle plugin or a dependency to internal gradle API is the same. They are both pieces that are outside your direct control and that might break things in the future.

Having said that, the plugin dependency is an improvement here, just for the sake of readability. @aalmiray, it would be great if you can create a PR with your suggestions!

@swpalmer
Copy link

Use of a plugin for such a simple thing (at this stage anyway - with only three flavours to pick) seems like overkill. Specially if you have to massage the strings with additional conditions anyway ('osx' -> 'mac').
The use of the internal API in the first place is strange. The author should have known they were making mistake before they finished typing the line. Forgivable for your own private code perhaps, but when making it a public example - worthy of a few lashes. :-)

@tiainen
Copy link
Contributor

tiainen commented Oct 10, 2018

Fixed by #7.

@tiainen tiainen closed this as completed Oct 10, 2018
@abhinayagarwal
Copy link
Collaborator

Just got my hand on a windows machine. The plugin actually sets the platform as 'windows' instead of 'win'.

A viable solution would be to extend the condition:

ext.platform = osdetector.os == 'osx' ? 'mac' : osdetector.os == 'windows' ? 'win' : osdetector.os

I am writing it here in case someone has a better approach.

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

No branches or pull requests

5 participants