sbt / sbt

sbt, the interactive build tool
https://scala-sbt.org
Apache License 2.0
4.81k stars 937 forks source link

Don't execute scaladoc for `publishLocal` #3537

Open jvican opened 7 years ago

jvican commented 7 years ago

steps

publishLocal always runs Scaladoc. I cannot think of any case where this is useful, I'm open to ideas.

problem

  1. Slows down publishLocal. 1.1. Relevant for sbt plugins. 1.2. Relevant for source dependencies (where jars are used for dependencies). 1.3. Relevant for any user that wants to test jars locally (pretty common in enterprises).

expectation

publishLocal does not execute Scaladoc.

Is this a welcome contribution?

eed3si9n commented 7 years ago

I think there's something wrong with the development process if it relies on publishLocal (including scripted) being fast.

It should be treated as the name suggest, a publishing step, so I think it makes sense to keep Scaladoc in there. The reason you might want to publish things locally could be for rehearsal of actual publishing or simply so one can use a library or a plugin locally. In both cases doc jar might be useful.

jvican commented 7 years ago

I think there's something wrong with the development process if it relies on publishLocal (including scripted) being fast.

I don't think anything relies on publishLocal being only fast, but doing the simplest possible task.

Regarding scripted, can you elaborate on how you would fix it without publishLocal being leaner? Any change done in sbt, or changing from one commit to another one, requires the following steps: compile, scaladoc, package all sources, publishLocal. scaladoc is basically another typechecking, and this makes testing sbt slow and requires sbt to use extra memory for nothing. It's essentially as you were compiling twice.

Aside from this question, I think it's becoming obvious I have a slightly different notion of publishing than you do, and I'm not sure we can reconcile them. Publishing for me is package + ship to ivy, not package any kind of resource (binaries, sources, and docs) and then ship them all to ivy. I do understand that this is the default behaviour and I'm not asking to change it in publish, but I think it requires reconsideration in publishLocal.

For such a case, I find it unlikely than users are going to look at the docs of their freshly published jars locally. Can you pinpoint any other cases where publishing docs locally would be useful? As you say " or simply so one can use a library or a plugin locally", this use case does not need docs to be published and the consumption is of binaries, neither the sources nor docs.

If you're not comfortable changing the behaviour of publishLocal, I think we should add an explicit method for a lightweight publish or add a new setting lightweight in publish to configure this behaviour.

However, let me note that I think sbt should fit the most common use case. And to be honest, I'm skeptical to believe that publishLocal of docs is a common use case. In my experience, it's rare.

eed3si9n commented 7 years ago

Regarding scripted, can you elaborate on how you would fix it without publishLocal being leaner?

If we step back, I think the larger problem is that scripted is slow and frustrating.

In other words, one of the core problem is isolation of changing artifact aka https://github.com/sbt/sbt/issues/1361. Instead of publishLocal if we had publishTemp where temp gets cleared out at the end, we can use normal version number and resolve quickly. Maybe publishTemp can skip docs too.

jvican commented 7 years ago

In other words, one of the core problem is isolation of changing artifact aka #1361. Instead of publishLocal if we had publishTemp where temp gets cleared out at the end, we can use normal version number and resolve quickly. Maybe publishTemp can skip docs too.

This may very well be the solution, but you're not solving the real problem, just sidestepping it.

What I propose addresses the root of the issue. I really don't want to compile my project twice every time I do publishLocal just because there is one scenario that happens less than 1% of the time. Therefore, my proposition of the changes in the semantics of publishLocal, or lightweight in publish.

Remember that there are legit use cases for this. The compiler bridge is one (jar has to be published in order to be consumed) and I definitely not need the docs of the bridge just to test it. This is independent from scripted.

eed3si9n commented 7 years ago

For such a case, I find it unlikely than users are going to look at the docs of their freshly published jars locally. Can you pinpoint any other cases where publishing docs locally would be useful? As you say " or simply so one can use a library or a plugin locally", this use case does not need docs to be published and the consumption is of binaries, neither the sources nor docs.

I don't use IDEs much, but don't they display documentation when you hover over? If someone wants to grab unreleased version of library X and use it as a normal library, I think it makes sense to have it identical setup as getting it from Maven Central.

jvican commented 7 years ago

I don't use IDEs much, but don't they display documentation when you hover over?

Not that I know of. I use Intellij a lot, I've never seen such a use case.

If someone wants to grab unreleased version of library X and use it as a normal library, I think it makes sense to have it identical setup as getting it from Maven Central.

Yes, but what I'm asking here is under which scenario a "user would like to grab an unreleased version of library X and use it as a normal library". Please, elaborate, because I don't see any legit scenario for such an action. It can certainly happen, but my point is that if there's not a good reason for it we should not bless such a use case at the cost of the majority of the use cases.

eed3si9n commented 7 years ago

Apparently it's Ctrl+Q (or Ctrl+J in older version) on IntelliJ - https://stackoverflow.com/questions/13781990/setting-up-scaladoc-for-intellij, and hovering on ScalaIDE http://scala-ide.org/blog/release-notes-4.0.0-RC1.html.

In any case, anything involving "local" might involve some degree of personalized tooling and taste. I am open to exploring ideas that can support various use cases. For example, we could think of global level flag that you could set that would affect publishLocal in all builds.

jvican commented 7 years ago

I would like to highlight that this problem does not only affect tools like scripted only, but also partest, which is used across both scalac and dotty. In general, I think the use of publishLocal is legit when it comes to integration testing.

In any case, anything involving "local" might involve some degree of personalized tooling and taste. I am open to exploring ideas that can support various use cases. For example, we could think of global level flag that you could set that would affect publishLocal in all builds.

This would be the lightweight in publish I was referring to before.

Apparently it's Ctrl+Q (or Ctrl+J in older version) on IntelliJ - https://stackoverflow.com/questions/13781990/setting-up-scaladoc-for-intellij, and hovering on ScalaIDE http://scala-ide.org/blog/release-notes-4.0.0-RC1.html.

I was unaware of this. For the record, I do not think most of the users should pay this cost because a minority uses Scaladoc on their IDEs. They should be the ones enabling it in their build tools, not me disabling it.

bjornregnell commented 5 years ago

@jvican This line in my build.sbt helped me prevent generating docs when publishLocal:

publishArtifact in (Compile, packageDoc) := false

Tested with dotty nightly and sbt 1.2.7

jvican commented 5 years ago

@bjornregnell Yeah, this ticket is about making that the default behavior.

bjornregnell commented 5 years ago

@jvican Aha. Yes that default behaviour makes sense. I think the docs should be updated anyhow to include the above setting example with true or false depending on what's the default Here I guess: https://www.scala-sbt.org/1.x/docs/Publishing.html

publishArtifact in (Compile, packageDoc) := true
eatkins commented 4 years ago

In sbt, we've worked around this by adding the publishLocalBin task which more or less duplicates publishLocal but with the docs excluded. I don't have an exact memory of what went wrong but I do know that I ran into problems with scripted when I tried to remove docs entirely from publishLocalBin. The workaround looks like:

dummyDoc := {
  val dummyFile = streams.value.cacheDirectory / "doc.jar"
  try {
    Files.createDirectories(dummyFile.toPath.getParent)
    Files.createFile(dummyFile.toPath)
  } catch { case _: FileAlreadyExistsException => }
  dummyFile
},
dummyDoc / packagedArtifact := (Compile / packageDoc / artifact).value -> dummyDoc.value,
packagedArtifacts in publishLocalBin :=
  Classpaths
    .packaged(Seq(packageBin in Compile, packageSrc in Compile, makePom, dummyDoc))
    .value

If I recall correctly, sbt could not load itself without the dummy docs because sbt would try to resolve the docs as part of metabuild resolution.

That being said, I would imagine that for the vast majority of projects there is no reason to publish the docs locally by default. If one wants to look at the local artifacts, one can just load them from the target directory.

mkurz commented 4 years ago

Setting

publishArtifact in (Compile, packageDoc) := false

will also prevent docs from being published when using publish. This is maybe not what you want (?)

ches commented 4 years ago

It should be treated as the name suggest, a publishing step, so I think it makes sense to keep Scaladoc in there. The reason you might want to publish things locally could be for rehearsal of actual publishing

I can agree with this, for a piece of anecdata, today I had publishing fail for one cross version of one module in a build, because scaladoc caused scalac to fail (not just a bad link, a more fun scala.reflect.internal.FatalError: bad constant pool tag, but I digress—the details are not relevant here).

Is that a common enough case to block a default of a common workflow? Maybe not. Would I have a publishLocal step in a CI environment just to fail and prevent incomplete remote publishing in a case like this? I wouldn't have considered it before, but now I actually might, unless sbt someday supports atomic multi-module publishing when aggregating.

So none of this is to say that there would be no value in a form of "light mode" or intuitive setting like skipDoc in publishLocal, but I do like the confidence that comes with publishLocal and publishM2 staying close to "full fidelity" by default.