-
Notifications
You must be signed in to change notification settings - Fork 9
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
Implement automatic HTML generation #41
Conversation
This PR closes cps-org#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
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) |
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 I think |
It will not, though I'd recommend using Also, I'll change the |
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.
Great!
I asked for some dev docs on spellcheck dictionary maintenance, but it also makes sense to just make an issue to create some dev docs and circle back on that. Just from this PR, we could use breadcrumbs to:
- sphinx
- poetry
- however spellchecking works
Can you clarify / expand on this?
This... runs spell check on the |
🔥 Obliterate `sys.path.insert` change
Sure, the documentation is here. It means that someone can manually approve changes to deploy to github pages.
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) |
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.
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 🙂 |
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. |
@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. 🙂 |
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 😮💨
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
Well I linked the gitmoji website in my comment so you could understand previous commits (minus the lock for lockfile 😓), and have a reference 😄 |
Yeah, apologies. I'm just poking around a bit. Didn't mean to sound the klaxons or anything. |
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. |
"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" 😝 |
✏️ Change branch name from `main` to `master` per feedback
…n of the publish target)
I've edited the PR title to be in line with what you've asked for, merged in latest, and also updated the @mwoehlke I've also resolved a few conversations with my fixes, and this should be good to go. |
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 still not working locally:
Running Sphinx v6.2.1
Extension error:
Could not import extension cps (exception: No module named 'cps')
make: *** [Makefile:37: all] Error 2
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. |
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 |
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. |
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 One last thing I think is worth doing: adding an |
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 |
Will do! This way since I'll be pushing the changes, we won't have to wait for bret to approve before merge 😝 |
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. 🙂 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. |
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. |
Okay, thanks @bretbrownjr! |
Co-authored-by: Matthew Woehlke <[email protected]>
...and I belatedly realize that Last call for pre-merge objections! 🎉 |
One last commit, since you simplified the |
Heh, I didn't even notice that one. 🙂 👍 |
I'll take that as a lack of objections, then! |
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! |
welp, sad news: deploy step failed, and the error message wasn't too helpful. I assume maybe the environment or similar isn't setup? 😬 |
@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 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? |
I'll look into it shortly. I've got some fires to put out at |
@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!) |
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. |
Right, though the problem being with deploy-pages, I'm not sure if that would have helped in this instance. 🙂 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. |
#54 if you're interested... |
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 Poetry1 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 actionbecause 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 theextensions 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.
Footnotes
https://python-poetry.org/ ↩
https://github.com/actions/upload-pages-artifact/ ↩