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

[Justin Lecher (jlec)]

On 15/02/16 15:35, Michał Górny wrote:
> 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=()

And what about the default assignment for the man page?

>
>>>> # @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.

we will look into that.

>
>>>> _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.

That will introduce an empty line between the two points.

>
>>>> 			#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?

right, it should be ${INTEL_X86}.rpm vs noarch.rpm

>
>>>> 			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.
>