| Message ID | 20241031-fix-cve-add-role-warning-v1-1-68e44c7e8961@baylibre.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | conf.py: silence cve role already registered | expand |
Hi Julien, On Thu Oct 31, 2024 at 5:52 PM CET, Julien Stephan wrote: > Commit bd9add8a0 - conf.py: add macro for Mitre CVE links > added a new macro for mitre cve links called "cve_mitre" which conflicts > with the already existing "cve" role. > > WARNING: role 'cve' is already registered, it will be overridden [app.add_role] > > The warning is turned into an error making "make html" fail, but > documentation is still successfully built. > > Since documentation is still correctly built, just suppress the warning. > > Signed-off-by: Julien Stephan <jstephan@baylibre.com> > --- > documentation/conf.py | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/documentation/conf.py b/documentation/conf.py > index 35c5c14535ba67aae3554819b55f0fb52a9d0027..aaddf2c789196fc26a734f06dbe005f147b9bbd3 100644 > --- a/documentation/conf.py > +++ b/documentation/conf.py > @@ -118,6 +118,9 @@ intersphinx_mapping = { > # Suppress "WARNING: unknown mimetype for ..." > suppress_warnings = ['epub.unknown_project_files'] > > +# Suppress "WARNING: role 'cve' is already registered, it will be overridden [app.add_role]" > +suppress_warnings.append('app.add_role') > + > # -- Options for HTML output ------------------------------------------------- > > # The theme to use for HTML and HTML Help pages. See the documentation for > > --- > base-commit: 9563855ccd92e21fb6f8320c96a3a83e115c947e > change-id: 20241031-fix-cve-add-role-warning-1147c223787a > > Best regards, Thanks, we're also updating Sphinx at the moment, so this comes at the right time! I do get the warning/error on 8.1.3, and this fixes the error and make the links point to NIST, as expected. Also verified and it is backward compatible with previous versions of Sphinx. Reviewed-by: Antonin Godard <antonin.godard@bootlin.com> Tested-by: Antonin Godard <antonin.godard@bootlin.com> Cheers, Antonin
Hi Julien, On 10/31/24 5:52 PM, Julien Stephan via lists.yoctoproject.org wrote: > Commit bd9add8a0 - conf.py: add macro for Mitre CVE links > added a new macro for mitre cve links called "cve_mitre" which conflicts > with the already existing "cve" role. > > WARNING: role 'cve' is already registered, it will be overridden [app.add_role] > > The warning is turned into an error making "make html" fail, but > documentation is still successfully built. > > Since documentation is still correctly built, just suppress the warning. > I don't understand the issue and why Sphinx complains about it. I'm also very wary about just silencing a warning instead of tackling it. I don't understand why this triggers it, extlinks contains an alias with a base URL and a caption. The only overlap we have is the CVE-%s caption. But why would that be an issue, it's just text! Also not sure what Antonin meant with "make the links point to NIST, as expected.". Which ones? What's the actual issue here we're trying to fix? Cheers, Quentin
Hi Quentin, On Mon Nov 4, 2024 at 11:33 AM CET, Quentin Schulz via lists.yoctoproject.org wrote: > Hi Julien, > > On 10/31/24 5:52 PM, Julien Stephan via lists.yoctoproject.org wrote: >> Commit bd9add8a0 - conf.py: add macro for Mitre CVE links >> added a new macro for mitre cve links called "cve_mitre" which conflicts >> with the already existing "cve" role. >> >> WARNING: role 'cve' is already registered, it will be overridden [app.add_role] >> >> The warning is turned into an error making "make html" fail, but >> documentation is still successfully built. >> >> Since documentation is still correctly built, just suppress the warning. >> > > I don't understand the issue and why Sphinx complains about it. > > I'm also very wary about just silencing a warning instead of tackling it. The issue is that newer Sphinx versions are already defining a :cve: role that points to cve.org, instead of the role we defined in conf.py that points to nvd.nist.gov. So we have different solutions: - either override the role to make it point to nvd.nist.gov, which is a bit of a hack but works: from sphinx.roles import CVE CVE._BASE_URL ='https://nvd.nist.gov/vuln/detail/CVE-' Also that may not be backwards compatible. Context: we already noticed last week that old releases don't build with newer versions of Sphinx, we were thus planning on keeping an old Sphinx version for building old doc versions. - just point to cve.org instead, I'm not sure how important pointing to the nist database is. May pose a problem with older sphinx versions, though. - remove the warning as Julien did. Has the advantage of being backward compatible, although not nice to suppress a warning of course. > I don't understand why this triggers it, extlinks contains an alias with > a base URL and a caption. The only overlap we have is the CVE-%s > caption. But why would that be an issue, it's just text! I guess extlinks doesn't really act as an "override existing roles" behavior but an "append to existing roles and warn if a role already exists". > Also not sure what Antonin meant with "make the > links point to NIST, as expected.". Which ones? What's the actual issue > here we're trying to fix? > > Cheers, > Quentin Cheers, Antonin
Hi Antonin, On 11/4/24 12:11 PM, Antonin Godard wrote: > Hi Quentin, > > On Mon Nov 4, 2024 at 11:33 AM CET, Quentin Schulz via lists.yoctoproject.org wrote: >> Hi Julien, >> >> On 10/31/24 5:52 PM, Julien Stephan via lists.yoctoproject.org wrote: >>> Commit bd9add8a0 - conf.py: add macro for Mitre CVE links >>> added a new macro for mitre cve links called "cve_mitre" which conflicts >>> with the already existing "cve" role. >>> >>> WARNING: role 'cve' is already registered, it will be overridden [app.add_role] >>> >>> The warning is turned into an error making "make html" fail, but >>> documentation is still successfully built. >>> >>> Since documentation is still correctly built, just suppress the warning. >>> >> >> I don't understand the issue and why Sphinx complains about it. >> >> I'm also very wary about just silencing a warning instead of tackling it. > > The issue is that newer Sphinx versions are already defining a :cve: role that > points to cve.org, instead of the role we defined in conf.py that points to > nvd.nist.gov. So we have different solutions: Ah, there it is. Thanks for the explanation, this should at the very least be added to the commit log :) > > - either override the role to make it point to nvd.nist.gov, which is a bit of a > hack but works: > > from sphinx.roles import CVE > CVE._BASE_URL ='https://nvd.nist.gov/vuln/detail/CVE-' > > Also that may not be backwards compatible. Context: we already noticed last > week that old releases don't build with newer versions of Sphinx, we were thus > planning on keeping an old Sphinx version for building old doc versions. > > - just point to cve.org instead, I'm not sure how important pointing to the nist > database is. May pose a problem with older sphinx versions, though. > > - remove the warning as Julien did. Has the advantage of being backward > compatible, although not nice to suppress a warning of course. > - rename :cve: to :cve_nist: Easy to backport, easy to replace with sed (s/:cve:/:cve_nist:/) no need to get rid of warnings or implement non-portable hacks. I'm biased, I vote for the fourth option :) Cheers, Quentin
Hi Quentin, On Mon Nov 4, 2024 at 1:21 PM CET, Quentin Schulz wrote: [...] > > - rename :cve: to :cve_nist: > > Easy to backport, easy to replace with sed (s/:cve:/:cve_nist:/) no need > to get rid of warnings or implement non-portable hacks. > > I'm biased, I vote for the fourth option :) Actually, sound like a reasonable approach to me too! :) I'm working on updating Sphinx atm, so a patch should come sooner or later. > Cheers, > Quentin Cheers, Antonin
diff --git a/documentation/conf.py b/documentation/conf.py index 35c5c14535ba67aae3554819b55f0fb52a9d0027..aaddf2c789196fc26a734f06dbe005f147b9bbd3 100644 --- a/documentation/conf.py +++ b/documentation/conf.py @@ -118,6 +118,9 @@ intersphinx_mapping = { # Suppress "WARNING: unknown mimetype for ..." suppress_warnings = ['epub.unknown_project_files'] +# Suppress "WARNING: role 'cve' is already registered, it will be overridden [app.add_role]" +suppress_warnings.append('app.add_role') + # -- Options for HTML output ------------------------------------------------- # The theme to use for HTML and HTML Help pages. See the documentation for
Commit bd9add8a0 - conf.py: add macro for Mitre CVE links added a new macro for mitre cve links called "cve_mitre" which conflicts with the already existing "cve" role. WARNING: role 'cve' is already registered, it will be overridden [app.add_role] The warning is turned into an error making "make html" fail, but documentation is still successfully built. Since documentation is still correctly built, just suppress the warning. Signed-off-by: Julien Stephan <jstephan@baylibre.com> --- documentation/conf.py | 3 +++ 1 file changed, 3 insertions(+) --- base-commit: 9563855ccd92e21fb6f8320c96a3a83e115c947e change-id: 20241031-fix-cve-add-role-warning-1147c223787a Best regards,