-
Notifications
You must be signed in to change notification settings - Fork 243
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 publish workflow #1078
fix publish workflow #1078
Conversation
@armanbilge I will take a look at #1069 and try to send another PR. |
echo "$HOME/vendor/bundle/bin" >> $GITHUB_PATH | ||
- name: Build docs | ||
run: | | ||
sbt docs/makeMicrosite |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does sbt
come from? I think you usually need to install it with an action.
Instead of as a hand-written workflow, Do you think we can include this in the sbt build with sbt-gh-actions? Then most of the setup and compiling is done automatically. See e.g. https://github.com/typelevel/fs2/blob/main/build.sbt#L349
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does sbt come from?
ubuntu-latest has sbt installed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I choose a hand-written workflow because I have one concern. Is it possible for sbt-gh-actions to define multiple workflow
It is faster to publish docs when we use multiple workflows because workflow runs in parallel but steps do not run in parallel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I missed the e.g. link.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we can include this in the sbt build
Answer is "yes, we can" but do we really need that?
I think hand-written workflow is better when it comes to managing spire document because
- current publish task has little to do with Scala(only
mdoc
task), it is done byjekyll
andactions-gh-pages
. Wrapping them in sbt task does not seem straightforward. - I took a look
sbt-gh-actions
andsbt-gh-actions
focus mainly on making it easier to test, build and publish Scala sources over cross java/scala versions and cross target platforms(jvm/native/js) (though it is possible to add publish docs jobs).- In addition, if we include publish task in jobs, we have to introduce another action to detect file changes, which can be done by github workflow's path filter.
- some libraries in typelevel repo like
cats-effect
uses hand-written workflow for publishing docs. https://github.com/typelevel/cats-effect/blob/docs/.github/workflows/microsite.yml - publish docs workflow has a lifecycle different from CI workflow. Ci should run every PR or commit in order to validate changes, but publish docs workflow should run when microsite actually changes. They are different concern.
- separating document settings from build settings will keep build settings simple(and makes it easier to migrate to another document tools in the future.)
You say "setup and compiling is done automatically" but these features make little difference for publishing docs. We do not have to setup Java or Scala for publishing docs. We need setup jekyll
, envs and paths even if we use sbt-gh-actions
.
If we would publish docs for each spire versions over each Scala versions and each platforms it would be worth managing it in build.sbt, but current microsite is independent of Scala versions or platforms, just displaying information of the latest version, so the publish doc workflow is simple enough and will not be likely to swell further.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@i10416 thanks for the detailed research and write-up, this is great.
We do not have to setup Java or Scala for publishing docs.
Yes, this was my faulty assumption. Sorry about that.
publish docs workflow has a lifecycle different from CI workflow. Ci should run every PR or commit in order to validate changes, but publish docs workflow should run when microsite actually changes. They are different concern.
That's true, but we should build the docs as part of CI for PRs to make sure the PR didn't break the documentation somehow. E.g. maybe an example in the docs is using some code that changed, etc.
In addition, if we include publish task in jobs, we have to introduce another action to detect file changes, which can be done by github workflow's path filter.
IIUC publishing the docs also publishes the API docs: http://typelevel.org/spire/api/spire/index.html
If it doesn't, then it should. This can change even when there haven't been changes in the docs folder. So we should actually be careful to only publish the site when we release a new, stable version, I think, so that the docs don't get mismatched?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, you mean you want to programatically garantee docs are successfully built using 'if ....' or 'needs build' ? If so, it would be better to include the doc job in build.sbt.
Sorry, I assumed that docs are supposed to be successfully built everytime PR is merged to main (or version tag is set) because current workflow contains build docs step and you can be sure docs are vaild.
So we should actually be careful to only publish the site when we release a new, stable version
Ok, workflow becomes simpler and there's no need to check doc file changes as publishing documents happens only when stable release, not when doc files changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, just for my own clarification the docs should be built as part of every PR, to avoid breaking something. And as you point out, they already are. But of course, they should not be published from the PR :)
Ok, workflow becomes simpler and there's no need to check doc file changes as publishing documents happens only when stable release, not when doc files changes.
The downside to this is that docs contributions/updates are not published until the next release ... but, it prevents the API docs from getting out of sync with the published artifact, which could be very confusing. I'm open to ideas to improve this.
One way would be to publish the API docs under a version directory (e.g., 0.17, 0.18, SNAPSHOT). So this way, even though updating the site would also update the API docs, it would publish under SNAPSHOT instead of overwriting the API docs for the current.
For clarification, I'm distinguishing between the site/main docs (which should ideally be updated as frequently as possible) and the API docs aka scaladocs, which should match the published artifact.
Thoughts? Thanks again for your help with this, much appreciated!
Yes, it is good idea but unfortunately sbt microsite supports multi version publishing only with light theme. I think supporting multi-versons while keeping the site apperance needs a lot to do. I think migrating to laika (as http4s project is doing) may help, but it still needs amounts of work too. |
So, for now I will move workflow in sbt task and try to support multi versioned docsite in another PR later. |
Oh, I had a better idea for this. Let's keep your strategy of publishing the site every time the That way, it is a stable link to API docs matching the latest release, and users can even change the version there. What do you think? |
That is quite simpler! |
I'm happy to keep your workflow as-is here, you made a good case for keeping it simple. So we just need to update the link :) |
@armanbilge ping |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for persevering on this! LGTM, will be a big improvement.
I'm going to ask @cquiroz to have a look as well.
mdocIn := (Compile / sourceDirectory).value / "mdoc" | ||
mdocIn := (Compile / sourceDirectory).value / "mdoc", | ||
mdocVariables := Map( | ||
"VERSION" -> version.value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 this is great!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, thanks for making this work
@@ -65,7 +65,7 @@ JVM and JS platforms. | |||
To get started with SBT, simply add the following to your `build.sbt` file: | |||
|
|||
``` | |||
libraryDependencies += "org.typelevel" %% "spire" % "0.17.0" | |||
libraryDependencies += "org.typelevel" %% "spire" % "@VERSION@" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice
Darn, the publish job failed: It's not your fault here, it seems the docs are broken. I thought we are checking them in CI for each PR, but I guess we're not. |
Add workflow to publish latest document microsite because https://typelevel.org/spire/ does not show latest document.
After this PR
This is a preview hosted on my gh-page.
https://i10416.github.io/spire/