Skip to content

Scaladoc: realign -doc-source-url behavior with 2.12 #10581

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.

Sign up for GitHub

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

Jump to bottom
Merged
lrytz merged 1 commit into from
Nov 23, 2023
Merged

Scaladoc: realign -doc-source-url behavior with 2.12 #10581

lrytz merged 1 commit into from
Nov 23, 2023

Conversation

@lrytz
Copy link
Member

@lrytz lrytz commented Oct 20, 2023 •
edited
Loading

Summary

  • In the common case, build tools pass absolute paths to the compiler for source files to compile
  • -sourcepath is used to relativize them
  • In 2.12.11, the relativized path starts with a /, so base€{FILE_PATH_EXT} becomes base/src/...
  • #10502 changed relativization to use srcpathUri.relativize(fileUri), which doesn't have a leading /
  • Fix: for a relative path, foo/€{FILE_PATH} and foo€{FILE_PATH} both emit foo/relative/path/File (insert / if a word character precedes €{FILE_PATH}). No such magic for absolute paths (if -sourcepath is not specified or is not a prefix of the file).

What happens without -sourcepath:

  • 2.13.11 used the path that the compiler got on the command line. So scaladoc a/B.scala would insert a relative path.
  • 2.13.12 relativizes source paths to the current directory because Paths.get("").toUri is the current directory
  • I changed it to use an absolute path. As build tools pass absolute paths to the compiler, the behavior is the same as in 2.13.11 in the common case.

Fixes scala/bug#12867

@scala-jenkins scala-jenkins added this to the 2.13.13 milestone Oct 20, 2023
@som-snytt
Copy link
Contributor

som-snytt commented Oct 20, 2023 •
edited
Loading

I have a couple of branches. I'll comment if either contains a break-through.

  issue/12867-doc-url
  issue/12867-scaladoc-source-link

One was from Sept 13, the longer one from Sept 20, when apparently it occurred to me to fix Scaladoc tickets?

commit d25e06dc81cd60d0efb9ffaf9239b65f341dd2cb (HEAD -> issue/12867-scaladoc-source-link)
Author: Som Snytt <[email protected]>
Date:   Thu Sep 21 17:40:22 2023 -0700

    more

commit 714275ffd3ac81506dd5e15081107b1c9b38038e
Author: Som Snytt <[email protected]>
Date:   Wed Sep 20 20:48:09 2023 -0700

    Minor refactor for reading

commit 6b994a4f30ee0cb78a5f2a28b0c6279467c890f6
Author: Som Snytt <[email protected]>
Date:   Wed Sep 20 19:10:03 2023 -0700

    No inheritdoc on Iterator.sameElements

commit 51885feabfae5030b6c473db1bf463418b81d2d2
Author: Som Snytt <[email protected]>
Date:   Wed Sep 20 18:36:49 2023 -0700

    Library leverages canonical url

commit 2df1a5fff4b745102203e1d68c6e62c6552fd3f9
Author: Som Snytt <[email protected]>
Date:   Wed Sep 20 17:28:36 2023 -0700

    Clean up doclet newinstance, avoid null currentRun

@lrytz lrytz force-pushed the t12867 branch 3 times, most recently from 12db79c to 2ad345f Compare October 27, 2023 08:27
@lrytz lrytz marked this pull request as ready for review October 27, 2023 08:28
@lrytz lrytz requested a review from som-snytt October 27, 2023 08:28
@lrytz
Copy link
Member Author

lrytz commented Nov 21, 2023

@SethTisue could you review this? I summarized everything in the description.

@SethTisue SethTisue self-assigned this Nov 21, 2023
@SethTisue SethTisue added prio:hi high priority (used only by core team, only near release time) release-notes worth highlighting in next release notes labels Nov 21, 2023
SethTisue
SethTisue approved these changes Nov 22, 2023
View reviewed changes
Copy link
Member

@SethTisue SethTisue left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I've looked through this and I can't think of a flaw or fresh angle, so I'll hit "Approve", but @som-snytt, you analyzed the situation pretty thoroughly at scala/bug#12867 (comment) ; would be good to have your signoff before merge, if you can spare the time

also perhaps @armanbilge would like to have a look?

@SethTisue SethTisue removed their assignment Nov 22, 2023
armanbilge
armanbilge reviewed Nov 22, 2023
View reviewed changes
@som-snytt
Copy link
Contributor

som-snytt commented Nov 23, 2023

my test dirs from that day...

...    4096 Sep 20 13:38  t12876
...    4096 Sep 20 14:37  t12867

som-snytt
som-snytt reviewed Nov 23, 2023
View reviewed changes
som-snytt
som-snytt reviewed Nov 23, 2023
View reviewed changes
som-snytt
som-snytt reviewed Nov 23, 2023
View reviewed changes
som-snytt
som-snytt approved these changes Nov 23, 2023
View reviewed changes
Copy link
Contributor

@som-snytt som-snytt left a comment

Choose a reason for hiding this comment

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

I happen to be tired and lacking focus at the moment, so reading even this much regex is like, I'm not going to read any more regex until after New Year's.

@som-snytt
Copy link
Contributor

som-snytt commented Nov 23, 2023

NBD I tried the crusty way. https://github.com/scala/scala/compare/2.13.x...som-snytt:scala:test/t12867?expand=1

@lrytz
Copy link
Member Author

lrytz commented Nov 23, 2023

I'm all for a simpler regex, thanks a lot for your cleanup.

@lrytz lrytz enabled auto-merge November 23, 2023 09:59
@lrytz lrytz merged commit d4f2056 into scala:2.13.x Nov 23, 2023
@SethTisue SethTisue changed the title -doc-source-url compatibility with 2.12.11 Scaladoc: realign -doc-source-url behavior with 2.12 Feb 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@SethTisue SethTisue SethTisue approved these changes

@som-snytt som-snytt som-snytt approved these changes

+1 more reviewer

@armanbilge armanbilge armanbilge left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

prio:hi high priority (used only by core team, only near release time) release-notes worth highlighting in next release notes

Projects

None yet

Milestone

2.13.13

Development

Successfully merging this pull request may close these issues.

scaladoc 2.13.12 -doc-source-url behavior change

5 participants

@lrytz @som-snytt @SethTisue @armanbilge @scala-jenkins