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

Don't execute scaladoc for publishLocal #3537

Open
jvican opened this issue Sep 15, 2017 · 14 comments
Open

Don't execute scaladoc for publishLocal #3537

jvican opened this issue Sep 15, 2017 · 14 comments
Labels
area/scripted scripted plugin

Comments

@jvican
Copy link
Member

jvican commented Sep 15, 2017

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
Copy link
Member

eed3si9n commented Sep 15, 2017

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.

  • Scaladoc might fail for whatever reason.
  • Scaladoc might be needed by IDEs.

@jvican
Copy link
Member Author

jvican commented Sep 15, 2017

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
Copy link
Member

eed3si9n commented Sep 15, 2017

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.

  • Why? It's slow because we are forced to use "-SNAPSHOT".
  • Why is it slow: -SNAPSHOT is really slow because it checks all the resolvers, including the Internet (Maven, repo.typesafe.com redirect to Bintray etc).
  • Why is it using -SNAPSHOT: because we are publishing into "local" and we need to tell Ivy that the artifact may have changed since the last run.

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.

@jvican
Copy link
Member Author

jvican commented Sep 15, 2017

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
Copy link
Member

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
Copy link
Member Author

jvican commented Sep 15, 2017

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
Copy link
Member

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
Copy link
Member Author

jvican commented Sep 15, 2017

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.

@eed3si9n eed3si9n added the uncategorized Used for Waffle integration label Sep 18, 2018
@bjornregnell
Copy link

@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
Copy link
Member Author

jvican commented Mar 3, 2019

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

@bjornregnell
Copy link

bjornregnell commented Mar 5, 2019

@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

@eed3si9n eed3si9n added area/scripted scripted plugin and removed uncategorized Used for Waffle integration labels Oct 2, 2019
@eatkins
Copy link
Contributor

eatkins commented Jan 21, 2020

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
Copy link
Member

mkurz commented Mar 5, 2020

Setting

publishArtifact in (Compile, packageDoc) := false

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

@ches
Copy link
Contributor

ches commented May 7, 2020

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.

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

No branches or pull requests

6 participants