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

Add sbt-paradox docs #139

Merged
merged 1 commit into from
Mar 10, 2022
Merged

Add sbt-paradox docs #139

merged 1 commit into from
Mar 10, 2022

Conversation

mdedetrich
Copy link
Contributor

@mdedetrich mdedetrich commented Mar 10, 2022

About this change - What it does

This PR adds documentation using sbt-paradox

Resolves: #50 , #28 , #19

Why this way

The PR does quite a few changes many of which is already documented in the docs which this PR providers. Here are some additional notes

  • The documentation in some areas of Guardian had to be fixed/updated otherwise document generation fails when using scaladoc
  • Due to issues with paradox the oldest JDK version that Guardian now supports is Java 11. This is because -XX:+IgnoreUnrecognizedVMOptions doesn't work fully in JDK 8 and using -XX:+IgnoreUnrecognizedVMOptions is the only way to make sbt-paradox work with multiple JDK versions
  • Although initial support for sbt-paradox-project-info was included, its currently not being used because I am waiting for a release which fixes the following issue Make levels optional lightbend/sbt-paradox-project-info#18 (comment)
  • Changes to sbt-github-actions was done to firstly check that the documentation compiles on every PR and to also publish the documentation when a PR has been merged to master.

@mdedetrich mdedetrich marked this pull request as draft March 10, 2022 09:26
@coveralls
Copy link

coveralls commented Mar 10, 2022

Pull Request Test Coverage Report for Build 1962932619

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 76.676%

Totals Coverage Status
Change from base Build 1962928117: 0.0%
Covered Lines: 263
Relevant Lines: 343

💛 - Coveralls

@mdedetrich mdedetrich marked this pull request as ready for review March 10, 2022 12:07
@mdedetrich mdedetrich requested review from jlprat and reta March 10, 2022 12:07
@@ -0,0 +1,3 @@
-XX:+IgnoreUnrecognizedVMOptions
Copy link
Contributor

Choose a reason for hiding this comment

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

why do you need -XX:+IgnoreUnrecognizedVMOptions?

Copy link
Contributor Author

@mdedetrich mdedetrich Mar 10, 2022

Choose a reason for hiding this comment

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

In that file some options only apply to specific versions of JVM and by default if you pass an option into the JVM that doesn't exist it will immediately terminate. --illegal-access=permit only works on JVM 16 and --add-opens java.base/java.lang=ALL-UNNAMED only works on JVM 17

Copy link
Contributor

Choose a reason for hiding this comment

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

May be we could do better?

Why I am concerned: we should not silence invalid JVM command line options / flags, the users should be aware of any mismatches here.

Copy link
Contributor Author

@mdedetrich mdedetrich Mar 10, 2022

Choose a reason for hiding this comment

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

I thought of this originally but the issue here is the typical way/s that SBT is used. For example a lot of people (myself included) use Intellij's SBT integration when working with Scala projects. When using sbt like this the only possible way to pass JVM arguments is with .jvmopts since Intellij behind the scenes uses the sbt directly via a jar for its integration. The same applies to the official sbt launcher script which is included when you install sbt via your operating systems package manager (homebrew, apt, pacman etc etc).

Ideally sbt should have support for dynamically supporting different JVM options depending the JVM option but this doesn't exist yet. Note the complication here is that the JVM args are designed for the build tool, not the runtime of the project that gets compiled. Its trivial for SBT to support different JVM arguments when you run tests/programs compiled by SBT (sbt supports forking the JVM with different JVM args when running tests) but in this case the JVM arguments are actually needed for the build tool itself (the document generation is done via a SBT plugin)

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 all set!

@mdedetrich mdedetrich merged commit 08cab88 into main Mar 10, 2022
@mdedetrich mdedetrich deleted the add-docs branch March 10, 2022 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document how configuration for storage backends work
3 participants