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

Avoid adding index file to the resources by default #4

Closed
vlsi opened this issue Jan 5, 2021 · 10 comments
Closed

Avoid adding index file to the resources by default #4

vlsi opened this issue Jan 5, 2021 · 10 comments
Assignees
Labels
enhancement New feature or request

Comments

@vlsi
Copy link

vlsi commented Jan 5, 2021

The plugin adds index file unconditionally, however, that would result in issues when dependency shading is used.

For instance, if different libraries add their own indices, then dependency shading would overwrite/duplicate index files.

I guess it would be nice to avoid adding the index file to the resulting jar file.
The jandex task would still be useful as it ensures the bytecode is jandex-parseable.

@aalmiray
Copy link
Collaborator

aalmiray commented Jan 5, 2021

Funny you mention this, the first releases placed the index file at a different location however changes were made to align this plugin with its Maven counterpart. You can change the location of the generated index file by setting the destination property on the jandex task.

@vlsi
Copy link
Author

vlsi commented Jan 5, 2021

I doubt the index would be usable for a library in case the filename is customized, so I'm inclined to avoid including it at all.

@aalmiray
Copy link
Collaborator

aalmiray commented Jan 5, 2021

Hmm I guess I'm missing something here. The plugin does give you options to change either the name of the index, its location, or both. It's up to you as library author to decide which conventions to follow. This being said, if the location is changed to a different path then you'd still need additional configuration in the build file if the index has to be included in a JAR at some point. Thus, if the conventions are followed you don't need extra configuration, but if you do deviate from the conventions then you require additional configuration.

A consumer that decides to shade resources would have to follow their own conventions, deal with potentially colliding entries. It's not the job of the index producer to solve this particular problem.

@vlsi
Copy link
Author

vlsi commented Jan 5, 2021

if the location is changed to a different path then you'd still need additional configuration in the build file if the index has t be included in a JAR

I see. I thought it would be included into the jar under a customized name.

It's not the job of the index producer to solve this particular problem

In practice, collision resolution is not very well implemented.
As you might know, a top-level /LICENSE would easily produce a collision. Unfortunately, consumers are not much into fixing that :(

Note: jandex misses the ability to merge indices, so consumers won't have an option to merge indices when they shade dependencies.
One of the solutions is to throw-away all the indicices and reindex after shading, however, that would require extra build configuration, and the benefits from the library author's perspective look very small 🤷‍♂️

That is why I would refrain from adding a fixed-named file to a library (e.g. https://github.com/pgjdbc/pgjdbc/ ).

Note: I do not need the index for my library yet. I have no use case for it. I want to pass the classes through jandex to ensure that the resulting bytecode is parseable by jandex.


Unfortunately, jandex has issues, and it results in Hibernate failing in surprising ways even though pgjdbc's bytecode is perfectly valid.

This PR fixes classes that Jandex can't parse, and the only thing I want for now is a CI check that would ensure jandex compatibility. Of course, I would love jandex bugs to be fixed, however, it would take time for everybody to upgrade.

@aalmiray
Copy link
Collaborator

aalmiray commented Jan 5, 2021

re: License file. Precisely. You don't see plugin/library authors trying to fix this particular file name, do you? Because the "problem" is on the consumer side that requires shading, not on the producing side.

I understand that Jandex has some short comings and the fixes may take some time to appear. Merging jandex index files could be done by Jandex itself or as an external shade/shadow transformer (like the [PropertiesFileTransformer](https://github.com/kordamp/maven-shade-ext-transformers) available to both Maven shade and Gradle shadow). This transformer is unrelated to the index producing plugin (Maven or Gradle).

This plugin lets you configure index name, location, and sources as needed. If the conventions are followed then there's little to no extra configuration needed. If a different convention is needed then extra configuration is mandated. I don't see what else needs to be updated/modified here, unless perhaps we enter a discussion on how much extra configuration may be required from a consumer's POV.

@vlsi
Copy link
Author

vlsi commented Jan 5, 2021

You don't see plugin/library authors trying to fix this particular file name, do you?

I do. The standard location for the licence file is /META-INF/LICENSE rather than /LICENSE, and shade plugins recognize that. On top of that, the license management plugins recognize that and they can gather and infer dependency licenses.

For example, junit4 renamed the file to LICENSE-junit.txt :( (however, they should have better be using the standard META-INF/LICENSE location)

I understand that Jandex has some short comings and the fixes may take some time to appear

I've filed the corresponding issue: smallrye/jandex#100

unless perhaps we enter a discussion on how much extra configuration

It would help if the documentation clarified when the index is automatically added to the jar, and when it is not.

@aalmiray
Copy link
Collaborator

aalmiray commented Jan 5, 2021

I do. The standard location for the licence file is /META-INF/LICENSE rather than /LICENSE, and shade plugins recognize that. On top of that, the license management plugins recognize that and they can gather and infer dependency licenses.

This validates my point regarding a resource transformer that targets Jandex index files until the time comes (if it comes) when Jandex offers an index merge option.

It would help if the documentation clarified when the index is automatically added to the jar, and when it is not.

The readme shows the default value of the destination property as build/resources/main/META-INF/jandex.idx

@vlsi
Copy link
Author

vlsi commented Jan 5, 2021

The readme shows the default value of the destination property as build/resources/main/META-INF/jandex.idx

It does not look like "adds index to the jar" or "does not add index to the jar" to me :-/

Just in case, I just tried 0.8.0, and it looks like a customized destination does not make a difference.
I guess it is caused by

jandex.get().destination.set(project.file("${t.destinationDir}/META-INF/jandex.idx"))
which always resets the destination to its default location.

@vlsi
Copy link
Author

vlsi commented Jan 5, 2021

Just in case, the sequence of the tasks for ./gradlew jar --dry-run is as follows:

Building pgjdbc 42.2.18-SNAPSHOT
:postgresql:preprocessVersion SKIPPED
:postgresql:compileJava SKIPPED
:postgresql:processResources SKIPPED // <-- destination overwrite happens here
:postgresql:classes SKIPPED
:postgresql:jandex SKIPPED
:postgresql:jar SKIPPED

@aalmiray
Copy link
Collaborator

aalmiray commented Jan 5, 2021

Indeed. The JandexPlugin is responsible for copying the index file to the resources directory, even if a different location was specified. This happens only for the explicit jandex task but not for any other task of type JandexTask. I'll add a config option to the task to keep or skip this step.

@aalmiray aalmiray self-assigned this Jan 5, 2021
@aalmiray aalmiray added the enhancement New feature or request label Jan 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants