On Mon, Mar 26, 2012 at 11:39 PM, Gregory M. Turner <gmt@malth.us> wrote:
> ----- Original Message -----
>> On Mon, Mar 26, 2012 at 4:12 PM, Gregory M. Turner <gmt@malth.us>
>> wrote:
>> > https://github.com/gmt/gmt-cygwin-overlay/blob/master/sys-apps/portage/files/portage-2.2.01.20271-cygdll_protect.patch
>
>> Consistency in the style would be nice.
>>
>> For instance in cygdll-update:
>>
>> # Here you have spaces after the $( and before the )
>> extra_protection="$( portageq cygdll_protect_list ${EROOT} )"
>>
>> # Later on you don't put spaces...
>> eval $(portageq envvar -v CYGDLL_PROTECT PORTAGE_HOSTNAME
>> + * * PORTAGE_CONFIGROOT PORTAGE_INST_GID PORTAGE_INST_UID
>> + * * PORTAGE_TMPDIR EROOT USERLAND)
>
> yes, good point, (the latter is cut-and-paste from etc-update, the former my own; I do that habitually as a way to avoid learning how tokenization works in bash
>
>> Assuming $PORTAGE_BASH is always bash, you should use [[ consistently
>> in your bash.
>
> vs. "["? *That sounds like cut-paste from etc-update again -- it seems to have been coded with sh support in mind. *Don't think that matters for cygwin (in fact it'd better not, the way I coded it), so, well spotted, I'll fix it.
>
>> IMHO it is a common mistake to write your own argument processing, is
>> there a reason getops is not appropriate (portability?)
>
> Got me there -- I'm a getopts ignoramus.
>
>> Onto the python:
>>
>> if os.environ.__contains__("PORTAGE_PYTHONPATH"):
>>
>> Any reason why
>> if "PORTAGE_PYTHONPATH" not in os.environ:
>> was not used?
>
> indeed, couldn't I just use 'if not os.environ["PORTAGE_PYTONPATH"]:' (if the python executable were "False" or "0" would something stupid happen?)? *Also cut-paste from portage, but I did notice it, and had the exact same question. *After my initial, unprintable reaction
If PORTAGE_PYTHONPATH is not in os.environ then it will raise a
KeyError, that is why we are doing a contains to begin with.
>
> Maybe the answer is buried somewhere in the docs ... afaik in 2.7, "x in foo" <=> "foo.__contains__(x)" (?). *Guess I'd need to double check 2.7 and 3.2 docs as well to be sure.
I'm not saying they are not equal, I'm saying foo.__contains__ is ugly as sin.
>
> One idea: maybe meant as future-proofing against security bugs that could open up in future pythons. *I'll try to find blame (or whatever they call that in cvs) on this line of code, maybe we can ask the culprit himself.
annotate
>
>> Python functions are not perl, don't pass 'argv' to the function.
>>
>> If the function takes to arguments, it should be:
>>
>> def func1(arg1, arg2)
>> If some of the args have defaults:
>> def func1(arg1, arg2=5)
>
> Hm, are you looking at bin/portage_master_lock when you mention this? *It's cut-and-paste from portageq (starting to see a pattern yet?
)
Bad code in portage? INCONCEIVABLE.
>
>> I've never seen anyone use <> before; I didn't even know it existed.
>> Most folks use !=.
>
> Sounds right, I'll seek and destroy.
>
>> I can't really imagine why people do stuff like share
>> /var/lib/portage
>> across machines, so adding a bunch of complexity just to make it work
>> seems nuts to me. That being said I have not worked on portage in
>> years and Zac seems happier to support weird use cases.
>
> Yikes, that's a pretty good critique, I'm afraid. *Almost painful to read
I'm a pretty blunt fellow.
>
> I decided to go ahead and implement this first, because I've seen some real-world file-sharing setups serving cygwin to clusters of development workstations, and secondly, because the consensus on #gentoo-portage was that this was a supported use-case.
Indeed, as I said I am not upstream, I'm just an opinionated dick
>
> Anyhow, seeing as how I already implemented it, it's too late for me to decide that it's too much work
>
> In my defense, I thought it would be way less involved and it just kinda feature-creep'ed up on me. *By the time I figured out what a monster I'd created I was kinda pot-stuck.
Its not the largest patch I've seen ;p
>
> -gmt
>
cygwin prefix patch: request for eyeballs
