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

Import sources for javadoc through a dependency #1461

Merged
merged 3 commits into from
Apr 10, 2020

Conversation

jbaiera
Copy link
Member

@jbaiera jbaiera commented Apr 9, 2020

We embed project code from the MR project into each and every integration Project in ES-Hadoop. When generating javadocs, we directly reference the mr project's source sets to pull their contents into the javadoc tool as an input.

This becomes a problem when a downstream project depends on java source code that is generated by a task. A specific instance of this problem arises when splitting the Spark core and SQL projects apart. The Spark projects take advantage of a compiler plugin to generate java sources specifically to be used with the Javadoc tool. These generated sources must be transmitted from the Spark core projects to the Spark SQL projects so that they may be bundled together in the same documentation package.

This PR adds configurations for importing and exporting source directories. It adds a sourceElements configuration that can be consumed by other projects. The build plugin then registers the main java source directories in the project with this configuration through the artifacts handler. The PR also adds a sources configuration that can be resolved. Projects use this configuration to denote source code directories from other projects that are needed to generate the Javadocs and Sources jar.

Add configurations for importing and exporting source directories Add a
sources configuration to add another project's source to the current
project's javadoc. Add a sourceElements configuration to register all
java source directories in the project under so they may be picked up
by other projects.
@@ -200,6 +235,15 @@ class BuildPlugin implements Plugin<Project> {
project.sourceCompatibility = '1.8'
project.targetCompatibility = '1.8'

// TODO: Remove all root project distribution logic. It should exist in a separate dist project.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a problem that I'm working on addressing in another PR. The root project is all sorts of messed up because it depends (without using dependencies in a lot of cases 😱) on all the subprojects to build an uber jar. This uberjar/dist zip should be generated in a subproject instead of the root project, using dependencies, configurations, and all other appropriate build script hygiene to resolve the cross project links.

// Import source configuration
Configuration sources = project.configurations.create("sources")
sources.canBeConsumed = false
sources.canBeResolved = true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is be false too ? the sources shouldn't be resolved since they should only be declared as a single project reference ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, gradle does not seem to like this. In order for Gradle to convert the project dependency into actual files it needs to resolve the configuration. It needs to do this for the sources jar, and any time a file collection that contains the configuration is mutated.

if (project != project.rootProject) {
SourceSet mainSourceSet = project.sourceSets.main
FileCollection javaSourceDirs = mainSourceSet.java.sourceDirectories
javaSourceDirs.each { File srcDir ->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this loop correct ? I would have expected to see something like:

project.getArtifacts().add('sourceElements', mainSourceSet.java.sourceDirectories)

(no loop)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe that will work since sourceDirectories is a FileCollection. The ArtifactHandler.add() doesn't accept collections. You need to add the directories individually.

That said, because of this, we are open to configuration ordering issues here. So any additional source directories need to be added before this code runs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I wasn't able to find anything on FileCollections that allows you to listen for changes like other domain object collections. I think anything outside of the default java directory needs to be added to the source set explicitly, and thus should be getting added to the sourceElements configuration explicitly as well.


// Export generated Java code from the genjavadoc compiler plugin
artifacts {
sourceElements(project.file("$buildDir/generated/java")) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does adding the sourceElements here conflict with plugin addition ? ( I don't know if it is additive or a replacement)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't adding a configuration. It's adding an artifact to the existing sourceElements configuration.

Copy link
Member Author

@jbaiera jbaiera Apr 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what you mean by this question. Do you mean the scala compiler plugin or the build plugin?

Edit: Github did not refresh before Mark answered :P

Copy link
Contributor

@jakelandis jakelandis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. A couple questions, mostly to help clarify my understand.

I think call the configuration additionalSources will help alot with readability.

Copy link
Contributor

@mark-vieira mark-vieira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

if (project != project.rootProject) {
SourceSet mainSourceSet = project.sourceSets.main
FileCollection javaSourceDirs = mainSourceSet.java.sourceDirectories
javaSourceDirs.each { File srcDir ->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe that will work since sourceDirectories is a FileCollection. The ArtifactHandler.add() doesn't accept collections. You need to add the directories individually.

That said, because of this, we are open to configuration ordering issues here. So any additional source directories need to be added before this code runs.


// Export generated Java code from the genjavadoc compiler plugin
artifacts {
sourceElements(project.file("$buildDir/generated/java")) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't adding a configuration. It's adding an artifact to the existing sourceElements configuration.

sources -> additionalSources
@jbaiera jbaiera merged commit 259ed4d into elastic:master Apr 10, 2020
@jbaiera jbaiera deleted the feature-source-configuration-sharing branch April 10, 2020 19:50
jbaiera added a commit that referenced this pull request May 13, 2020
Add configurations for importing and exporting source directories Add an
additionalSources configuration to add another project's source to the
current project's javadoc. Add a sourceElements configuration to register
all java source directories in the project under so they may be picked up
by other projects.
@jbaiera
Copy link
Member Author

jbaiera commented Jul 14, 2020

Relates #1423

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants