Implement automatic HTML generation

[bruxisma]

Add automation via GitHub Actions to generate the specification HTML and deploy the same to GitHub Pages once merged.

Additionally, rework HTML generation to use Poetry^1 to set up and manage a virtual environment used to generate the HTML. This helps ensure that both local and automated builds are using a uniform environment.

Note that deployment eschews the upload-pages-artifact^2 action because the mechanism used here allows us to bundle the files and correct their permissions in one command. (Additionally, this is done via the Makefile, which allows the archive to be created locally as well as via an Action.)

For clarity, the _ext directory is renamed to _extensions, and the extensions therein are now "installed" to the Poetry environment. Additionally, the minimum Sphinx version is now to 6.2 or later, as that's what's been in use recently and Poetry allows us to be less "stuck" on what's provided by distributions.

Lastly, the Makefile is now cross platform and it will now work on Windows as long as a GNU Make (or GNU Make compatible tool) is used.

Fixes #40.

[bruxisma]

One thing to note: once merged, you will need to change the github pages "source" to Github Actions and then kick off a build manually (or change it pre-merge and then it will kick off)

[mwoehlke]

I'm not that familiar with poetry; will this interfere with using the Makefile to generate the HTML locally? (I'm wondering if it makes sense to change the Makefile and instead use make to build, so we aren't duplicating logic.)

I think sys.path.insert(0, os.path.abspath('_ext')) needs to be either changed or removed? (BTW, is it possible to keep the name as _ext? I think that helps make it clear that it is content related to "rendering" the specification, rather than to the specification itself. Note also _static. Yes, I ought to also change the Makefile to _site.)

[bruxisma]

will this interfere with using the Makefile to generate the HTML locally?

It will not, though I'd recommend using poetry via the Makefile regardless. What poetry does is provide a lockfile for well known dependencies, the ability to create a virtualenv on a per-project basis (so your global system is left alone), the ability to execute from within said virtualenv without having to run any special scripts, creating an "editable" install that can still be debugged, and making sure anyone who uses it is on the same page. (It also means in your case you're not tied to "whatever fedora provides" but rather "what version of python does fedora provide?"). In other words the documentation build becomes more reproducible and a bit more hermetic.

Also, _static et. all. gets rendered verbatim into the build directory. I'll clean this up a bit and move stuff into _extensions, but wanted to mention that if we did care about how this is all laid out we could easily organize it a bit better. 🙂

I'll change the Makefile to use _site as well.

[mwoehlke]

I'd recommend possibly putting some approvals for the actual final deployment to GitHub Pages in place

Can you clarify / expand on this?

Lastly, I took an extra step and added a spellchecker for the github actions steps.

This... runs spell check on the .rsts? Is it able to flag only newly-introduced errors? Otherwise I can't imagine, in such a technical work, this producing so many false positives as to make the signal-to-noise ratio unmanageable.

[bruxisma]

I'd recommend possibly putting some approvals for the actual final deployment to GitHub Pages in place

Can you clarify / expand on this?

Sure, the documentation is here. It means that someone can manually approve changes to deploy to github pages.

Lastly, I took an extra step and added a spellchecker for the github actions steps.

This... runs spell check on the .rsts? Is it able to flag only newly-introduced errors? Otherwise I can't imagine, in such a technical work, this producing so many false positives as to make the signal-to-noise ratio unmanageable.

I already ran it and there are no detected errors 🙂. This would prevent changes from getting added that have common typos. If a word isn't recognized it rarely results in a false positive (all the same I've had it ignore the css and svg files)

[bruxisma]

BTW, please a) rebase, b) squash, c) avoid using emoji in commit messages (at least in summaries!).

The one in the PR summary while it's still WIP is okay.

I'll fix the merge conflicts, but I'd recommend you set the following settings for the repository

This would remove the need to worry about merge commits, rebasing, squashing, etc. and would use just the PR description, and not including every small little "wip" or "fix" commit message when merging.

regarding the emoji commit messages, I use gitmoji so that at a glance someone can have a general idea of what a PR involves, regardless if they are ESL or not. In this case, the PR involves continuous integration, and thus I used the emoji for such. nlohmann of nlohmann json fame is actually the one who inspired me to do this. You can also edit the title of a PR before merging if you'd like 🙂

[mwoehlke]

I'd recommend you set the following settings for the repository

I prefer to write my own clean history rather than expecting others to clean up my mess. I also loathe projects that require merge strategies that change the history during the merge. It's akin to maintainers force-pushing to contributor's repositories, and it makes more work for the contributor to clean up afterwards.

...but if you want to make your life harder, I won't force you to squash.

As for emoji, I don't necessarily mind them on websites, but git histories are often viewed in terminals where emoji display poorly or even cause issues. And they aren't more understandable than text. For example, I'm very much not ESL and I took the emoji to mean "work in progress". It seems that isn't what you intended. For some of your other commits, I don't know that I could even begin to guess what meaning is intended.

[bruxisma]

@bretbrownjr Just got the notification that the workflow was run, but this was before I could handle the other changes and reviews that @mwoehlke had requested. I'll be tweaking this a bit this weekend. 🙂

[bruxisma]

I'd recommend you set the following settings for the repository

I prefer to write my own clean history rather than expecting others to clean up my mess. I also loathe projects that require merge strategies that change the history during the merge. It's akin to maintainers force-pushing to contributor's repositories, and it makes more work for the contributor to clean up afterwards.

...but if you want to make your life harder, I won't force you to squash.

The reason I won't is because I've had cases more often than not where I've caused regressions because of a rebase + squash and then couldn't find the intermediate changes unless I went through every. single. commit. in the pr. It's not fun, and when it comes to devops stuff where debugging is basically "do it live", life is hell for it 😮‍💨

As for emoji, I don't necessarily mind them on websites, but git histories are often viewed in terminals where emoji display poorly or even cause issues.

This has not been my experience across all 3 major operating systems, especially windows (cmd.exe doesn't support unicode properly so I don't consider it a terminal of any kind). And, even then if there are issues, all the better to use them to find the bugs lurking in someone's workflow! 😝

I kid, I kid. That said tools like gh will strip out emoji for situations like this IIRC. Granted, as I said we can change the PR title which will prevent the PR from going in with an emoji in the first place. I'm afraid to say this is a hill I'm willing to die on :v

And they aren't more understandable than text. For example, I'm very much not ESL and I took the emoji to mean "work in progress". It seems that isn't what you intended. For some of your other commits, I don't know that I could even begin to guess what meaning is intended.

Well I linked the gitmoji website in my comment so you could understand previous commits (minus the lock for lockfile 😓), and have a reference 😄

[bretbrownjr]

Yeah, apologies. I'm just poking around a bit. Didn't mean to sound the klaxons or anything.

[mwoehlke]

I'm afraid to say this is a hill I'm willing to die on :v

Likewise. In addition to technical concerns, it's an additional barrier to anyone that hasn't already learned the system, and it's hardly a wide-spread practice. "When in Rome..."

To be clear, I say this in the same respect as doing your own squashes; namely, don't expect them to end up in master.

[bruxisma]

I'm afraid to say this is a hill I'm willing to die on :v

Likewise. In addition to technical concerns, it's an additional barrier to anyone that hasn't already learned the system, and it's hardly a wide-spread practice. "When in Rome..."

"When in Rome..." should be a statement only for those that aren't trying to make any changes in a given location. After all, for hundreds of years humorism was the primary form of disease transmission, and look how that turned out in the long run 😉

Otherwise, why work on CPS when we could just use pkg-config. "When in Rome" 😝

[bruxisma]

I've edited the PR title to be in line with what you've asked for, merged in latest, and also updated the Makefile to rely on the same commands we use, while also supporting some additional features, such as auto-installing the project if no poetry based virtualenv is detected. With the exception of the publish target, the Makefile now works on Windows natively as well, and several "builtin" makefile rules and variables are disabled to help when debugging via make -p if something goes awry.

@mwoehlke I've also resolved a few conversations with my fixes, and this should be good to go.

[mwoehlke]

This isn't working in the GHA environment, either. Same error as local.

[bruxisma]

This isn't working in the GHA environment, either. Same error as local.

@mwoehlke Yep, read the error message. Windows case insensitivity strikes again! Just pushed a fix, should work now. 🙂

[bruxisma]

Looks like there's only two warnings and that affects package caching (which can be fixed later. It did build pretty fast without it), BUT the artifact upload step is working. You just need to setup the github pages environment now so the deploy step can work on merge to the default branch 🙂

[mwoehlke]

Just pushed a fix, should work now

Joy. Apparently, not only does that break the build, it does so a) without leaving any evidence that's the problem (at least via make), and b) once the error has been hit, Poetry is in a state that needs manual repair; just applying the fix doesn't trigger Poetry to sort itself out.

I get the idea of Poetry. I'm becoming less impressed with its reliability.

Also, please see other review comments.

[bruxisma]

oh that's most likely because I was doing a check against the venv on whether to run the make setup target, and that gets created regardless of whether the current package was installed or not. Still, there should be a flag of some kind to fail if the install failed for the current project. At the very least, I could have make setup execute a poetry check before it runs, as that would have caught the issue. That should also be added to the workflow. I'll add that, then respond to other comments.

[mwoehlke]

Given that the mere existence of the venv is no guarantee that it is up to date, or even valid, should all just depend on setup unconditionally? That adds ~1.5s to the build on my machine, but seems worth it for the build to not be broken. (GHA will always need that step, of course, so no difference there.)

While we're at it, maybe the actual page generation should be html, with all depending on html and html depending on setup... And a target to nuke the Poetry venv would be nice, too...

[bruxisma]

I didn't want to hoist the cost of an "always install" command on folks without someone else saying they were fine with it. If you're ok with it, then I'm ok with it. 🙂

I can add the venv obliteration as well. I'll add a purge target so that you can more or less have a clean setup for the repository, separate from clean and cache-clean.

[mwoehlke]

I didn't want to hoist the cost of an "always install" command on folks without someone else saying they were fine with it

Fair enough. However, given that the cost is ~1.5s versus a potentially broken environment, I don't mind going on record as being okay with it. :slightly_smiling_face: (Anyway, I don't expect many people to be building the HTML locally all that often, so...)

I'll add a purge target...

Thanks!

FWIW, I was thinking env-clean/venv-clean. :shrug: On its own, "purge" is fine, but w.r.t. the existing cache-clean it seems a little less consistent. OTOH, maybe you have a better idea for naming cache-clean?

[mwoehlke]

Oh, sorry, I'm seeing comments in reverse temporal order. If there's a way to only require setup if poetry check is unhappy, that's okay too if that can tell the difference between "a venv exists but is out of date or doesn't actually have stuff 'installed'" and "the venv is ready to go". Nope, poetry check is happy with a partial install. Looks like unconditionally depending on setup / poetry install is needed.

[bruxisma]

Oh, sorry, I'm seeing comments in reverse temporal order. ~If there's a way to only require setup if poetry check is unhappy, that's okay too if that can tell the difference between "a venv exists but is out of date or doesn't actually have stuff 'installed'" and "the venv is ready to go".~ Nope, poetry check is happy with a partial install. Looks like unconditionally depending on setup / poetry install is needed.

We may need to do a check + install as the full setup target, or something similar. I'll figure something out.

[mwoehlke]

We may need to do a check + install as the full setup target

Note that, as far as I can tell, poetry check reports "All set!" even if the local project isn't installed:

$ poetry install
...
[Errno 2] No such file or directory: '/home/src/matthew/work/cps/README.md'
$ poetry check
All set!

(I intentionally reintroduced the readme issue so the local project install wouldn't happen.)

That being the case, unless there's a more "complete" test, I think HTML generation should just depend on poetry install unconditionally.

[bruxisma]

Sorry for not responding sooner. Busy day :v

I've set the setup target to be a dependency of all, add a purge target that cleans the venv and the build directory, changed the target names to clean.venv and clean.cache, but kept clean for the build itself. I've also set it up so that we always check the pyproject.toml config in CI and during the build as well just in case. When you're in a broken state, there's no way for us to run install then check and have that work. But we could do check + install and that would prevent a failed install. You were in a broken state because of out of order execution.

I also added your code suggestion. I'll get to the rest of this in the AM. 🫡

[mwoehlke]

@bruxisma, unless I hear an objection Very Soon, I am going to do two things:

1. Push some minor fixes to fully resolve the issues I see. (Mostly, using make in the GHA recipe and some very minor issues in the Makefile.)
2. Replace the description in this Issue with the intended commit message, as it's still not obvious if GitHub is going to let me edit that beforehand.

For posterity, here is the original description:

This PR closes #40 and brings in a few changes to the general build workflow:

1. [`poetry`][1] is now used to setup a virtual environment separate from your global environment to allow having a uniform installation of tools.
2. `cps` now exists as a sphinx plugin + module and is "installable". This also applies for the forked/modified `autosectionlabel`. Details on how to add additional custom extensions can be found in the `pyproject.toml` file (`tool.poetry.packages` and the `tool.poetry.plugins."sphinx-builders"` sections)
3. We do NOT use the github [upload-pages-artifact][2] action, as we can package + fixup permissions in one move.
4. We do NOT use the Makefile during the CI step.
5. We cache sphinx dependencies now so when upgrading/installing things should go much faster (especially on PRs)

GitHub Pages *do not yet support* previews of PRs for the general public. However, GitHub Pages alternatives such as Cloudflare Pages *do*. This is something I'd recommend considering if having per-commit + per-branch previews is desired.

> [!TIP]
> I'd recommend *possibly* putting some approvals for the actual final deployment to GitHub Pages in place

Lastly, I took an extra step and added a spellchecker for the github actions steps. Unfortunately, they do not currently support having generalized tags for a specific version. Keeping up to date with it without automation would be difficult. For this reason, we're currently using `@master`. However, I'd recommend considering installing [renovatebot][3] to help keep our github actions workflows up to date and also *pinned* to specific git commit hashes. 

You can see how this operates in practice by looking at [my personal `workflow_call` capable workflows][4] and how they've managed to keep actions pinned and up to date for *quite some time*. The usage of renovatebot requires the installation at the organization level, and so I'm unable to actually turn it on for CPS itself 🙂

> [!NOTE]
> I understand this is a lot of suggestions + changes, and also this initial PR might shit the bed right out of the gate, but we can get that resolved ASAP.

[1]: https://python-poetry.org
[2]: https://github.com/actions/upload-pages-artifact/
[3]: https://docs.renovatebot.com/
[4]: https://github.com/bruxisma/actions/pulls?q=is%3Apr+is%3Aclosed

[bruxisma]

@bruxisma, unless I hear an objection Very Soon, I am going to do two things:

1. Push some minor fixes to fully resolve the issues I see. (Mostly, using `make` in the GHA recipe and some very minor  issues in the `Makefile`.)

2. Replace the description in this Issue with the intended commit message, as it's still not obvious if GitHub is going to let me edit that beforehand.

Alas, I had a late morning and didn't get to a PC until now. I'll get the last things fixed up in about 20-30 minutes.

[mwoehlke]

I'll get the last things fixed up in about 20-30 minutes.

Ack, you managed to post that seconds after I pushed fixes. I'm okay with the current state; is there anything else you think still needs attention?

(Besides dropping the @ from the make setup commands — I don't think we need that — the Makefile changes are hopefully straight-forward. Mostly it was rearranging things, fixing the .PHONY targets, removing the unused srcdir and adding setup.flags.)

[mwoehlke]

Also, please let me know if you think anything is missing in the description. I took what I'd written for #49, but there are additional refinements here.

[bruxisma]

I updated the description slightly to add more context and use a bit more markdown while not destroying the footnotes you'd used, but also not using the [word](link) syntax.

One last thing I think is worth doing: adding an archive or prepare-archive task so we can share steps between CI and local for testing. Thoughts? Wouldn't take more than a few minutes. tar has been available on Windows since about 2018 under system32, so it shows up on the system path. (It's also bsdtar, the same tar used by macOS)

[mwoehlke]

One last thing I think is worth doing: adding an archive or prepare-archive task so we can share steps between CI and local for testing. Thoughts?

I was actually just thinking the exact same thing. I was planning to circle back so as to not be pushing further changes to your branch, but since you had the same thought, let's do it. I vote for archive, though, and preferably put it right before publish; thanks! :slightly_smiling_face:

[bruxisma]

Will do! This way since I'll be pushing the changes, we won't have to wait for bret to approve before merge 😝

[mwoehlke]

Well, I don't have to wait on Bret, though I pinged him to see if he wants to do a review anyway. If I don't hear back, I'll wait until tomorrow to press the button.

Thanks for the cross-platform note! I changed a couple words to keep the tone more consistent, but that's indeed a notable mention I'd overlooked. Since this is intended to become the commit message verbatim, I prefer to avoid markdown that doesn't significantly benefit a reader that only sees plaintext, but I can live with that mechanism for footnotes. :slightly_smiling_face: I also re-wrapped for the same reason (i.e. because I don't trust GitHub to do it right), and I preemptively added a few words about archive generation.

[bretbrownjr]

I won't be able to review until tomorrow in the Western hemisphere, but I'm not worried about that. If you two are satisfied, I am too.

[mwoehlke]

If you two are satisfied, I am too.

Okay, thanks @bretbrownjr!

[mwoehlke]

...and I belatedly realize that $(if $(foo),$(foo),stuff) can be replaced with FOO ?= stuff, so I did that. :slightly_smiling_face:

Last call for pre-merge objections! :tada:

[bruxisma]

One last commit, since you simplified the if 😝

[mwoehlke]

Heh, I didn't even notice that one. :slightly_smiling_face: :+1:

[mwoehlke]

I'll take that as a lack of objections, then!

[mwoehlke]

Okay, for future reference, I can edit the commit message after pressing the button. (Not total wasted effort, though, since having done so preemptively, I didn't need to edit it...)

Thanks again!

[bruxisma]

welp, sad news: deploy step failed, and the error message wasn't too helpful. I assume maybe the environment or similar isn't setup? 😬

[mwoehlke]

@bruxisma, yes, I was going to ask if you have any thoughts. Default(?) workflow permissions are read-only but the option to change that is disabled (and probably not what we want, anyway). The github-pages environment exists and seems to be correctly configured. Pages source is already GHA (which a) surprised me, and b) makes me wonder how the old pages, which AFAIK are using the classic "branch" model, are still present).

I don't recall making changes during the course of this PR because it's not clear exactly what needs to change? Maybe you have some ideas?

[bruxisma]

I'll look into it shortly. I've got some fires to put out at $JOB I'm afraid :(

[mwoehlke]

@bruxisma, solved! And I think the solution may not have been previously known. (Also, that was fun; the only way to test was to push a hope-this-works commit directly to master. I was going to force-push that away real fast if it hadn't worked!)

[bruxisma]

oof. Glad it worked at least. One of the things I like about cloudflare pages, vercel, et. al. is they allow previews in case deployment to master/main fails, whereas that's not available to github pages yet and I don't know what the roadmap for it is.

[mwoehlke]

Right, though the problem being with deploy-pages, I'm not sure if that would have helped in this instance. :slightly_smiling_face: In any case, I left an annoyed comment that upload-pages-artifact really ought to fix their misleading documentation.

I'm about to push a follow-up PR to fix some minor things I noticed.

[mwoehlke]

54 if you're interested...