Re: [gentoo-portage-dev] [PATCH] Generate soname dependency metadata (282639)

[Zac Medico]

On 01/29/2015 09:00 PM, Michał Górny wrote:
> Dnia 2015-01-26, o godz. 19:16:28
> Zac Medico <zmedico@g.o> napisał(a):
>> +from ...util.elf.constants import (
>
> Please do not use relative imports. Almost all code is using absolute
> imports so if we're going to change that, we should get a proper
> discussion first.

Well, is anyone else here opposed to using relative imports? They are
supported since python 2.5, and I don't see anything wrong with them.

>> +	needed_file.close()
>
> Looks like you really want 'with ... as needed_file:' here :).

That's an instance of atomic_ofstream, and it's designed to leave the
original file in place if the close call isn't reached. I did this on
purpose, so that the existing NEEDED.ELF.2 file is left intact if the
code is interrupted for some reason (such as with ^C). I think that's
better than leaving a partial/corrupt NEEDED.ELF.2 file there if it's
interrupted. Plus, adding "with" block makes it harder to see what's
changed in the patch, because I'll have to indent a bunch of existing code.

>> +	def __str__(self):
>> +		"""
>> +		Format this entry for writing to a NEEDED.ELF.2 file.
>> +		"""
>> +		return (
>> +			self.arch + ";" +
>> +			self.filename + ";" +
>> +			self.soname + ";" +
>> +			":".join(self.runpaths) + ";" +
>> +			",".join(self.needed) +
>> +			(";" + self.multilib_category if self.multilib_category
>> +			is not None else "") +
>> +			"\n"
>
> How about using ';'.join? Would be definitely clearer in the intention.

Like this?

";".join(
	self.arch,
	self.filename,
	self.soname,
	":".join(self.runpaths),
	",".join(self.needed),
	(self.multilib_category if self.multilib_category
	is not None else "")
) + "\n"

That would be fine with me.

>> diff --git a/pym/portage/util/elf/constants.py b/pym/portage/util/elf/constants.py
>> new file mode 100644
>> index 0000000..3857b71
>> --- /dev/null
>> +++ b/pym/portage/util/elf/constants.py
>> @@ -0,0 +1,36 @@
>> +# Copyright 2015 Gentoo Foundation
>> +# Distributed under the terms of the GNU General Public License v2
>
> I think you could mention here where those constants come from so that
> others can easily find them and update for new arches. Then it would be
> probably good to note what else needs to be updated :).

Honestly, I don't know where they originally come from, but we can just
say that they come from elfutils as suggested by Anthony.

>> +def decode_uint32_le(data):
>> +	"""
>> +	Decode an unsigned 32-bit integer with little-endian encoding.
>> +
>> +	@param data: string of bytes of length 4
>> +	@type data: bytes
>> +	@rtype: int
>> +	@return: unsigned integer value of the decoded data
>> +	"""
>> +	return (
>> +		ord(data[0:1]) +
>> +		(ord(data[1:2]) << 8) +
>> +		(ord(data[2:3]) << 16) +
>> +		(ord(data[3:4]) << 24)
>> +	)
>
> How about using the struct module instead of reinventing the wheel?

Well the versions that I wrote seem a lot more readable to me than they
would be if written using things like struct.unpack_from("<L", data)[0].
Either way is fine with me, though. Google seems to show some issues
with struct import failing for broken python installations, but that
should not be an issue in practice.
-- 
Thanks,
Zac