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

BaselineExactDependencies applies to all source sets #1262

Merged
merged 13 commits into from
Feb 28, 2020

Conversation

ferozco
Copy link
Contributor

@ferozco ferozco commented Feb 26, 2020

Before this PR

BaselineExactDependencies only applied to the main source set

After this PR

==COMMIT_MSG==
BaselineExactDependencies applies to all source sets
==COMMIT_MSG==

Possible downsides?

This will flag even more unused dependencies (including in tests), and could block upgrades

@changelog-app
Copy link

changelog-app bot commented Feb 26, 2020

Generate changelog in changelog/@unreleased

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

BaselineExactDependencies applies to all source sets

Check the box to generate changelog(s)

  • Generate changelog entry

GUtil.toLowerCamelCase("checkUnusedDependencies " + sourceSet.getName()),
CheckUnusedDependenciesTask.class,
task -> {
task.dependsOn(JavaPlugin.CLASSES_TASK_NAME);
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be per source set

Preconditions.checkArgument(
configuration.isCanBeResolved(),
"May only add sourceOnlyConfiguration if it is resolvable: %s",
configuration);
Copy link
Contributor

Choose a reason for hiding this comment

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

To be fair this method shouldn't exist, because I don't know in what situation users might actually use it.
The name is confusing now too since it implies source-only configurations need to be added, but we already add (ignore) all compileOnly configurations from all source sets.

But for now, let's at least throw eagerly if the user gave us a bad configuration.

@iamdanfox
Copy link
Contributor

I don't really want to apply this to test source sets, as I think it will cause more noisy and frustration. The intention of this task is to reduce the cruft that people ship in prod, and requiring test sourcesets to be minimal I think is overly strict.

@dansanduleac
Copy link
Contributor

So, as mentioned in person I think it's fine in principle to require people's tests to also have exact dependencies, as this is an opt-in thing and can help reduce the amount of stuff downloaded when running tests.

However, having tried this in practice, checkUnusedDependenciesTest won't work in its current form because

  • testImplementation extends from implementation
  • testCompile extends from compile
    and so all of the dependencies coming from the main source set will show up as unused when running this on the test source set

@ferozco
Copy link
Contributor Author

ferozco commented Feb 27, 2020

Ok, seems like we're not in sync on the desired outcome and it doesn't help solve the initial problem that got me looking at this. Happy to close this out

@ferozco ferozco closed this Feb 27, 2020
@dansanduleac dansanduleac reopened this Feb 28, 2020
@dansanduleac
Copy link
Contributor

I think we still want this outcome, it's just that we need to be clear on what dependencies we consider 'explicit' and thus markable as unused.
I wrote up a change that only resolves dependencies that come directly from the compile and implementation of each source set, and this seems to solve the problem (I've tested it on a project using gradle-atlas internally).

// testImplementation extendsFrom(implementation)
// \-- testCompile extendsFrom(compile)
// We therefore want to look at only the dependencies _directly_ declared in the implementation and compile
// configurations (belonging to our source set)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we write a test for this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, wrote one

'''.stripIndent()

then:
def result = with('checkUnusedDependencies', '--stacktrace').build()
Copy link
Contributor

Choose a reason for hiding this comment

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

Without my latest changes (with the previous implementation at https://github.com/palantir/gradle-baseline/pull/1262/files/bc8e9e2cca856833a02d3d8775c9641378a8953f) this test failed with:

* What went wrong:
Execution failed for task ':checkUnusedDependenciesTest'.
> Found 1 dependencies unused during compilation, please delete them from 'build.gradle' or choose one of the suggested fixes:
  	com.google.guava:guava

@bulldozer-bot bulldozer-bot bot merged commit 649b6e1 into develop Feb 28, 2020
@bulldozer-bot bulldozer-bot bot deleted the fo/check-dependencies-all-source-sets branch February 28, 2020 16:00
@svc-autorelease
Copy link
Collaborator

Released 3.7.0

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

Successfully merging this pull request may close these issues.

None yet

5 participants