Re: [gentoo-dev] RFC: intel-sdp-r1.eclass

[Michał Górny]

On Mon, 15 Feb 2016 14:37:41 +0100
"Justin Lecher (jlec)" <jlec@g.o> wrote:

> On 15/02/16 13:59, Michał Górny wrote:
> > On Mon, 15 Feb 2016 09:16:53 +0100
> > "Justin Lecher (jlec)" <jlec@g.o> wrote:
> >
> >> # @ECLASS-VARIABLE: INTEL_SUBDIR
> >> # @DEFAULT_UNSET
> >> # @DESCRIPTION:
> >> # The package sub-directory where it will end-up in /opt/intel
> >> # To find out its value, you have to do a raw install from the Intel tar ball
> >
> > To be honest, I find this kinda terrible. There's a huge block of docs
> > which makes me feel small and confused. Maybe it'd useful to give some
> > semi-complete example on top (in global doc)?
>
> That makes definitely make sense. We will add one.
>
> Although nobody other then the maintainer of this eclass will ever use it.

Remember that maintainers can change. It's better to have good then
have new maintainers figure out all stuff over again.

> >> # e.g. CLI_install/rpm/intel-vtune-amplifier-xe-cli
> >> : ${INTEL_BIN_RPMS:=()}
> >
> > $ : ${foo:=()}
> > $ declare -p foo
> > declare -- foo="()"
> >
> > In other words, it doesn't work the way you expect it to.
>
> I already wondered about this. Is there any way to force a variable to
> be an array in bash? Or define it as an empty array?

Look at e.g. python-utils-r1.

To check for array:

  if [[ $(declare -p foo) != "declare -a"* ]]; then
    ...
  fi

To default to empty, simple (yet a bit imperfect) way:

  [[ ${foo[@]} }] || foo=()

> >> # @ECLASS-VARIABLE: INTEL_SINGLE_ARCH
> >> # @DESCRIPTION:
> >> # Unset, if only the multilib package will be provided by intel
> >> : ${INTEL_SINGLE_ARCH:=true}
> >
> > This is really weird. It sounds like I'm supposed to do:
> >
> >   inherit intel-sdp-r1
> >   unset INTEL_SINGLE_ARCH
> >
> > I suggest you used positive logic instead.
>
> The wording is wrong. Setting it to anything but "true" like
> "INTEL_SINGLE_ARCH=false" works. We will fix the wording.

I still think positive logic is better. That is, a variable which
defaults to, say, unset, and changes behavior if it becomes set to
non-empty value.

> >> _isdp_big-warning() {
> >> 	debug-print-function ${FUNCNAME} "${@}"
> >>
> >> 	case ${1} in
> >> 		pre-check )
> >> 			echo ""
> >
> > Don't mix echo with ewarn.
>
> Why?

Because they won't go through the same output channels.

> >> 				comp_full="${ED}/${INTEL_SDP_DIR}/linux/bin/${arch}/${comp}"
> >
> > Double slash imminent (ED has one).
>
> Always? Per definition?

Yes, sadly. i wanted to change this but it's unlikely to go since it
makes EAPI migration hard. If you really want to cover both cases, you
can always do ${foo%/}/bar.

> >> # @CODE
> >
> > Err, this is not code, you know.
>
> This is needed for nice formatting. Otherwise there is no line break

Add an empty line between the two. That should do it correctly, without
code blocks in devmanual.

> >> 			#maybe use nullglob or [[ $(echo ${dir/*lic) != "${dir}/*lic" ]]
> >> 			[[ $( ls "${dir}"/*lic 2>/dev/null ) ]]; ret=$?
> >
> > Maybe you should use something sane indeed.

Maybe file_exists from eutils could help here btw.

> > Wouldn't you be able to collapse that into one loop?
>
> no, because the first has ${INTEL_X86}.rpm as suffeix and the later has
> ${INTEL_X86}.rpm.

Errrrr... am I reading wrong, or did you just type the same thing twice?

> >> 			einfo "Unpacking ${rb}"
> >> 			rpm2tar -O ${r} | tar xvf - | sed -e \
> >> 				"s:^\.:${EROOT#/}:g" > /dev/null; assert "unpacking ${r} failed"
> >
> > What's the deal with this sed?
>
> Good question, but it was there since always and probably the original
> author had good reasons for it. We will look into it and comment the code.

Didn't it change in the past? As I see it, tar here outputs file names,
sed edits them and then you discard them to /dev/null. In this case,
sed doesn't return any specific exit code so it seems to be a complete
no-op.

Maybe originally it verbosely output the extracted files.

> >> EXPORT_FUNCTIONS pkg_setup src_unpack src_install pkg_postinst pkg_postrm pkg_pretend
> >
> > We usually do this on top, and it's better to do it outside guards so
> > that order from inherit is always respected.
>
> we will move it up. I don't get your second comment. Do you mean the
> case someone does
>
> inherit intel-sdp-r1 some-other-eclass intel-sdp
>
> ?

Rather something unlikely like:

  inherit foo bar intel-sdp-r1

where foo inherits intel-sdp-r1, and therefore the exports occur before
bar, and bar takes over even though intel-sdp-r1 is explicitly
listed last.

-- 
Best regards,
Michał Górny
<http://dev.gentoo.org/~mgorny/>