Michał Górny <[email protected]> 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
signature.asc
Description: PGP signature
