[gentoo-dev] cygwin prefix patch: request for eyeballs

[gentoo-dev] cygwin prefix patch: request for eyeballs

[Gregory M. Turner]

Hello, I would appreciate if those of you with portage development experience and a moment to spare could please take a look at:

https://github.com/gmt/gmt-cygwin-overlay/blob/master/sys-apps/portage/files/portage-2.2.01.20271-cygdll_protect.patch

Not gunning for upstream or anything, but I would eventually like to start moving towards "not a complete pile of crap" status :)

Notes:

  o based against a-few-weeks-old rsync prefix-portage

  o may contain a very few hunks that should arguably go
    upstream--I will make bugs for them and they are not
    what (I feel) needs eyeballs.

  o trying to actually run this may be difficult.  let me
    know where you're stuck and I'll try to help.

  o I must admit, I pretty much learned python as I went,
    so... if you are thinking "that looks like a total
    rookie mistake" it probably is, and I'd probably
    appreciate your constructive critique.

  o I'll probably keep hacking on this so... moving target.
    But it's mostly working, I think.

  o This doesn't make any big changes to upstream code.  So,
    If it looks like it might be a big indentation change,
    it probably is.

  o this is to solve the problem that, in cygwin, a running python
    crashes as soon as you change any filesystem .dll's on which
    it depends and fork() -- this is semi-orthogonal to the
    typical cygwin "rebasing" problem and distinct from it.
    The problem has nothing to do with locked files (except
    that my solution does use some standard portage file-
    locking features).

-gmt

Re: [gentoo-dev] cygwin prefix patch: request for eyeballs

[Alec Warner]

On Mon, Mar 26, 2012 at 4:12 PM, Gregory M. Turner <gmt@malth.us> wrote:
> Hello, I would appreciate if those of you with portage development experience and a moment to spare could please take a look at:
>
> https://github.com/gmt/gmt-cygwin-overlay/blob/master/sys-apps/portage/files/portage-2.2.01.20271-cygdll_protect.patch
>
> Not gunning for upstream or anything, but I would eventually like to start moving towards "not a complete pile of crap" status :)
>
> Notes:

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)

Assuming $PORTAGE_BASH is always bash, you should use [[ consistently
in your bash.

IMHO it is a common mistake to write your own argument processing, is
there a reason getops is not appropriate (portability?)

Onto the python:

if os.environ.__contains__("PORTAGE_PYTHONPATH"):

Any reason why
if "PORTAGE_PYTHONPATH" not in os.environ:
was not used?

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)

>
>  o based against a-few-weeks-old rsync prefix-portage
>
>  o may contain a very few hunks that should arguably go
>    upstream--I will make bugs for them and they are not
>    what (I feel) needs eyeballs.
>
>  o trying to actually run this may be difficult.  let me
>    know where you're stuck and I'll try to help.
>
>  o I must admit, I pretty much learned python as I went,
>    so... if you are thinking "that looks like a total
>    rookie mistake" it probably is, and I'd probably
>    appreciate your constructive critique.

I've never seen anyone use <> before; I didn't even know it existed.
Most folks use !=.

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.

>
>  o I'll probably keep hacking on this so... moving target.
>    But it's mostly working, I think.
>
>  o This doesn't make any big changes to upstream code.  So,
>    If it looks like it might be a big indentation change,
>    it probably is.
>
>  o this is to solve the problem that, in cygwin, a running python
>    crashes as soon as you change any filesystem .dll's on which
>    it depends and fork() -- this is semi-orthogonal to the
>    typical cygwin "rebasing" problem and distinct from it.
>    The problem has nothing to do with locked files (except
>    that my solution does use some standard portage file-
>    locking features).
>
> -gmt
>

Re: [gentoo-dev] cygwin prefix patch: request for eyeballs

[Gregory M. Turner]

----- 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 :)

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.

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.

> 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? :) )

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

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.

-gmt

Re: [gentoo-dev] cygwin prefix patch: request for eyeballs

[Alec Warner]

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
>

Re: [gentoo-dev] cygwin prefix patch: request for eyeballs

[Gregory M. Turner]

----- Original Message -----
> > 'if not os.environ["PORTAGE_PYTONPATH"]:' 
> 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.

I somehow got the idea that the python gods had sprinkled magical syntax-sugar on bool(x[y])?  But maybe that doesn't make sense, it could cause gross surprises.  Anyhow I should rtfm rather than post my OT un-educated guesses...

-gmt