docs(cleanup): making small changes for cleaning up manual linting#147
docs(cleanup): making small changes for cleaning up manual linting#147benschwaller wants to merge 1 commit into
Conversation
|
@benschwaller do you have any error logs for the proxy-agent piece? I'm trying to get a better idea of what's requiring it to see if it needs to be added to the Sphinx Stack makefile template upstream. It's already in .sphinx/node_modules for me locally but isn't in the makefile anywhere so I'm not sure where/when/why it's already installed. |
|
I needed it for specifically running pa11y. When I did a fresh make pa11y in I did not have proxy-agent npm installed. If I uninstall it and wipe my .sphinx/venv now though I don't have the issue. I am trying to get a fresh multipass instance to get a better view (seems like a good usecase for workshop but I want to get you an answer sooner). |
|
I concur with Ashley's comment here. When I ran |
|
So I spun up a fresh multipass. Never came across the proxy-agent issue, so let's count that as a one off. Here's new findings though from a freshly born ubuntu 24.04 that I needed to get everything to work. If python 3.12-venv is not installed before make, you need to do a rm -rf .sphinx/venv otherwise you get make run then works, I run make pa11y and: ldd shows missing packages for puppeteer which can then be added to make pa11y happy. What would be best to change / edit this commit and then also to address the reqs I needed to follow? |
|
Testing this with workshop now too for getting familiar with that |
|
In workshop with minimal 24.04, this is all I needed to add these apt installs for puppeteer. The other issues held. sudo apt update && sudo apt install -y |
|
@AshleyCliff @NucciTheBoss just a gentle ping, how should I proceed with this commit? I am leaning towards:
Thoughts? |
|
AI summarization below of what to do now with 2.0 migration on sphinx in mind. TLDR just some names would change for 2.0 but the installs needed for puppeteer would still be present, make a reqpa11y and do things similarly to pdf. I didn't see any reason we would wait on this given the name changes will need to be done for several other bits of the makefile anyway.
pa11y-deps-force: 2.0 translation (for when you upgrade) v1.x v2.0 Reasoning per decision Why a separate -deps / -deps-force split, mirroring pdf-prep. That pattern exists in the existing Makefile specifically because the Makefile shouldn't be silently making system-wide changes. The check target emits a helpful message and exits 1; the force target is opt-in via sudo. Pa11y needs the same courtesy. CI images bake the deps in at build time; dev machines run sudo make pa11y-deps-force once; day-to-day make pa11y is silent. Why prereq on pa11y-install, not pa11y. pa11y-install is also a public target someone could run on its own (e.g. to populate the node_modules cache). Putting the check there means any code path that actually launches pa11y (which depends on pa11y-install for the binary) gets the system check. pa11y itself stays untouched. This matches the pdf: pdf-prep indirection. Why no apt-get upgrade -y in pa11y-deps-force. The existing pdf-prep-force has it, but that's actually a footgun: on a CI runner it can pull in unrelated security updates mid-build and obscure failures, and on a dev machine it's destructive in a way the user probably didn't consent to. I left pdf-prep-force alone (don't expand scope of this change), but the new target should be the cleaner pattern. If you want, drop apt-get upgrade -y from pdf-prep-force too in a follow-up. Why the trailing \ on the install line. Matches the existing pdf-prep-force cosmetic. It's a no-op continuation to a blank line and doesn't break anything. Keeping it means a future diff against sphinx-stack stays clean. Why 36 packages and not the 10 I had before. The full list is the official Puppeteer troubleshooting list (https://pptr.dev/troubleshooting) for the Chromium binary that pa11y 9's bundled Puppeteer 24 actually downloads. I had a partial and partially-wrong list in my earlier message; this one is verbatim from the upstream source, which is what you want as a source of truth. Why npm is NOT in REQPA11YPACKS. The Puppeteer list is the system-deps floor for the Chromium binary. npm is a project-wide prereq (you need it for make install in many JS projects), and some users install it via nvm or NodeSource instead of apt. Mixing it in would be overreach. If your environment gets pa11y working without apt install npm, leave it out. If your docs already say sudo apt install npm as a prereq, keep that documentation as-is. Why the variable name is REQPA11YPACKS and not DOCS_PA11YPACKAGES. Matches your existing REQPDFPACKS (v1.x style). When you upgrade to 2.0, you'd rename it then. |
Pre-submission checklist
Summary of Changes
//: Added LTS to a version number in performance.md to satisfy vale.
//: Added proxy-agent to pa11y install for correct functionality.
//: Added node_modules to excludes for passing pa11y.
Related Issues, PRs, and Discussions