On Wed, 2019-11-20 at 10:23 -0800, Patrick McLean wrote: > Hi Michał, > > Thanks for doing this work, it's always better to have standardized > ways of doing things. > > On Wed, 20 Nov 2019 15:21:55 +0100 > Michał Górny <[email protected]> wrote: > > <snip> > > > +distutils_enable_sphinx() { > > + debug-print-function ${FUNCNAME} "${@}" > > + [[ ${#} -ge 1 ]] || die "${FUNCNAME} takes at least one arg: <subdir>" > > + > > + _DISTUTILS_SPHINX_SUBDIR=${1} > > + shift > > + _DISTUTILS_SPHINX_PLUGINS=( "${@}" ) > > + > > + local deps autodoc=1 d > > + for d; do > > + if [[ ${d} == --no-autodoc ]]; then > > + autodoc= > > + else > > + deps+=" > > + ${d}[\${PYTHON_USEDEP}]" > > + fi > > + done > > + > > + if [[ ! ${autodoc} && -n ${deps} ]]; then > > + die "${FUNCNAME}: do not pass --no-autodoc if external plugins > > are used" > > + fi > > + if [[ ${autodoc} ]]; then > > + deps="$(python_gen_any_dep " > > + dev-python/sphinx[\${PYTHON_USEDEP}] > > + ${deps}")" > > + > > + python_check_deps() { > > + use doc || return 0 > > + local p > > + for p in "${_DISTUTILS_SPHINX_PLUGINS[@]}"; do > > + has_version "${p}[${PYTHON_USEDEP}]" || return 1 > > + done > > + } > > I think it would be better to put this code in sphinx_check_deps > (or some such) and define a python_check_deps that just calls > sphinx_check_deps. That would allow ebuilds that need to do more than > the sphinx stuff to not have to reimplement this.
They would have to reimplement stuff anyway since it needs to be exactly
paired with python_gen_any_dep.
>
> > + else
> > + deps="dev-python/sphinx"
> > + fi
> > +
> > + python_compile_all() {
> > + use doc || return
> > +
> > + cd "${_DISTUTILS_SPHINX_SUBDIR}" || die
> > + [[ -f conf.py ]] ||
> > + die "conf.py not found, distutils_enable_sphinx call
> > wrong"
> > +
> > + if [[ ${_DISTUTILS_SPHINX_PLUGINS[0]} == --no-autodoc ]]; then
> > + if grep -q 'sphinx\.ext\.autodoc' "conf.py"; then
>
> Since this is just searching for a fixed string maybe
> "grep -F -q 'sphinx.ext.autodoc'" is a bit nicer.
Hmm, probably yes, indeed.
>
> > + die "distutils_enable_sphinx: --no-autodoc
> > passed but sphinx.ext.autodoc found in conf.py"
> > + fi
> > + else
> > + if ! grep -q 'sphinx\.ext\.autodoc' "conf.py"; then
> > + die "distutils_enable_sphinx:
> > sphinx.ext.autodoc not found in conf.py, pass --no-autodoc"
> > + fi
> > + fi
> > +
> > + # disable intersphinx (internet use)
> > + sed -e 's:^intersphinx_mapping:disabled_&:' -i conf.py || die
> > + # not all packages include the Makefile in pypi tarball
> > + sphinx-build -b html -d _build/doctrees . _build/html || die
> > +
> > + HTML_DOCS+=( "${_DISTUTILS_SPHINX_SUBDIR}/_build/html/." )
> > + }
>
> Same as above, I think it would be better to define this as
> sphinx_compile, and define a python_compile_all that just calls it.
> That way if an ebuild defines it's own python_compile_all it can call
> sphinx_compile to get this functionality.
I suppose it makes sense here.
>
> > +
> > + IUSE+=" doc"
> > + if [[ ${EAPI} == [56] ]]; then
> > + DEPEND+=" doc? ( ${deps} )"
> > + else
> > + BDEPEND+=" doc? ( ${deps} )"
> > + fi
> > +
> > + # we need to ensure successful return in case we're called last,
> > + # otherwise Portage may wrongly assume sourcing failed
> > + return 0
> > +}
> > +
> > # @FUNCTION: distutils_enable_tests
> > # @USAGE: <test-runner>
> > # @DESCRIPTION:
>
>
--
Best regards,
Michał Górny
signature.asc
Description: This is a digitally signed message part
