scala / bug

Scala 2 bug reports only. Please, no questions — proper bug reports only.
https://scala-lang.org
232 stars 21 forks source link

scaladoc 2.13.12 `-doc-source-url` behavior change #12867

Closed armanbilge closed 10 months ago

armanbilge commented 1 year ago

Changed in https://github.com/scala/scala/pull/10502

Reproduction steps

// /workspace/sandbox/foo.scala
class Foo
scaladoc -doc-source-url 'http://somewhere€{FILE_PATH}.scala' /workspace/sandbox/foo.scala

Problem

In 2.13.11 we get source links like:

<a href="http://somewhere/workspace/sandbox/foo.scala" target="_blank">foo.scala</a>

In 2.13.12 we get:

<a href="http://somewherefoo.scala" target="_blank">foo.scala</a>

h/t @Jasper-M, cc @j-mie6

som-snytt commented 1 year ago

Probably due to URI usage whack-a-mole. I don't know what -doc-source-url does, but I am confident there is minimal testing.

som-snytt commented 1 year ago

I see

image

<a href="http://somewhere/tmp/sandbox/foo.scala" target="_blank">foo.scala</a>

after

scaladoc -d target -doc-source-url 'http://somewhere€{FILE_PATH}.scala' /tmp/sandbox/foo.scala

➜  t12867 scaladoc -version
Scaladoc version 2.13.12 -- Copyright 2002-2023, LAMP/EPFL and Lightbend, Inc.
armanbilge commented 1 year ago

I just tried again and I'm still getting the same result as above, even with -d target.

In case it makes a difference.

Linux armanbilge-sandbox-9vtqm47lh3h 6.1.44-060144-generic #202308111259 SMP PREEMPT_DYNAMIC Fri Aug 11 14:20:00 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux
openjdk version "11.0.20" 2023-07-18 LTS
OpenJDK Runtime Environment Zulu11.66+15-CA (build 11.0.20+8-LTS)
OpenJDK 64-Bit Server VM Zulu11.66+15-CA (build 11.0.20+8-LTS, mixed mode)
j-mie6 commented 1 year ago

Can confirm, I've ran the same commands as Arman and you:

scaladoc -d target -doc-source-url 'http://somewhere€{FILE_PATH}.scala' ~/sandbox/foo.scala

on 2.13.11 it gives the right link and on 2.13.12 it gives the wrong one, exactly as Arman described.

For completeness, I used cs install scaladoc:2.13.12 to switch to the 2.13.12 scaladoc

som-snytt commented 1 year ago

For some reason, I need to specify -sourcepath /tmp to reproduce.

som-snytt commented 1 year ago

The previous bug in 2.13.11 is that it strips the prefix -sourcepath (as a string value) from the file path.

scaladoc -sourcepath /tmp/sand -d target -doc-source-url 'http://somewhere€{FILE_PATH}.scala#€{FILE_LINE}' /tmp/sandbox/foo.scala

results in

<a href="http://somewherebox/foo.scala#3" target="_blank">foo.scala</a>

but first, trailing / is stripped from -sourcepath:

scaladoc -sourcepath /tmp/ -d target -doc-source-url 'http://somewhere€{FILE_PATH}.scala#€{FILE_LINE}' /tmp/sandbox/foo.scala

in order to preserve the / before the relative path

<a href="http://somewhere/sandbox/foo.scala#3" target="_blank">foo.scala</a>

Note the help:

➜  t12867 scaladoc -help
Usage: scaladoc <options> <source files>

where possible scaladoc options:
***
  -doc-source-url <url>                       A URL pattern used to link to the source file, with some variables supported: For example, for `scala.collection.Seq` €{TPL_NAME} gives `Seq`, €{TPL_OWNER} gives `scala.collection`, €{FILE_PATH} gives `scala/collection/Seq`, €{FILE_EXT} gives `.scala`, €{FILE_PATH_EXT} gives `scala/collection/Seq.scala`, and €{FILE_LINE} gives `25` (without the backquotes). To obtain a relative path for €{FILE_PATH} and €{FILE_PATH_EXT} instead of an absolute one, use the -sourcepath setting.

One may take the liberty of reformatting:

  -doc-source-url <url>

A URL pattern used to link to the source file, with some variables supported:

For example, for `scala.collection.Seq`

€{TPL_NAME} gives `Seq`,
€{TPL_OWNER} gives `scala.collection`,
€{FILE_PATH} gives `scala/collection/Seq`,
€{FILE_EXT} gives `.scala`,
€{FILE_PATH_EXT} gives `scala/collection/Seq.scala`, and
€{FILE_LINE} gives `25` (without the backquotes).

To obtain a relative path for €{FILE_PATH} and €{FILE_PATH_EXT} instead of an absolute one, use the -sourcepath setting.

It was just a bug to retain the leading '/' in a path that ought to be relative.

In 2.13.12, URI computes the relative path, instead of the old string arithmetic.

The two differences are that -sourcepath must be a path (not just an arbitrary string prefix) and the -doc-source-url must include a '/' if that is desired in front of a relative path (where -sourcepath determines relative paths).

scaladoc -sourcepath /tmp/sandbox -d target -doc-source-url 'http://somewhere/€{FILE_PATH_EXT}#€{FILE_LINE}' /tmp/sandbox/foo.scala

Note that the FILE_PATH_EXT token includes the "file extension" in the example.

SethTisue commented 1 year ago

I've made a note to myself to add this to the 2.13.12 release notes, as it's very easy to be affected; I just realized that our own published Scaladoc is affected. I just opened #12875 on that

we have officially been hoisted by our own petard and we shot ourselves in the foot as we ate our own dogfood

SethTisue commented 1 year ago

added to the 2.13.12 notes:

Scaladoc tool changes

Library authors should be aware that the behavior of -doc-source-url has changed, as follows:

You may need to adjust your build accordingly, to avoid generating broken source links.

armanbilge commented 1 year ago

For reference, here's how we fixed it in sbt-typelevel. https://github.com/typelevel/sbt-typelevel/pull/643/files#diff-0314694cc89c600666817f8d964cb769c7db8f921b9cc76686c0ee3b6f88c8aaR96-R98

- s"${info.browseUrl}/blob/${vh}€{FILE_PATH}.scala"
+ s"${info.browseUrl}/blob/${vh}/€{FILE_PATH}.scala"
SethTisue commented 1 year ago

@som-snytt if we had seen this coming, I think we would have sought to avoid inconveniencing library authors. and now that we know, if we had some other reason to rush a 2.13.13 out, then that would have been an opportunity to find a way for library authors not to need to change their builds..... but as it turns out, we don't have any other reason to rush 2.13.13, so it seems this change is a fait accompli

regardless, we could still consider changes in an eventual 2.13.13 release. I suspect many library authors simply won't notice that their Scaladoc has broken links in it. can we fix it for them?

Lukas is wondering if we could make both old (slashless) and new (slashful) style configs work? perhaps by inserting the slash automatically if it isn't already present? wdyt, does this make any sense?

som-snytt commented 1 year ago

@SethTisue yes, I was also thinking about better ergonomics, because the scaladoc -help doesn't make much sense to me.

I intended, too, to test normalization of root//path.

The reason to release 2.13.13 on Friday the 13th is because our luck could not get worse.

som-snytt commented 1 year ago

Dotty equivalent where the "source link" is not a special scheme such as github:

~/projects/dotty/bin/scaladoc -d target -source-links:/tmp/=http://somewhere/€{FILE_PATH}.scala#€{FILE_LINE} /tmp/sandbox/C.tasty

The leading "sub-path" is the path prefix to relativize. The file path is always rendered with leading slash, so adding the slash as shown results in a doubled slash.

The mitigation for Scaladoc 2 will be to ensure a separator. somewhere/€{FILE_PATH_EXT} vs somewhere€{FILE_PATH_EXT} will be at the discretion of anyone who notices the difference.

Wise words from Dotty:

We should really merge tests for source links to prevent further regressions

som-snytt commented 1 year ago

Confirming that Akka has always had double-slash in source links.

image

https://doc.akka.io/api/akka/current/akka/actor/AbstractActor.html

https://github.com/apache/incubator-pekko/blob/main/project/Doc.scala#L54

Noticed while wondering what is "-doc-canonical-base-url", which was added for Akka. There are still a couple of checkboxes at

https://github.com/akka/akka.io/issues/623