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

Tackle deprecation warning generated by Gradle 4.x #1853

Merged
merged 1 commit into from Dec 4, 2017

Conversation

@smoothreggae
Copy link
Contributor

@smoothreggae smoothreggae commented Dec 2, 2017

In Gradle 4.x, the method getClassesDir() in SourceSetOutput has been
deprecated in favor of a new method getClassesDirs().

Use reflection to invoke this method, if it is available, so that
consumers using Gradle 4.x are spared the deprecation warning. Fall back
to the original method for consumers using Gradle 3.x.

Fixes #1686

In Gradle 4.x, the method getClassesDir() in SourceSetOutput has been
deprecated in favor of a new method getClassesDirs().

Use reflection to invoke this method, if it is available, so that
consumers using Gradle 4.x are spared the deprecation warning. Fall back
to the original method for consumers using Gradle 3.x.

Fixes #1686
@CLAassistant
Copy link

@CLAassistant CLAassistant commented Dec 2, 2017

CLA assistant check
All committers have signed the CLA.

Method getClassesDirs = SourceSetOutput.class.getMethod("getClassesDirs");

// use alternative method available in Gradle 4.0
FileCollection classesDirs = (FileCollection)getClassesDirs.invoke(sourceSet.getOutput());

This comment has been minimized.

@inikolaev

inikolaev Dec 3, 2017
Contributor

I think it would be better to encapsulate this logic in some kind of SourceSetOutputResolver or something like that which would handle differences between different versions of Gradle and would return a list of paths.

For example something like this (haven't tested the code though):

class SourceSetOutputResolver {
    private static final Method getClassesDirs;

    static {
        try {
            getClassesDirs = SourceSetOutput.class.getMethod("getClassesDirs");
        } catch (NoSuchMethodException e) {
            getClassesDirs = null;
        }
    }

    public Set<File> getFiles(SourceSet sourceSet) {
        SourceSetOutput sourceSetOutput = sourceSet.getOutput();

        if (getClassesDirs != null) {
            FileCollection classesDirs = (FileCollection) getClassesDirs.invoke(sourceSetOutput);
            return classesDirs.getFiles();
        } else {
            return Collections.singletonSet(sourceSetOutput.getClassesDir());
        }
    }
}

Alternatively it could be part of the current class.

This comment has been minimized.

@axelfontaine

axelfontaine Dec 3, 2017
Contributor

Why? What would be the benefit over the current simple implementation in this pull request?

This comment has been minimized.

@inikolaev

inikolaev Dec 3, 2017
Contributor

Well at least to keep code DRY, because right now same logic is basically repeated twice. Easier to make changes in case Gradle changes API in the future again. It doesn't have to be a separate class, I just gave one possible solution and it could be just a private/protected method which encapsulates this logic.

And IMHO it's easier to follow this logic:

for (File directory: getClassesDirs()) {
    URL classesUrl = directory.toURI().toURL();
    getLogger().debug("Adding directory to Classpath: " + classesUrl);
    extraURLs.add(classesUrl);
}

than figuring out what is going on in all these try-catch blocks.

It's a rather simple change, and at the same time it's not a big deal. I just wanted to point out that there's some room for improvement.

This comment has been minimized.

@smoothreggae

smoothreggae Dec 3, 2017
Author Contributor

I could certainly move out the code that derives classesUrl to a private method: this would avoid duplicating two lines (the first being the logging call and the second being the one adding classesUrl to extraURLs). A stronger case could be made for this were there more than one place in the file that had this change. Let me know if I should do this.

A separate class seems like overkill, though, since we have just this one instance of having to deal with different versions of Gradle. If the plugin chose to support more versions of Gradle and more such changes were needed, I suspect a separate class with appropriate utility methods would evolve from the required code changes.

This comment has been minimized.

@inikolaev

inikolaev Dec 4, 2017
Contributor

Also another thing is that feature detection is performed for each subclass of AbstractFlywayTask and for multiple SourceSet-s potentially, but I would guess that it's not executed that many times to cause any issues.

I agree that using a class is an overkill but would still argue that pulling code into a separate method would improve readability and it takes less time to implement than was spent to discuss it :)

I also agree that this a simple use-case, I just try to avoid code duplication when ever possible.

@axelfontaine axelfontaine merged commit 7eb1738 into flyway:master Dec 4, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
@axelfontaine
Copy link
Contributor

@axelfontaine axelfontaine commented Dec 4, 2017

@smoothreggae Thanks you very much for the pull request! Merged.

selliera pushed a commit to selliera/flyway that referenced this pull request Dec 7, 2017
Tackle deprecation warning generated by Gradle 4.x
dohrayme pushed a commit to dohrayme/flyway that referenced this pull request Feb 3, 2020
Tackle deprecation warning generated by Gradle 4.x
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.