Add an Automatic-Module-Name manifest entry

[overheadhunter]

Now, that Dagger runs with JDK 9 (as issue #880 is resolved), it might be time to look forward to JPMS compatibility.

In a first step I'd suggest to add a Automatic-Module-Name entry to the jar manifest. This way other libraries depending on Dagger can be published to Maven Central or other repositories. Without it, the automatic module name would be derived from the Dagger .jar file and as such is pretty unreliable.

This proposed change doesn't affect any code and will simply make a name reservation until an actual module-info.java will be added eventually. More info can be found in this blog post.

[ronshapiro]

This sounds fine to me. I presume that we'll probably want the module names to match the maven artifact names?

• com.google.dagger
• com.google.dagger.compiler
• com.google.dagger.producers
• com.google.dagger.android
• com.google.dagger.android.support

etc.

@netdpb WDYT?

[JakeWharton]

Package name seems more appropriate since it's agnostic to build system and modules deal in exposing of packages.

…

On Sun, Mar 4, 2018, 10:59 PM Ron Shapiro ***@***.***> wrote: This sounds fine to me. I presume that we'll probably want the module names to match the maven artifact names? - com.google.dagger - com.google.dagger.compiler - com.google.dagger.producers - com.google.dagger.android - com.google.dagger.android.support etc. @netdpb <https://github.com/netdpb> WDYT? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1079 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEEEak9rj_DO5SCnhWxtknsUuXzt722ks5tbLgxgaJpZM4SScow> .

[overheadhunter]

To get this issue going, since it can be solved by a single line of configuration:

I don't know how to configure bazel builds, but apparently there is this deploy_manifest_lines argument in the java_binary rule that should allow you to add entries to the MANIFEST.MF.

However, this argument doesn't exist for the java_library rule, which wouldn't make too much sense. Probably just bad documentation?

[metteo]

Currently the build uses java_library rules which don't support MANIFEST.MF additions (see: bazelbuild/bazel#4197). According to linked issue, the output of java_library is only intermediary and should not be used for deployment (java_binary and java_test should be used for such purpose):

@iirina wrote:

Bazel considers only java_binary or java_test as rules that output something for deployment. The other rules should be used as intermediary and the user, in theory, should not depend on the jars created (e.g. by java_library) or implementation details.

Also java_binary's deploy_manifest_lines works only for a name_deploy.jar which is an uber-jar containing all the dependencies. Regular name.jar is not affected as mentioned in documentation and bazelbuild/bazel#2009

To summarize bazel doesn't support custom MANIFEST.MF attributes for main java artifacts and the feature request is open since 2017.

As a workaround we could use jar x command from JDK to extract existing MANIFEST.MF, append required lines and finally update the artifact using jar u command. All this could be added to execute-deploy.sh script.

[metteo]

@ronshapiro FYI currently without the Automatic-Module-Name the name of the module is derived from the file name so for maven artifact dagger-2.26.jar it's dagger:

module some.module {
    requires dagger; // automatic, filename-based
}

Because of that maven build outputs following message:

[WARNING] **** (...)
[WARNING] * Required filename-based automodules detected: [dagger-2.26.jar]. Please don't publish this project to a public artifact repository! *
[WARNING] **** (...)

Seems like this autogenerated module name (dagger) could be used for manifest entry and in the future, in the module-info.java. @overheadhunter provided one article about naming in JPMS.
Below you can find some more (written by @jodastephen)

Java SE 9 - JPMS module naming
Java SE 9 - JPMS modules are not artifacts

[jodastephen]

As per the blog, the module name for each jar file is generally the root package name of the jar file. At a quick glance that would appear to be dagger. (The artifact name is not the one to use, so it isn't com.google.dagger as that is not a package name).

[metteo]

Could any of the decision-making contributors suggest / choose a solution?
From my perspective we have 3 options (sorted by the increasing effort required):

1. Add a pre-maven-deploy process which modifies bazel generated jars by appending required lines in the MANIFEST.MF. As mentioned above JDK jar command has built-in support for this. (involves execute-deploy.sh)
2. Extend bazel to allow MANIFEST.MF customizations in java_library intermediary output and use it to include Automatic-Module-Name. (java_library: Support manifest manipulation bazelbuild/bazel#4197 - closed in 2017)
3. Extend bazel to allow MANIFEST.MF customizations in java_binary main artifact (java_binary: No way to provide dynamic manifest file content bazelbuild/bazel#2009 - inactive since 2017)
   3.1. Switch dagger build to use java_binary (involves BUILD and .sh files)

Seems like (2) won't be accepted in bazel, (3) is stalled and adding the functionality in bazel is non trivial (core contributors didn't figure out yet how to implement it). Also (3) will require major changes in the dagger build infrastructure (simple additions in BUILD files won't suffice)

[netdpb]

Dagger already uses jarjar to create the Maven artifact. We can use or modify that to append to the MANIFEST.MF file.

[metteo]

At the moment jarjar_library is used for generation of shaded_grpc_server_processor and shaded_android_processor here.

However the one that we are interested in is deployed directly:

deploy_library \
  java/dagger/libcore.jar \

Even if we would process it with jarjar it wouldn't help because it doesn't support manifest merging. Instead jarjar_library rule uses inlined bash script to do that:

  # Concatenate similar files in META-INF that allow it.
  mergeMetaInfFiles=(services/.* {merge_meta_inf_files})
  for metaInfPattern in ${{mergeMetaInfFiles[@]}}; do
    for file in $(find META-INF -regex "META-INF/$metaInfPattern\\(~[0-9]*\\)?"); do
      original=$(echo $file | sed s/"~[0-9]*$"//)
      if [[ "$file" != "$original" ]]; then
        cat $file >> $original
        rm $file
      fi
    done
  done

Again we have few options here:

1. Extend jarjar to properly support manifest merging and customizations. Update bazel build rule in the process. Not sure which one though (google's, pants' or shevek's. (The last one seems the most updated)
2. Update jarjar_library rule to support additional attribute - manifest file which will be merged with the ones extracted from input jars.

BTW @netdpb I guess I understand now how you came up with this idea :)

[bcorso]

Short-term, I think we'll want to go with option 2.

[netdpb]

Haha, yes. I should get back to that unfinished work.

…

On Mon, Mar 9, 2020 at 12:50 PM Grzegorz Nowak ***@***.***> wrote: At the moment jarjar_library is used for generation of shaded_grpc_server_processor and shaded_android_processor here <https://github.com/google/dagger/blob/master/BUILD#L56>. However the one that we are interested in is deployed directly <https://github.com/google/dagger/blob/master/util/execute-deploy.sh#L151> : deploy_library \ java/dagger/libcore.jar \ Even if we would process it with jarjar it wouldn't help because it doesn't support manifest merging <https://github.com/pantsbuild/jarjar/blob/master/src/main/java/org/pantsbuild/jarjar/ManifestProcessor.java#L34>. Instead jarjar_library rule uses inlined bash script <https://github.com/google/bazel-common/blob/master/tools/jarjar/jarjar.bzl#L36> to do that: # Concatenate similar files in META-INF that allow it. mergeMetaInfFiles=(services/.* {merge_meta_inf_files}) for metaInfPattern in ${{mergeMetaInfFiles[@]}}; do for file in $(find META-INF -regex "META-INF/$metaInfPattern\\(~[0-9]*\\)?"); do original=$(echo $file | sed s/"~[0-9]*$"//) if [[ "$file" != "$original" ]]; then cat $file >> $original rm $file fi done done Again we have few options here: 1. Extend jarjar to properly support manifest merging and customizations. Update bazel build rule in the process. Not sure which one though (google's <https://github.com/google/jarjar>, pants' <https://github.com/pantsbuild/jarjar> or shevek's <https://github.com/shevek/jarjar>. (The last one seems the most updated) 2. Update jarjar_library rule to support additional attribute - manifest file which will be merged with the ones extracted from input jars. BTW @netdpb <https://github.com/netdpb> I guess I understand now how <https://github.com/google/bazel-common/blob/master/tools/jarjar/jarjar.bzl#L25> you came up with this idea :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1079?email_source=notifications&email_token=ABA3LSPK4BLTFWWN2B6MTWLRGUM5JA5CNFSM4ESJZIYKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEOIBZFA#issuecomment-596647060>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABA3LSMUA5LNC3HR2DXC2C3RGUM5JANCNFSM4ESJZIYA> .

[rovarga]

Any progress on this? :-)
Even if not on the implementation side, has the module name been agreed to be 'dagger'?

[bcorso]

Yeah, I think we still need to decide on module names. Based off #1079 (comment), it seems like dagger is the correct module name for the com.google.dagger:dagger artifact.

Going through each of our artifacts, I've listed the likely java module names in the table below. If these seem reasonable, I can try to add these to the META-INF as suggested in #1079 (comment)

Artifact Name	Java Module Name
dagger	dagger
dagger-android	dagger.android
dagger-android-jarimpl	dagger.android
dagger-android-legacy	dagger.android
dagger-android-processor	dagger.android.processor
dagger-android-support	dagger.android.support
dagger-android-support-jarimpl	dagger.android.support
dagger-android-support-legacy	dagger.android.support
dagger-grpc-server	dagger.grpc.server
dagger-grpc-server-annotations	dagger.grpc.server
dagger-grpc-server-processor	dagger.grpc.server.processor
hilt-android	dagger.hilt.android
hilt-android-testing	dagger.hilt.android.testing
hilt-android-gradle-plugin	dagger.hilt.android.plugin
hilt-compiler	dagger.hilt.processor
hilt-android-compiler	(deprecated) dagger.hilt.android.processor
dagger-compiler	dagger.internal.codegen
dagger-lint	dagger.lint
dagger-lint-aar	dagger.lint
dagger-producers	dagger.producers
dagger-spi	dagger.spi

[jodastephen]

Just something to bear in mind is that you can't have two modules with the same name loaded at the same time. The list above suggests you have split packages, where the same package appears in two different jar files. If that is the case, then you can't really modularise as is.

(The exception to that rule is if the user is expected to choose one of the artifacts, eg one of"dagger-android-jarimpl", "dagger-android-legacy" or "dagger-android-processor".)

[rovarga]

Right, but the most critical is com.google.dagger:dagger, as that is the primary interface to other modules. Once that has a stable name, people can start publishing components referring to it.

[metteo]

@rovarga it's not that simple. Once a project claims a module name, it's responsible for its contents and compatibility. If dagger is too eager, it may cause split package problem which will be hard to solve. Most of the projects from pre-JPMS era will require some API package refactoring to solve this, meaning major version release.

[rovarga]

@metteo True, but that needs analysis of whether com.google.dagger:dagger split packages against all the others. If it does not, it is fine.
Nothing changes in the class/modulepath world, so it should be pretty safe. If there is functionality present which should be in a separate module, dagger.jar can deal with it in JPMS through a 'requires transitive dagger.this.should.have.been.elserwhere;'.

[rjbaucells]

Any progress on this?

[Chang-Eric]

Took another look at this and I think what might make sense is for us to add the automatic name entry in just the main dagger artifact to claim the module name dagger. I think this would solve most of the use cases without committing us too much in terms of module structure where we might need to evaluate our situation with respect to split packages.

I'll work on adding this entry assuming that sounds reasonable.