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

Fix function classpathArgs() #14559

Merged
merged 4 commits into from
Mar 23, 2022
Merged

Fix function classpathArgs() #14559

merged 4 commits into from
Mar 23, 2022

Conversation

michelou
Copy link
Contributor

(cc @SethTisue)
Both commands scaladoc and scaladoc.bat rely on function classpathArgs() to get their class path.

Thanks to a tiny difference in behaviour between bash script scaladoc and batch file scaladoc.bat (I wrote the batch file and I mostly work with Scala batch commands on MS Windows 10) I get an execution error due to the absence of library file ST4-4.0.7.jar in version 3.1.2-RC1. With my change on line 154 in batch file scaladoc.bat I do align its behaviour with its bash counterpart.

Important: This PR assumes that ST4-4.0.7.jar is not used anymore in versions 3.1.2-RC1 and newer.

PS. This issue is another example that underpins my motivation for the discussion External library management (started on October 22, 2021).

@michelou michelou added stat:fix available area:doctool area:tooling fasttrack Simple fix. Reviewer should merge or apply additional changes directly. labels Feb 24, 2022
@michelou michelou requested a review from romanowski February 24, 2022 19:55
@michelou
Copy link
Contributor Author

The CI error seems unrelated to the changes of this PR:

[...]
+ dist/target/pack/bin/scaladoc -d /tmp/tmp.6b3jw0jGAS -project 'scaladoc testcases' -source-links:out/bootstrap/stdlib-bootstrapped/scala-3.1.3-RC1-bin-SNAPSHOT-nonbootstrapped/src_managed/main/scala-library-src=github://scala/scala/v2.13.8#src/library -source-links:out/bootstrap/stdlib-bootstrapped/scala-3.1.3-RC1-bin-SNAPSHOT-nonbootstrapped/src_managed/main/dotty-library-src=github://lampepfl/dotty/3.0.0#library/src -source-links:github://lampepfl/dotty/3.0.0 '-external-mappings:.*scala/.*::scaladoc3::https://dotty.epfl.ch/api/,.*java/.*::javadoc::https://docs.oracle.com/javase/8/docs/api/' '-skip-by-regex:.+\.internal($|\..+)' '-skip-by-regex:.+\.impl($|\..+)' -project-logo docs/_assets/images/logo.svg -social-links:github::https://github.com/lampepfl/dotty,discord::https://discord.com/invite/scala,twitter::https://twitter.com/scala_lang -Ygenerate-inkuire -skip-by-id:scala.runtime.stdLibPatches -skip-by-id:scala.runtime.MatchCase -snippet-compiler:scaladoc-testcases/docs=compile -siteroot scaladoc-testcases/docs -project-footer 'Copyright (c) 2002-2022, LAMP/EPFL' -default-template static-site-main -author -groups -revision main -project-version 3.1.3-RC1-bin-SNAPSHOT out/bootstrap/scaladoc-testcases/scala-3.1.3-RC1-bin-SNAPSHOT-nonbootstrapped/classes 
[468](https://github.com/lampepfl/dotty/runs/5324445657?check_suite_focus=true#step:7:468)+ diff -rq /tmp/tmp.6b3jw0jGAS scaladoc/output/testcases 
[469](https://github.com/lampepfl/dotty/runs/5324445657?check_suite_focus=true#step:7:469)Files /tmp/tmp.6b3jw0jGAS/inkuire-db.json and scaladoc/output/testcases/inkuire-db.json differ 
[470](https://github.com/lampepfl/dotty/runs/5324445657?check_suite_focus=true#step:7:470)Error: Process completed with exit code 1.
[...]

@michelou michelou changed the title fixed function classpathArgs() Fix function classpathArgs() Feb 25, 2022
@michelou
Copy link
Contributor Author

@anatoliykmetyuk
Backport candidate to branch release-3.1.2.

@nicolasstucki nicolasstucki added the backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. label Mar 2, 2022
@pikinier20
Copy link
Contributor

Hmm the reason of this error is that classpath of script is different than classpath that is used to run Scaladoc in SBT. Because of that, there are differences in inkuire-db.json.

@michelou
Copy link
Contributor Author

michelou commented Mar 8, 2022

Hmm the reason of this error is that classpath of script is different than classpath that is used to run Scaladoc in SBT. Because of that, there are differences in inkuire-db.json.

Not sure you get the point : here we are talking about the Scala 3 shell scripts (resp. batch files on MS Windows) the end-user executes from the command prompt (nothing to do with sbt or inkuire-db.json).

Concretely we need function classpathArgs() (defined inside a bash script resp. a batch file) to return an up-to-date dependency list of external Java libraries to execute the main entry of scaladoc (built from source files in scaladoc/src/).

@michelou
Copy link
Contributor Author

michelou commented Mar 8, 2022

@pikinier20 Addendum (to be even more concrete) :
The dependency list of external Java libraries returned by classpathArgs() must be a valid subset of the JAR files located in directory lib/ of the Scala 3 software distribution, e.g. that list should be validated for each release (that check was not done with release 3.1.2-RC1).

@pikinier20
Copy link
Contributor

Hmm the reason of this error is that classpath of script is different than classpath that is used to run Scaladoc in SBT. Because of that, there are differences in inkuire-db.json.

Not sure you get the point : here we are talking about the Scala 3 shell scripts (resp. batch files on MS Windows) the end-user executes from the command prompt (nothing to do with sbt or inkuire-db.json).

Concretely we need function classpathArgs() (defined inside a bash script resp. a batch file) to return an up-to-date dependency list of external Java libraries to execute the main entry of scaladoc (built from source files in scaladoc/src/).

The test that is failing is comparing documentation of our test project generated with script and with sbt. The test fails so the output of script differs from the output of sbt task. The only thing that changed is classpath so I'm assuming that the classpath used in script is different from the classpath that is set in sbt when running Scaladoc task.

I think I've added the ST4 jar because of this test failure and it's because it's the dependency of ANTLR4. We probably don't use anything that needs it but sbt is fetching it and therefore we might want to leave it on classpath.

@michelou
Copy link
Contributor Author

michelou commented Mar 9, 2022

@Kordyjan @anatoliykmetyuk It's a pitty that this backport-nominated PR was not included into release 3.1.2-RC2.

PS. As Windows user I will have to manually patch batch file scaladoc.bat in my Scala 3 installations (as with 3.1.2-RC1).

@anatoliykmetyuk
Copy link
Contributor

Unfortunately, we can't merge PRs that don't pass the CI. According to what @pikinier20 says, it seems the CI failure is not spurious and is related to the PR. So it needs to be addressed first.

@michelou
Copy link
Contributor Author

I do not know anything about inkuire-db.json (generated by the external tool Inkuire I guess).
My PR is just about an obvious mismatch between the bin/ and lib/ directories of the Scala 3 distribution.
@ALL (@griggt ?!) Help is welcome.

dist/bin/scaladoc Outdated Show resolved Hide resolved
@michelou
Copy link
Contributor Author

@Kordyjan @anatoliykmetyuk Ok for backporting this tiny PR to 3.1.2 and 3.1.3 before their final version comes out ?!

PS. Thanks again to @griggt for his help.

@anatoliykmetyuk anatoliykmetyuk assigned Kordyjan and unassigned michelou Mar 23, 2022
@Kordyjan Kordyjan merged commit d9b3271 into scala:main Mar 23, 2022
@Kordyjan
Copy link
Contributor

It will definitely be included in 3.1.3-RC1.

@michelou michelou deleted the dotty-scaladoc branch April 23, 2022 08:23
@Kordyjan Kordyjan removed the backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. label Jul 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:doctool area:tooling fasttrack Simple fix. Reviewer should merge or apply additional changes directly. stat:fix available
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants