Skip to content

Support multiple configurations in one diff #13

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

Open
wants to merge 4 commits into
base: trunk
Choose a base branch
from

Conversation

TWiStErRob
Copy link
Contributor

Allow gradlew :module:dependencies > deps.txt to be the input to the program.
Also includes some fixes from #11 and #12, but I think those will get merged first.

Solution

Originally the "dependency list" was created by looking between +--- and an empty line.
I've extended this to look for the starting token +---, then peek before to find the configuration name (and sometimes description). Then create a map of (configuration, diff), and produce a nice output.

Note: all expected.txts had to change because it now outputs which configuration is being diffed, even when there's only one.


Examples where configurations and dependencies change
(gradlew :dependencies on this project with different version of Gradle and Kotlin):

The unit tests include gradle742-kotlin14.txt and gradle742-kotlin15.txt diff (configuration-differences) as well as a diff of one of my projects when updating Dagger which affects compile, runtime and kapt classpaths (multiple-configurations).

@TWiStErRob TWiStErRob force-pushed the support_multiple_configurations branch from 149cfc0 to 1e3c82a Compare June 21, 2022 16:40
@TWiStErRob
Copy link
Contributor Author

Rebased on #12 and #11 merges, now clean, ready for review/rejection :)

@JakeWharton
Copy link
Owner

Yeah I'm a bit hesitant about adding this. The reason we didn't do this initially was because there's far too much noise/duplication in the output. In an Android project or a Kotlin multiplatform project you can get upwards of 30 or 40 configurations which rarely are unique.

What's the motivation to diff them all?

@TWiStErRob
Copy link
Contributor Author

TWiStErRob commented Jun 25, 2022

Simple: I want to see an overall change, even if it's long. At the moment I'm diffing two trees by hand, and I want to adopt this tool, but, without support for multiple configurations, I can't get equivalent use.

Here's what I have so far, based on this PR to add multiple support: https://github.com/TWiStErRob/net.twisterrob.cinema/runs/7052429575
This check run fails the build if the deps change, and I can see a ready-made diff, which I can approve via a few clicks. (This last bit is just for fun to practice and test what GHA can be used for.)

Argument 1: changing a dependency won't affect just one configuration

Let's take the above Dagger as the first example: if I diff'd the runtime classpath as you suggest in the readme, I would get this:
image
Looks all good, but looking at all configurations reveals that it actually changed kapt classpaths a lot, which, if I had multiple annotation processors could cause problems.

Another example for the same is changing test-only dependencies, e.g. Robolectric / Espresso, or prod+test dependencies like androidx fragment + fragment-testing.

To discover issues in all the above cases, I (and everyone else in the world using this) would need to know exactly which configurations are affected for each of the dependency changes and manually diff each one of them.

Argument 2: multiple modules is already adding enough overhead

Having a multi-module project is normal and dependencies in each module change. What I did in this cinema project is a bit overkill, diffing all modules, but you can easily imagine having multiple entry points in one project (android, desktop, backend monorepo). Creating diffs for each module is already complicated, imagine having to use the tree-diff tool to generate 7-8 diffs per module for specific relevant configurations (release, debug, variant) x (kapt, kaptTest, test, compileOnly, runtime).

Argument 3: sheer dev laziness

How much simpler it is to run gradlew :app:dependencies than using --configuration= 😅. I would imagine most devs have problems with the syntax of Gradle's command line, or don't know what configurations are, or know but don't care. So catering for more developer use cases helps the community.

Argument 4: compatibility and DX

In addition to the above use case I think it's simply a DX improvement to handle the standard output gradlew dependencies can produce, not just a specific case, but all of them (similar to #12, but the other way around 😄). Allowing to diff the whole output also helps to use this in automation detecting any kind of change in the tree, and outputting it in a nice way, like I did in the above CI job.

@TWiStErRob
Copy link
Contributor Author

Another example for 1) I'm migrating from buildSrc to libs and that's a syntax change, so there shouldn't be any deps diff. At this point I need full diff, which of course could be done just with a normal diff, but your tree diff is way better. GitHub Check run discovering I made a mistake somewhere: https://github.com/TWiStErRob/net.twisterrob.cinema/pull/145/checks?check_run_id=7059856668

@JakeWharton
Copy link
Owner

In order to maintain behavioral compatibility, if there's only a single configuration lets omit the header with its name.

Otherwise this seems fine to include.

@TWiStErRob
Copy link
Contributor Author

Can I add a flag to include it?

@JakeWharton
Copy link
Owner

If there's only one why is it needed?

@JakeWharton
Copy link
Owner

If you want to rebase and remove the header for single configuration scenarios I'm still open to this. Going to do a release with a bugfix first, however.

@TWiStErRob
Copy link
Contributor Author

TWiStErRob commented Jan 2, 2024

Ugh, this slipped off my to-do list :)
Will do the rebase this week year.

@TWiStErRob TWiStErRob force-pushed the support_multiple_configurations branch from 1e3c82a to a60ce68 Compare April 8, 2024 19:35
@TWiStErRob TWiStErRob force-pushed the support_multiple_configurations branch from a60ce68 to bcb15a7 Compare April 8, 2024 20:00
@TWiStErRob TWiStErRob force-pushed the support_multiple_configurations branch from bcb15a7 to ba817de Compare April 8, 2024 20:13
@TWiStErRob
Copy link
Contributor Author

@JakeWharton ready for re-review / potentially merge.

  • Rebase onto latest trunk: force push

  • Rebase dropping "single configuration headers" (as requested here): force push

  • If there's only one why is it needed?

    There are cases when the gradlew :x:y --dependencies only has a single configuration (special definitely non-Android modules). In case the tree-diff jar is used to process a module like that, then the single configuration has meaning and could provide very valuable context (because the single configuration wasn't requested explicitly with --configuration). The tool is meant for humans after all, right?

    It also is just more self-consistent (even though historically different); and the code is simpler. See the if that's required for "behavioral compatibility".

  • I looked at the diff of the jar and it adds 8k extra, I tried changing the code to use less functions from stdlib, but it actually added 1k more 😫.

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

Successfully merging this pull request may close these issues.

2 participants