Re: [gentoo-dev] [PATCH] rebar.eclass: Build Erlang/OTP projects using dev-util/rebar

[Amadeusz Żołnowski]

Michał Górny <mgorny@g.o> writes:
>> +export ERL_LIBS="${EPREFIX}$(get_erl_libs)"
>
> I think calling get_libdir in global scope is forbidden. You should
> really export this somewhere in phase function.

Fixed.


>> +# @FUNCTION: _find_dep_version
>
> Namespace it, please. Just in case.

Fixed.

>> +_find_dep_version() {
>> [...]
>> +	pushd "${EPREFIX}$(get_erl_libs)" >/dev/null
>
> || die
>
> [...]
>
>> +	popd >/dev/null
>
> || die

Fixed.


>> +eawk() {
>> +	local f="$1"; shift
>> +	local tmpf="$(emktemp)"
>
> Missing eutils inherit for EAPI 6.

Fixed.

>> +# @FUNCTION: erebar
>> +# @USAGE: <target>
>> +# @DESCRIPTION:
>> +# Run rebar with verbose flag. Die on failure.
>> +erebar() {
>> +	debug-print-function ${FUNCNAME} "${@}"
>> +
>> +	rebar -v skip_deps=true "$1" || die "rebar $1 failed"
>> +}
>
> Any reason it doesn't pass all the parameters? This inconsistency with
> emake etc. could be mildly confusing, esp. that you don't check for
> wrong argc.

Fixed.


>> +# @FUNCTION: rebar_fix_include_path
>> +# @USAGE: <project_name> [<rebar_config>]
>> +# @DESCRIPTION:
>> +# Fix path in rebar.config to 'include' directory of dependant project/package,
>> +# so it points to installation in system Erlang lib rather than relative 'deps'
>> +# directory.
>> +#
>> +# <rebar_config> is optional. Default is 'rebar.config'.
>
> Is it likely that you would be passing different values to it? Maybe it
> would be reasonable to make this an eclass variable.

Unlikely. But I'd better just remove these parameters rather than
defining as eclass variable.


>> +	eawk "${rebar_config}" \
>> +		-v erl_libs="${erl_libs}" -v pn="${pn}" -v pv="${pv}" \
>> +		'/^{[[:space:]]*erl_opts[[:space:]]*,/, /}[[:space:]]*\.$/ {
>> +	pattern = "\"(./)?deps/" pn "/include\"";
>> +	if (match($0, "{i,[[:space:]]*" pattern "[[:space:]]*}")) {
>> +		sub(pattern, "\"" erl_libs "/" pn "-" pv "/include\"");
>> +	}
>> +	print $0;
>> +	next;
>> +}
>> +1
>> +' || die "failed to fix include paths in ${rebar_config}"
>
> I suggest you indent this a bit more since it feels like you start at
> two tabs and finish at zero.

How? Add space between "'" and "||" like follows?

+	eawk "${rebar_config}" \
...
+	print $0;
+	next;
+}
+1
+'          || die "failed to fix include paths in ${rebar_config}"

It looks a bit weird as well...


>> +# @FUNCTION: rebar_src_prepare
>> +# @DESCRIPTION:
>> +# Prevent rebar from fetching in compiling dependencies. Set version in project
>> +# description file if it's not set.
>> +#
>> +# Existence of rebar.config is optional, but file description file must exist
>> +# at 'src/${PN}.app.src'.
>
> Wouldn't it be reasonable to make this configurable? Of course, it
> might be better to leave it for a possible future extension when
> it becomes necessary.

Which part you mean? 'src/${PN}.app.src'? Fixed: REBAR_APP_SRC eclass var.

>> +rebar_src_prepare() {
>> +	debug-print-function ${FUNCNAME} "${@}"
>> +
>> +	rebar_set_vsn
>> +	[[ -f rebar.config ]] && rebar_remove_deps
>> +}
>
> You're missing obligatory default call for EAPI 6. You should really
> test stuff before submitting it.

Shame I have forgot to test it, sorry. I have completely forgotten about
EAPI 6. Fixed - I have made it working with EAPI 6 and dropped EAPI 5.


>> +# @FUNCTION: rebar_src_install
>> +# @DESCRIPTION:
>> +# Install BEAM files, include headers, executables and native libraries.
>> +# Install standard docs like README or defined in DOCS variable. Optionally
>
> Optionally what? It looks like an unfinished sentence.

Nothing. (-: Some leftover.


>> +rebar_src_install() {
>> +	debug-print-function ${FUNCNAME} "${@}"
>> +
>> +	local bin
>> +	local dest="$(get_erl_libs)/${P}"
>> +
>> +	insinto "${dest}"
>> +	doins -r ebin
>> +	[[ -d include ]] && doins -r include
>> +	[[ -d bin ]] && for bin in bin/*; do dobin "$bin"; done
>
> Please don't do inlines like this.

Is there a particular problem with this?

>> +	[[ -d priv ]] && cp -pR priv "${ED}${dest}/"
>
> This is about preserving executable bits, correct?

Yes.

Thanks for review.

-- 
Amadeusz Żołnowski