On Mon, Mar 26, 2012 at 11:39 PM, Gregory M. Turner <g...@malth.us> wrote: > ----- Original Message ----- >> On Mon, Mar 26, 2012 at 4:12 PM, Gregory M. Turner <g...@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 >
