Skip to content

Conversation

@EntilZha
Copy link
Contributor

Pull request to accompany: https://issues.apache.org/jira/browse/SPARK-4543

Javadoc is missing from network-common causing publish-local to fail executing to completion, most importantly publishing the jar locally. This pull request adds placeholder TODO comments to allow the Javadoc to compile, thus allowing publish-local to complete successfully.

@AmplabJenkins
Copy link

Can one of the admins verify this patch?

@srowen
Copy link
Member

srowen commented Nov 21, 2014

No, that's really not a solution to put in a bunch of dummy TODOs. The Javadoc messages are just warnings. I do not see why just the network common module ends up showing them as errors in the SBT build. network/shuffle does not -- they're just warnings. There must be a simple config difference, although I don't see it.

@vanzin
Copy link
Contributor

vanzin commented Nov 21, 2014

Maybe related to 293672c4? Although that should apply for all projects...

@srowen
Copy link
Member

srowen commented Nov 21, 2014

Hm, I doubt it, but don't know 100%. That change should affect how Java code gets generated from Scala code for purposes of generating one big set of docs. The "errors" here are on non-generated Java source code.

@EntilZha
Copy link
Contributor Author

In sbt they are warnings, so I suppose the question is as you posed, what configuration is different. I have scoured through the directories/config files + search on github and can't find any diff that would be causing this. For the moment my temporary fix is TODO filler tags so i can continue working on my project, but I agree, that it is not the optimal/right way to fix this.

@aarondav
Copy link
Contributor

We actually don't want these methods to be exposed in Javadoc right now,
none of them are actually public. Java simply does not expose the
fine-grained visibility that Scala does. Would it fix the issue if we could
just not generate docs for these packages?

On Fri, Nov 21, 2014 at 3:00 PM, Pedro Rodriguez notifications@github.com
wrote:

In sbt they are warnings, so I suppose the question is as you posed, what
configuration is different. I have scoured through the directories/config
files + search on github and can't find any diff that would be causing
this. For the moment my temporary fix is TODO filler tags so i can continue
working on my project, but I agree, that it is not the optimal/right way to
fix this.


Reply to this email directly or view it on GitHub
#3405 (comment).

@EntilZha
Copy link
Contributor Author

In theory yes, I think that would fix the issue. What change would be needed for that?

@aarondav
Copy link
Contributor

No clue!

On Fri, Nov 21, 2014 at 6:11 PM, Pedro Rodriguez notifications@github.com
wrote:

In theory yes, I think that would fix the issue. What change would be
needed for that?


Reply to this email directly or view it on GitHub
#3405 (comment).

@srowen
Copy link
Member

srowen commented Nov 22, 2014

I stared at the SBT config for a while here, and I don't understand how it works. I see some bits that seem to exclude these packages, but they're in the TestSettings section, not in the Unidoc section. Adding packages to them doesn't do anything. I don't see how to simply modify what packages are passed to javadoc. The javadoc settings seem to be in javacOptions. unidocAllSources seems to already exclude these packages but it's not working I guess.

SBT expert is needed here. Here's one area where Maven is much easier to grok and control I think.

Of course another 'workaround' is to actually try to fill in this javadoc meaningfully.

@aarondav
Copy link
Contributor

Regarding filling in the javadoc, why do we have to add @param and @return everywhere? Other Spark javadoc doesn't necessarily have this, and where it's not used it's frequently because it wouldn't be useful.

@srowen
Copy link
Member

srowen commented Nov 23, 2014

Yeah, I'm not clear why only this module is treating javadoc warnings as errors. Ideally we'd figure that out too. But strictly speaking it's just necessary to figure out how to not make javadoc for these classes now, but I don't know how yet. I don't think we should have to fill in all that javadoc in general; it's too much work. This shouldn't be required. But was just suggesting that it is a way to workaround this in the short term too!

@EntilZha
Copy link
Contributor Author

Agreed, requiring to fill in the javadoc is tedious (it was tedious just entering in TODOs, let alone real docs). Who might be someone good to ping that would know more about this?

@ueshin
Copy link
Member

ueshin commented Nov 25, 2014

Hi, is this related to #3058?

@srowen
Copy link
Member

srowen commented Nov 25, 2014

I don't think so in the sense that it only affects Java 8, and should turn off errors for missing javadoc. Here, the javadoc messages are indeed just warnings, but SBT shows them as errors. Just for one module.

@ueshin
Copy link
Member

ueshin commented Nov 26, 2014

Hmm, I can't reproduce the situation that the javadoc messages are just warnings.
In my environment, the build succeeds in Java7 or fails with javadoc error messages in Java8.

@srowen
Copy link
Member

srowen commented Nov 26, 2014

I think we should add @ueshin 's patch at #3058 which should properly disable javadoc 8 missing javadoc warnings anyway. I think this is indeed Java 8-specific after all; I'm running Java 8.

@pwendell
Copy link
Contributor

@EntilZha are you building with Java 8? @srowen so did we decide this isn't just an instance of #3058

@srowen
Copy link
Member

srowen commented Nov 28, 2014

@pwendell There's a slightly different mystery here -- even if javadoc warnings are generated, why are they an error in just this module? and why is javadoc run at all for this? the SBT build looks like it's trying not to.

Successfully disabling doclint "missing" may just make it a moot point, and so 'fix' this, but it's a slightly different thing.

@EntilZha
Copy link
Contributor Author

@pwendell, I am building on Java 8.

@EntilZha
Copy link
Contributor Author

@srowen, definitely a good question. Looks like as #3058 was closed/merged in so this might be fixed now?

@pwendell
Copy link
Contributor

@EntilZha would you mind trying to build against the master branch and seeing if it works?

@srowen
Copy link
Member

srowen commented Nov 29, 2014

@pwendell @EntilZha It seems to work for me. I see fewer warnings, and no errors. The steps in the JIRA succeed for me.

@EntilZha
Copy link
Contributor Author

Works for me for building against master branch. Close PR and close issue with reference to #3058?

@pwendell
Copy link
Contributor

Yep - that's the best way to proceed. Close this PR, if you don't mind. I can update the JIRA.

@EntilZha EntilZha closed this Nov 30, 2014
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.

7 participants