-
Notifications
You must be signed in to change notification settings - Fork 829
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
Introduce automatic module names into all libraries being built. #781
Conversation
Codecov Report
@@ Coverage Diff @@
## master #781 +/- ##
============================================
- Coverage 78.58% 78.55% -0.04%
+ Complexity 778 777 -1
============================================
Files 101 101
Lines 2779 2779
Branches 268 268
============================================
- Hits 2184 2183 -1
- Misses 493 494 +1
Partials 102 102
Continue to review full report at Codecov.
|
Is there any gradle magic we can do so we don't have repeat that jar section in every build.gradle? Some top level |
I wondered the same thing. My Gradle these days is weak, so I was hoping someone else had guidance, otherwise I'll go and research. |
It should be possible to re-use this part, although I don't remember myself now the specifics. Once you do this, we should be able to merge this PR. |
Also, please sign the CLA. :) Thanks very much for the contribution! |
I did sign the CLA today, not sure how long it takes to update... |
You might need to add a comment saying "I signed it" |
I signed it |
Guess not. Sometimes that kicks the CLAbot into action |
api/build.gradle
Outdated
manifest { | ||
attributes 'Automatic-Module-Name': moduleName | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing empty newline at the end of these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix this everywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added an empty new line to the root build.gradle, as it did not have one (prior to my involvement). All other files no longer have been touched at the end of the file, so they remain as they were.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution :)
I think we can group all these changes in the main build script. We could modify the jar.manifest section to:
jar.manifest {
ext.moduleName = "io." + project.name.replaceAll("-",".")
attributes('Implementation-Title': name,
'Implementation-Version': version,
'Automatic-Module-Name': moduleName,
'Built-By': System.getProperty('user.name'),
'Built-JDK': System.getProperty('java.version'),
'Source-Compatibility': sourceCompatibility,
'Target-Compatibility': targetCompatibility)
}
But I am not sure if "io." + project.name.replaceAll("-",".")
does all the magic correctly
I think we should centralise it but still explicitly set the module names. From my experience doing it as you suggest tends to lead to... Surprises after it is too late. But happy to follow consensus... |
With a quick test, I see that there is some difference between package structure and module name. Therefore, I agree with you that setting explicitly the module names is a better strategy on the long term. |
I managed to centralise the configuration into the root build.gradle. Please take another look and let me know of any further concerns. Thanks. |
LGTM. |
@@ -1,4 +1,5 @@ | |||
description = 'OpenTelemetry API' | |||
ext.moduleName = "io.opentelemetry.api" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't have "api" in the package names, does that matter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it doesn't matter. Module names do not need to relate to the package name, although they are a good starting point for choosing a good module name.
This is a proposal to fix #691, by adding automatic module names into the built manifest.mf files that ship within the jar files for each library. These names are all proposals and I am happy to change them as you suggest. Thanks.
Fixes #691