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

De-duplicate Module.xml jar entries based on scope. #53

Merged

Conversation

benmusson
Copy link
Contributor

The logic:

  1. Group the manifests by jarName.
  2. For each jarName, find all the scopes where it is applied and append them together in a list using flatMap.
  3. Convert this list of scopes to a set in order to deduplicate, then join back together into a string.

At this point the de-duplication is complete and the original issue is resolved. #52

I think it would also be a good idea to force a sort order for the jars, so I added it in.

  1. Sort by scope length (jars used in more places are listed first).
  2. Sort by scope alphabetically
  3. Sort by jarName alphabetically

Before:

<jar scope="C">core-client-0.1.0.jar</jar>
<jar scope="C">kotlin-stdlib-2.0.0.jar</jar>
<jar scope="C">annotations-13.0.jar</jar>
<jar scope="C">if97-steam-tables-client-0.1.0.jar</jar>
<jar scope="CDG">core-common-0.3.0.jar</jar>
<jar scope="CDG">if97-2.0.0.jar</jar>
<jar scope="CDG">kotlin-stdlib-2.0.0.jar</jar> 
<jar scope="CDG">annotations-13.0.jar</jar>
<jar scope="CDG">if97-steam-tables-common-0.1.0.jar</jar>
<jar scope="D">core-designer-0.3.0.jar</jar>
<jar scope="D">kotlin-stdlib-2.0.0.jar</jar>
<jar scope="D">annotations-13.0.jar</jar>
<jar scope="D">if97-steam-tables-designer-0.1.0.jar</jar>
<jar scope="G">core-gateway-0.3.0.jar</jar>
<jar scope="G">kotlin-stdlib-2.0.0.jar</jar>
<jar scope="G">annotations-13.0.jar</jar>
<jar scope="G">if97-steam-tables-gateway-0.1.0.jar</jar>

After:

<jar scope="CDG">annotations-13.0.jar</jar>
<jar scope="CDG">core-common-0.3.0.jar</jar>
<jar scope="CDG">if97-2.0.0.jar</jar>
<jar scope="CDG">if97-steam-tables-common-0.1.0.jar</jar>
<jar scope="CDG">kotlin-stdlib-2.0.0.jar</jar>
<jar scope="C">core-client-0.1.0.jar</jar>
<jar scope="C">if97-steam-tables-client-0.1.0.jar</jar>
<jar scope="D">core-designer-0.3.0.jar</jar>
<jar scope="D">if97-steam-tables-designer-0.1.0.jar</jar>
<jar scope="G">core-gateway-0.3.0.jar</jar>
<jar scope="G">if97-steam-tables-gateway-0.1.0.jar</jar>

@brianeray
Copy link
Collaborator

Hi @benmusson , thanks for the PR. Can you add a unit test in WriteModuleXmlTest.kt?

@brianeray brianeray self-requested a review August 9, 2024 23:37
@benmusson benmusson force-pushed the fix/module-xml-jar-deduplication branch from 3010cfa to 865d6f2 Compare August 10, 2024 05:42
add test for jar presence, duplication, and sort order

linting
@benmusson benmusson force-pushed the fix/module-xml-jar-deduplication branch from 865d6f2 to 54b121e Compare August 13, 2024 21:12
@benmusson
Copy link
Contributor Author

@brianeray rebased/squashed

@brianeray
Copy link
Collaborator

@benmusson can you take a look at my stab in #56? I'll probably pull in a reviewer from our side too, someone who knows module dev, because I'm a build engineer.

My focus is too narrow. I can do stuff in Gradle and JVM languages but not have the bigger picture in focus.

Add `--foldJars` task option to preserve the old behavior by default,
it's a breaking change and maybe the dups are useful to some.

The option if passed does the scope folding and the sorting as before
and another sort of folding for `CDG`-scoped jars, turning them into
the `A`[ll]-scoped equivalent.

Plus noticed that the tests fall into the `assertEquals` API trap where
you think the argument order is `actual, expected` but it's really
`expected, actual`. Completely nonintuitive and sort of asymmetric to
other `assert*` APIs but when `assertEquals` fails having them in the
wrong order makes things harder to troubleshoot.
@brianeray
Copy link
Collaborator

One final (🤞 ) bit of ping pong after the module developers here weighed in. It's in my new draft PR #57 at the last commit 22946e2.

@brianeray brianeray merged commit a9f0a15 into inductiveautomation:master Aug 26, 2024
1 check passed
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