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

Don't implicitly run generateMetricsMarkdown when writing locks #283

Closed
wants to merge 2 commits into from

Conversation

pkoenig10
Copy link
Member

Similar to palantir/gradle-baseline#1389.

Before this PR

Running ./gradlew --write-locks triggers generateMetricsMarkdown which requires compiling all projects that are dependencies of the distribution project.

After this PR

Running ./gradlew --write-locks does not trigger generateMetricsMarkdown. To update the generated markdown users need to run ./gradlew generateMetricsMarkdown --write-locks explicitly.

@pkoenig10 pkoenig10 requested a review from carterkozak June 14, 2020 18:04
@changelog-app
Copy link

changelog-app bot commented Jun 14, 2020

Generate changelog in changelog/@unreleased

Type

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

Description

Running ./gradlew --write-locks no longer runs generateMetricsMarkdown implicitly. To update your generated markdown, you need to run ./gradlew generateMetricsMarkdown --write-locks explicitly.

Check the box to generate changelog(s)

  • Generate changelog entry

@ferozco
Copy link
Contributor

ferozco commented Jun 15, 2020

👎 Its actually pretty important that ./gradlew --write-locks re-generates the metrics.md file since otherwise PRs opened by our automation would require users action. Its also kinda nice that there is a single command users have to know to update a variety of generated files.

That being said we should definitely look into ways to make generating markdown as cheap as possible.

@pkoenig10
Copy link
Member Author

How is this any different than checkClassUniqueness? After palantir/gradle-baseline#1389 PRs opened by our automation will have to run ./gradlew checkClassUniqueness --write-locks in order to update those lockfiles.

It just seems odd that we're taking an inconsistent position here. Without this PR, palantir/gradle-baseline#1389 doesn't really provide any benefit because I still need to compile all my code every time I run --write-locks.

@ferozco
Copy link
Contributor

ferozco commented Jun 15, 2020

I agree that the inconsistency is confusing and I wasn't aware that the change was made. I think there is a difference since it is much more uncommon for an excavator to result in conflicting classes and it the cases where they do it can be valuable to require the repo owner to consider the conflict. Meanwhile I don't think its valuable to force repo owners to accept every change to the metrics produced by a library.

I'm sympathetic to the concern though, so I'd like to spend some time investigating how we can get the behaviour we want without needing to compile the world.

@bmarcaur
Copy link
Member

Huge +1 to what @pkoenig10 is saying here, --write-locks has specific semantic meaning inside of Gradle and even its usage require some companion task i.e. dependencies. This decision effectively turns the trivial task of resolving dependencies into a full compile. This may only take a few seconds on some smaller repositories, but on PGDev this can be a difference of ~10min.

As a more meta-point, this setup is extremely opaque and difficult to disable. It required me to trace through multiple build scans investigating top level tasks in order to discover which one was triggering the compile task. This was made even more difficult given that nothing in PGDev actually references this task by name. After a Google search I was able to discover that this plugin hijacked --write-locks.

As a workaround I am forced to disable the task explicitly -x generateMetricsMarkdown just so that it is fast enough to be useful as a pre-commit hook.

@ferozco
Copy link
Contributor

ferozco commented Jul 22, 2020

Should be fixed by #313 which allows us to avoid compilation when updating the metrics markdown

@ferozco ferozco closed this Aug 20, 2020
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.

3 participants