Re: [gentoo-dev] [PATCH 1/4] distutils-r1.eclass: Add distutils_enable_sphinx helper function

[Michał Górny]

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 <mgorny@g.o> 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