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

LUCENE-9849: Don't cross-link between modules for interim snapshot builds. #21

Merged
merged 1 commit into from Mar 18, 2021

Conversation

dweiss
Copy link
Contributor

@dweiss dweiss commented Mar 17, 2021

This isn't ideal - I think the root property luceneDocUrl should be moved to render-javadoc, but it's a start?

Copy link
Member

@rmuir rmuir left a comment

Choose a reason for hiding this comment

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

I'm not a gradle expert, but from javadocs side, this logic looks good

@dweiss
Copy link
Contributor Author

dweiss commented Mar 18, 2021

Hey, @uschindler do you want to take a peek? Or should I just commit this in and we can follow-up with other improvements later on?

Copy link
Contributor

@uschindler uschindler left a comment

Choose a reason for hiding this comment

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

Looks fine.
I was too tired yesterday to do this. I was also stuck on the RenderJavaDocsTask property with the link not able to handle null correctly.

This is exactly what I expected. Those code here should be done in same way on the Solr, side. But as code heavy diverges, we should make a separate PR.

// Add offline links.
allOfflineLinks.each { url, dir ->
// Some sanity check/ validation here to ensure dir/package-list or dir/element-list is present.
if (!project.file("$dir/package-list").exists() &&
!project.file("$dir/element-list").exists()) {
throw new GradleException("Expected pre-rendered package-list or element-list at ${dir}.")
}
logger.lifecycle("Linking ${url} to ${dir}")
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be a lower log level, you used lifecycle to actually see output, right?

@@ -284,7 +282,8 @@ class RenderJavadocTask extends DefaultTask {
def offlineLinks = [:]

@Input
def luceneDocUrl = "${-> project.luceneDocUrl }"
@Optional
Property<String> luceneDocUrl = project.objects.property(String)
Copy link
Contributor

Choose a reason for hiding this comment

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

How to do this part I was missing yesterday (my javadocs then contained the string "null" as URL..... Property is the one I was looking for! Great! Much better than before.

} else {
// For absolute links, we determine the target URL by assembling the full URL (if base is available).
def value = luceneDocUrl.getOrElse(null)
if (value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

great! My code yesterday looked similar!

@dweiss dweiss merged commit ca3de30 into apache:main Mar 18, 2021
gsmiller pushed a commit to gsmiller/lucene that referenced this pull request Mar 19, 2021
gsmiller pushed a commit to gsmiller/lucene that referenced this pull request Mar 19, 2021
gsmiller pushed a commit to gsmiller/lucene that referenced this pull request Mar 19, 2021
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.

None yet

3 participants