[gentoo-portage-dev] [PATCH] ArchChecks: don't mix arches between ebuilds

[gentoo-portage-dev] [PATCH] ArchChecks: don't mix arches between ebuilds

[Zac Medico]

Fix ArchChecks to not mix arches of ebuilds together, so that
errors are only reported for those arches that the ebuild has
keywords for.
---
Applies to the *repoman* branch.

 pym/repoman/modules/scan/arches/arches.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/pym/repoman/modules/scan/arches/arches.py b/pym/repoman/modules/scan/arches/arches.py
index 4df25a8..6e1c17d 100644
--- a/pym/repoman/modules/scan/arches/arches.py
+++ b/pym/repoman/modules/scan/arches/arches.py
@@ -69,7 +69,7 @@ class ArchChecks(ScanBase):
 				arches.add(('**', '**', ('**',)))
 		# update the dynamic data
 		dyn_arches = kwargs.get('arches')
-		#dyn_arches.clear()
+		dyn_arches.clear()
 		dyn_arches.update(arches)
 		return False
 
-- 
2.7.4

Re: [gentoo-portage-dev] [PATCH] ArchChecks: don't mix arches between ebuilds

[Michał Górny]

[-- Attachment #1: Type: text/plain, Size: 1070 bytes --]

On Sat, 23 Apr 2016 16:57:21 -0700
Zac Medico <zmedico@gentoo.org> wrote:

> Fix ArchChecks to not mix arches of ebuilds together, so that
> errors are only reported for those arches that the ebuild has
> keywords for.
> ---
> Applies to the *repoman* branch.
> 
>  pym/repoman/modules/scan/arches/arches.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/pym/repoman/modules/scan/arches/arches.py b/pym/repoman/modules/scan/arches/arches.py
> index 4df25a8..6e1c17d 100644
> --- a/pym/repoman/modules/scan/arches/arches.py
> +++ b/pym/repoman/modules/scan/arches/arches.py
> @@ -69,7 +69,7 @@ class ArchChecks(ScanBase):
>  				arches.add(('**', '**', ('**',)))
>  		# update the dynamic data
>  		dyn_arches = kwargs.get('arches')
> -		#dyn_arches.clear()
> +		dyn_arches.clear()
>  		dyn_arches.update(arches)

Is that some crazy way of replacing contents of an existing dict? May
be worth a comment since the code looks suspicious, at least.

-- 
Best regards,
Michał Górny
<http://dev.gentoo.org/~mgorny/>

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 949 bytes --]

Re: [gentoo-portage-dev] [PATCH] ArchChecks: don't mix arches between ebuilds

[Brian Dolbec]

[-- Attachment #1: Type: text/plain, Size: 2151 bytes --]

On Sun, 24 Apr 2016 08:15:56 +0200
Michał Górny <mgorny@gentoo.org> wrote:

> On Sat, 23 Apr 2016 16:57:21 -0700
> Zac Medico <zmedico@gentoo.org> wrote:
> 
> > Fix ArchChecks to not mix arches of ebuilds together, so that
> > errors are only reported for those arches that the ebuild has
> > keywords for.
> > ---
> > Applies to the *repoman* branch.
> > 
> >  pym/repoman/modules/scan/arches/arches.py | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/pym/repoman/modules/scan/arches/arches.py
> > b/pym/repoman/modules/scan/arches/arches.py index 4df25a8..6e1c17d
> > 100644 --- a/pym/repoman/modules/scan/arches/arches.py
> > +++ b/pym/repoman/modules/scan/arches/arches.py
> > @@ -69,7 +69,7 @@ class ArchChecks(ScanBase):
> >  				arches.add(('**', '**', ('**',)))
> >  		# update the dynamic data
> >  		dyn_arches = kwargs.get('arches')
> > -		#dyn_arches.clear()
> > +		dyn_arches.clear()
> >  		dyn_arches.update(arches)  
> 
> Is that some crazy way of replacing contents of an existing dict? May
> be worth a comment since the code looks suspicious, at least.
> 

It was an interim step so that modules didn't pass back
arbitrary data to be re-used by other modules after it.  This way
maintains the data pointer to the set (not dict) object in the calling
function. Just assigning it a new set doesn't set the 'arches' pointer
in the calling function so that it can pass that data along to other
modules.

Instead it is getting an extended Future instance from asyncio to hold
the target data.  That also gives it the ability to raise an error if
the data is not set or something tries to set it a second time, etc..

BTW, the code to replace the contents of an existing dictionary is way
crazier...  It has to loop and pop() each item until empty, it does
not have a clear(). Then you can update it with the new contents.

But this step did prove the basic tightened up modules data interface.
And debugged the initial conversion, so I'll be coding the rest of the
conversion in the morning. Then I'll apply your xml patches.
-- 
Brian Dolbec <dolsen>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 951 bytes --]

Re: [gentoo-portage-dev] [PATCH] ArchChecks: don't mix arches between ebuilds

[Zac Medico]

On 04/24/2016 01:00 AM, Brian Dolbec wrote:
> On Sun, 24 Apr 2016 08:15:56 +0200
> Michał Górny <mgorny@gentoo.org> wrote:
> 
>> On Sat, 23 Apr 2016 16:57:21 -0700
>> Zac Medico <zmedico@gentoo.org> wrote:
>>
>>> Fix ArchChecks to not mix arches of ebuilds together, so that
>>> errors are only reported for those arches that the ebuild has
>>> keywords for.
>>> ---
>>> Applies to the *repoman* branch.
>>>
>>>  pym/repoman/modules/scan/arches/arches.py | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/pym/repoman/modules/scan/arches/arches.py
>>> b/pym/repoman/modules/scan/arches/arches.py index 4df25a8..6e1c17d
>>> 100644 --- a/pym/repoman/modules/scan/arches/arches.py
>>> +++ b/pym/repoman/modules/scan/arches/arches.py
>>> @@ -69,7 +69,7 @@ class ArchChecks(ScanBase):
>>>  				arches.add(('**', '**', ('**',)))
>>>  		# update the dynamic data
>>>  		dyn_arches = kwargs.get('arches')
>>> -		#dyn_arches.clear()
>>> +		dyn_arches.clear()
>>>  		dyn_arches.update(arches)  
>>
>> Is that some crazy way of replacing contents of an existing dict? May
>> be worth a comment since the code looks suspicious, at least.
>>
> 
> It was an interim step so that modules didn't pass back
> arbitrary data to be re-used by other modules after it.  This way
> maintains the data pointer to the set (not dict) object in the calling
> function. Just assigning it a new set doesn't set the 'arches' pointer
> in the calling function so that it can pass that data along to other
> modules.

I feel like the plugin structure has lead to decoupling of things that
really should be coupled. For example, the code in the ArchChecks could
be a function that's called by the ProfileDependsChecks plugin, given
that ProfileDependsChecks seems to be the only consumer of this data.
-- 
Thanks,
Zac

Re: [gentoo-portage-dev] [PATCH] ArchChecks: don't mix arches between ebuilds

[Zac Medico]

On 04/24/2016 02:45 PM, Zac Medico wrote:
> On 04/24/2016 01:00 AM, Brian Dolbec wrote:
>> On Sun, 24 Apr 2016 08:15:56 +0200
>> Michał Górny <mgorny@gentoo.org> wrote:
>>
>>> On Sat, 23 Apr 2016 16:57:21 -0700
>>> Zac Medico <zmedico@gentoo.org> wrote:
>>>
>>>> Fix ArchChecks to not mix arches of ebuilds together, so that
>>>> errors are only reported for those arches that the ebuild has
>>>> keywords for.
>>>> ---
>>>> Applies to the *repoman* branch.
>>>>
>>>>  pym/repoman/modules/scan/arches/arches.py | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/pym/repoman/modules/scan/arches/arches.py
>>>> b/pym/repoman/modules/scan/arches/arches.py index 4df25a8..6e1c17d
>>>> 100644 --- a/pym/repoman/modules/scan/arches/arches.py
>>>> +++ b/pym/repoman/modules/scan/arches/arches.py
>>>> @@ -69,7 +69,7 @@ class ArchChecks(ScanBase):
>>>>  				arches.add(('**', '**', ('**',)))
>>>>  		# update the dynamic data
>>>>  		dyn_arches = kwargs.get('arches')
>>>> -		#dyn_arches.clear()
>>>> +		dyn_arches.clear()
>>>>  		dyn_arches.update(arches)  
>>>
>>> Is that some crazy way of replacing contents of an existing dict? May
>>> be worth a comment since the code looks suspicious, at least.
>>>
>>
>> It was an interim step so that modules didn't pass back
>> arbitrary data to be re-used by other modules after it.  This way
>> maintains the data pointer to the set (not dict) object in the calling
>> function. Just assigning it a new set doesn't set the 'arches' pointer
>> in the calling function so that it can pass that data along to other
>> modules.
> 
> I feel like the plugin structure has lead to decoupling of things that
> really should be coupled. For example, the code in the ArchChecks could
> be a function that's called by the ProfileDependsChecks plugin, given
> that ProfileDependsChecks seems to be the only consumer of this data.

We'll have to go through all of the plugins and look for other things
that have too aggressively decoupled. The fact Scanner._scan_ebuilds
calls plugins in a hardcoded order is troubling. It means that there may
be coupling between plugins that makes the order matter, yet this
coupling is hidden and non-trivial to discover. If we couple more things
together where appropriate (like ArchChecks and ProfileDependsChecks),
then it will make the system easier for a person to reason about.
-- 
Thanks,
Zac

Re: [gentoo-portage-dev] [PATCH] ArchChecks: don't mix arches between ebuilds

[Zac Medico]

On 04/24/2016 03:04 PM, Zac Medico wrote:
> On 04/24/2016 02:45 PM, Zac Medico wrote:
>> On 04/24/2016 01:00 AM, Brian Dolbec wrote:
>>> On Sun, 24 Apr 2016 08:15:56 +0200
>>> Michał Górny <mgorny@gentoo.org> wrote:
>>>
>>>> On Sat, 23 Apr 2016 16:57:21 -0700
>>>> Zac Medico <zmedico@gentoo.org> wrote:
>>>>
>>>>> Fix ArchChecks to not mix arches of ebuilds together, so that
>>>>> errors are only reported for those arches that the ebuild has
>>>>> keywords for.
>>>>> ---
>>>>> Applies to the *repoman* branch.
>>>>>
>>>>>  pym/repoman/modules/scan/arches/arches.py | 2 +-
>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/pym/repoman/modules/scan/arches/arches.py
>>>>> b/pym/repoman/modules/scan/arches/arches.py index 4df25a8..6e1c17d
>>>>> 100644 --- a/pym/repoman/modules/scan/arches/arches.py
>>>>> +++ b/pym/repoman/modules/scan/arches/arches.py
>>>>> @@ -69,7 +69,7 @@ class ArchChecks(ScanBase):
>>>>>  				arches.add(('**', '**', ('**',)))
>>>>>  		# update the dynamic data
>>>>>  		dyn_arches = kwargs.get('arches')
>>>>> -		#dyn_arches.clear()
>>>>> +		dyn_arches.clear()
>>>>>  		dyn_arches.update(arches)  
>>>>
>>>> Is that some crazy way of replacing contents of an existing dict? May
>>>> be worth a comment since the code looks suspicious, at least.
>>>>
>>>
>>> It was an interim step so that modules didn't pass back
>>> arbitrary data to be re-used by other modules after it.  This way
>>> maintains the data pointer to the set (not dict) object in the calling
>>> function. Just assigning it a new set doesn't set the 'arches' pointer
>>> in the calling function so that it can pass that data along to other
>>> modules.
>>
>> I feel like the plugin structure has lead to decoupling of things that
>> really should be coupled. For example, the code in the ArchChecks could
>> be a function that's called by the ProfileDependsChecks plugin, given
>> that ProfileDependsChecks seems to be the only consumer of this data.
> 
> We'll have to go through all of the plugins and look for other things
> that have too aggressively decoupled. The fact Scanner._scan_ebuilds
> calls plugins in a hardcoded order is troubling. It means that there may
> be coupling between plugins that makes the order matter, yet this
> coupling is hidden and non-trivial to discover. If we couple more things
> together where appropriate (like ArchChecks and ProfileDependsChecks),
> then it will make the system easier for a person to reason about.

I think "encapsulation" is a good word for this theme. See, ArchChecks
should have been encapsulated by ProfileDependsChecks. Having ArchChecks
decoupled the way that it is only makes the system more difficult to
reason about, due to lack of encapsulation.
-- 
Thanks,
Zac

Re: [gentoo-portage-dev] [PATCH] ArchChecks: don't mix arches between ebuilds

[Brian Dolbec]

On Sun, 24 Apr 2016 15:04:57 -0700
Zac Medico <zmedico@gentoo.org> wrote:

> On 04/24/2016 02:45 PM, Zac Medico wrote:
> > On 04/24/2016 01:00 AM, Brian Dolbec wrote:  
> >> On Sun, 24 Apr 2016 08:15:56 +0200
> >> Michał Górny <mgorny@gentoo.org> wrote:
> >>  
> >>> On Sat, 23 Apr 2016 16:57:21 -0700
> >>> Zac Medico <zmedico@gentoo.org> wrote:
> >>>  
> >>>> Fix ArchChecks to not mix arches of ebuilds together, so that
> >>>> errors are only reported for those arches that the ebuild has
> >>>> keywords for.
> >>>> ---
> >>>> Applies to the *repoman* branch.
> >>>>
> >>>>  pym/repoman/modules/scan/arches/arches.py | 2 +-
> >>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/pym/repoman/modules/scan/arches/arches.py
> >>>> b/pym/repoman/modules/scan/arches/arches.py index
> >>>> 4df25a8..6e1c17d 100644 ---
> >>>> a/pym/repoman/modules/scan/arches/arches.py +++
> >>>> b/pym/repoman/modules/scan/arches/arches.py @@ -69,7 +69,7 @@
> >>>> class ArchChecks(ScanBase): arches.add(('**', '**', ('**',)))
> >>>>  		# update the dynamic data
> >>>>  		dyn_arches = kwargs.get('arches')
> >>>> -		#dyn_arches.clear()
> >>>> +		dyn_arches.clear()
> >>>>  		dyn_arches.update(arches)    
> >>>
> >>> Is that some crazy way of replacing contents of an existing dict?
> >>> May be worth a comment since the code looks suspicious, at least.
> >>>  
> >>
> >> It was an interim step so that modules didn't pass back
> >> arbitrary data to be re-used by other modules after it.  This way
> >> maintains the data pointer to the set (not dict) object in the
> >> calling function. Just assigning it a new set doesn't set the
> >> 'arches' pointer in the calling function so that it can pass that
> >> data along to other modules.  
> > 
> > I feel like the plugin structure has lead to decoupling of things
> > that really should be coupled. For example, the code in the
> > ArchChecks could be a function that's called by the
> > ProfileDependsChecks plugin, given that ProfileDependsChecks seems
> > to be the only consumer of this data.  
> 
> We'll have to go through all of the plugins and look for other things
> that have too aggressively decoupled. The fact Scanner._scan_ebuilds
> calls plugins in a hardcoded order is troubling. It means that there
> may be coupling between plugins that makes the order matter, yet this
> coupling is hidden and non-trivial to discover. If we couple more
> things together where appropriate (like ArchChecks and
> ProfileDependsChecks), then it will make the system easier for a
> person to reason about.

Yes, I agree, but the split out process was a long tedious one.  And in
making this module system, I did them in the order they were run
originally.  The code was split up all over with more and more things
added to the spaghetti code that ended up at 1000+ lines of code for
the one pkg & ebuild loop.  It's no wonder things got split badly.

The code now can be given decent scrutiny to optimize and refactor
where appropriate.  Like re-coupling some split up code.

-- 
Brian Dolbec <dolsen>