search

simonsobs / sodetlib

Tools for performing core instrument testing, quality control, and analysis tasks.
BSD 2-Clause "Simplified" License
5 stars 0 forks source link

Update docs requirements #408

Closed BrianJKoopman closed 7 months ago

BrianJKoopman commented 8 months ago

Prompted by Dependabot emailing me this advisory: https://github.com/simonsobs/sodetlib/security/dependabot/1

It wasn't clear to me why Jinja2 was pinned. I think we should update it, but please comment if this presents problems @jlashner.

This also follows the update that @msilvafe did the other day on socs in https://github.com/simonsobs/socs/pull/618.

msilvafe commented 8 months ago

Prompted by Dependabot emailing me this advisory: Jinja vulnerable to HTML attribute injection when passing user input as keys to xmlattr filter

It wasn't clear to me why Jinja2 was pinned. I think we should update it, but please comment if this presents problems @jlashner.

This also follows the update that @msilvafe did the other day on socs in simonsobs/socs#618.

@mhasself expressed some issue with these updates as the newest sphinx versions are only 18 months old. I think he's worried it hasn't been tested enough and we may be introducing some bugs in that version. Maybe we can pin to the 2nd-most recent versions?

jlashner commented 8 months ago

I can't remember why I had to pin the Jinja version.... so I'm fine removing the pin and seeing if it still causes issues.

BrianJKoopman commented 7 months ago

@mhasself expressed some issue with these updates as the newest sphinx versions are only 18 months old. I think he's worried it hasn't been tested enough and we may be introducing some bugs in that version. Maybe we can pin to the 2nd-most recent versions?

Understood, how about the solution in https://github.com/simonsobs/sodetlib/pull/408/commits/466cbbee2729e9c49f5d9c129c28d909791b43fc? Pinning to a version at the previously pinned level or higher?

A side-by-side comparison comparison of two builds looked OK to me, if anything with rendering improvements on the newer theme version.

I should also point out that the automatic deployment to simons1 broke with the MFA ssh requirements ~6 months ago, so the linked version in the README is out of date.

mhasself commented 7 months ago

he's worried it hasn't been tested enough and we may be introducing some bugs in that version

No, I just don't like forcing people to bump dependencies needlessly. Sometimes it necessary, and that's fine. But sometimes people encounter a minor error and solve it by just pushing forward all the requirements, and that breaks people's installations (including on high inertia shared systems such as site computing, simons1, NERSC, della...). As a maintainer of some shared installations, I am going to resist dependency bumps somewhat.

Having looked into it a bit more ... seems that sphinx is pure python, and should be easy to upgrade on all our supported Python 3.x versions. So if it's fixing some problem, go for it.

BrianJKoopman commented 7 months ago

If you are able to build docs locally I am fine with merging this.

I reverted unpinning the theme and argparse plugin, and can build locally, but the workflow fails to build. If this is all irrelevant due to needing to move to readthedocs anyway, let me know how you want to proceed.

I'm sympathetic to the shared systems point, but do the docs requirements get installed on those systems?

BrianJKoopman commented 7 months ago

Since it is fixing a build problem with the current workflow I'm going to keep it in. Will squash and merge now. I'll leave the readthedocs setup to you @jlashner.