Bug#733489: Remaining patches for bug #733489

[Michael Schaller]

Friendly ping to Julian...

Bug#733489: Remaining patches for bug #733489

[Julian Andres Klode]

On Fri, Sep 11, 2015 at 11:47:22AM +0200, Michael Schaller wrote:
> Friendly ping to Julian...
> 

Merged into debian/sid in commit
  da965cc17daec4dfb25b6d05e435a0bfc8f1e33f

Thank you for your contribution.

-- 
Julian Andres Klode  - Debian Developer, Ubuntu Member

See http://wiki.debian.org/JulianAndresKlode and http://jak-linux.org/.

Be friendly, do not top-post, and follow RFC 1855 "Netiquette".
- If you don't I might ignore you.

Bug#733489: Remaining patches for bug #733489

[Michael Schaller]

Awesome. Thanks a million, Julian!

About the missing 'doc/source/whatsnew/1.0' update.  I think these 
commits could be relevant:

https://anonscm.debian.org/cgit/apt/python-apt.git/commit/?id=8ad17a7fbc01935eb5f7cb252631e6d7d2ebedf7
https://anonscm.debian.org/cgit/apt/python-apt.git/commit/?id=fdd173dd444098ed533cbcd541a7f10f228bc47e
https://anonscm.debian.org/cgit/apt/python-apt.git/commit/?id=2a142993b99075f4d14ec0486270039c876cc5d8
https://anonscm.debian.org/cgit/apt/python-apt.git/commit/?id=562f51ea28b3dd9e5049972102e20753884bef8c
https://anonscm.debian.org/cgit/apt/python-apt.git/commit/?id=3d0da5e5a08d22a3b236ca8910ca090c6e8b5205
https://anonscm.debian.org/cgit/apt/python-apt.git/commit/?id=48eeabf982134ecfac3b915726cedd7bfe81adc1
https://anonscm.debian.org/cgit/apt/python-apt.git/commit/?id=c1fc0bcd3a509000ca09b9d4c22fb9e9032b37c7
https://anonscm.debian.org/cgit/apt/python-apt.git/commit/?id=12b349e0c6e9d4fd6d73a694c797c3a835e8d9ea
https://anonscm.debian.org/cgit/apt/python-apt.git/commit/?id=93790910dca7870c0e4912d1aed59d5380d2eccd


Julian, how much of this would you actually add to the 
'doc/source/whatsnew/1.0' file?  In the end my primary goal was to add 
the 'target_versions' methods to the Dependency and BaseDependency 
objects which essentially fixes bug #733489.


With the 'target_versions' methods one is finally able to build a graph 
as long as one keeps in mind that edges between apt.Package and 
apt.Version and between apt.Dependency and apt.Version are in an 
or-relationship.  The or-relationship can be easily implemented by 
giving all edges a probability attribute and the edges in an 
or-relationship then have a probability < 1.0.

Bug#733489: Remaining patches for bug #733489

[Michael Schaller]

Julian, I've went ahead and implemented the changes you've requested and 
updated my development branch:

https://github.com/michael-schaller/python-apt/commits/dev

Bug#733489: Remaining patches for bug #733489

[Michael Schaller]

On 09/06/2015 11:48 AM, Julian Andres Klode wrote:

On Sun, Sep 06, 2015 at 11:14:28AM +0200, Michael Schaller wrote:
* You don't need to assign "tvers = self.target_versions" before
   the list comprehension, that just reduces readability. Better
   split the comprehension over two lines if neede


Done.


* Version.is_installed has no .. versionadded:: 1.0.0


Done.


* The changed __eq__ method might want a

   .. versionchanged:: 1.0.0

I've added a docstring to _cmp.  This seemed a better idea than adding 
docstrings to __eq__, __gt__, ... .



* I don't really understand why you add type comments everywhere.

There are already such comments in other places in the code base and 
whenever I use python-apt for projects at work such comments make it 
easier for the reviewer to understand what is going on between apt and 
apt_pkg.  So it's kind of a habit...


Julian, should I remove them?


And I really need the changes documented in doc/source/whatsnew/1.0
add some point, otherwise people do not notice them. But I guess
that's my work, as I still need to add your other changes I merged
there.

Let's finish the current patches and then discuss this topic.  I'd 
rather avoid making this too chaotic again.  ;-)

Bug#733489: Remaining patches for bug #733489

[Julian Andres Klode]

On Sun, Sep 06, 2015 at 11:14:28AM +0200, Michael Schaller wrote:
> Julian, I've went ahead and implemented the changes you've requested and
> updated my development branch:
> https://github.com/michael-schaller/python-apt/commits/dev

Thanks, we're almost done. I was about to merge this, but noticed
a few things:

* You don't need to assign "tvers = self.target_versions" before
  the list comprehension, that just reduces readability. Better
  split the comprehension over two lines if neede

* Version.is_installed has no .. versionadded:: 1.0.0

* The changed __eq__ method might want a

  .. versionchanged:: 1.0.0

* I don't really understand why you add type comments everywhere.

And I really need the changes documented in doc/source/whatsnew/1.0
add some point, otherwise people do not notice them. But I guess
that's my work, as I still need to add your other changes I merged
there.
-- 
Julian Andres Klode  - Debian Developer, Ubuntu Member

See http://wiki.debian.org/JulianAndresKlode and http://jak-linux.org/.

Be friendly, do not top-post, and follow RFC 1855 "Netiquette".
- If you don't I might ignore you.

Bug#733489: Remaining patches for bug #733489

[Michael Schaller]

Friendly ping to Julian.  Julian, do you think we can finish this before 
your University courses start again?

Bug#733489: Remaining patches for bug #733489

[Julian Andres Klode]

On Wed, Sep 2, 2015 at 10:34 AM, Michael Schaller  wrote:
> Friendly ping to Julian.  Julian, do you think we can finish this before
> your University courses start again?
>
Sure. I'm sorry I forgot to send this out, but here we go:

There are some things I don't like currently:

helpers/filter patch:
* The 'for' loops building lists should be comprehensions.
* The early return in is_installed does not make sense, use an 'and' instead
* The 'if' in InstalledFilter.apply should go away, simply return the value.

target_versions patch:
* We cannot simply create a new Package() object, the Cache object keeps
  a weak reference to all packages. This needs a slight refactoring by
extracting a
  method out of Cache.__getitem__ that creates the apt.Package for the
apt_pkg.Package.

* I'm not sure if Version.__eq__ or whatever should take the name into
consideration,
  it probably should, this would remove the need for the hack and the
long comment.


-- 
Julian Andres Klode  - Debian Developer, Ubuntu Member

See http://wiki.debian.org/JulianAndresKlode and http://jak-linux.org/.

Bug#733489: Remaining patches for bug #733489

[Michael Schaller]

On 09/02/2015 11:28 AM, Julian Andres Klode wrote:

On Wed, Sep 2, 2015 at 10:34 AM, Michael Schaller  wrote:

Friendly ping to Julian.  Julian, do you think we can finish this before
your University courses start again?


Sure. I'm sorry I forgot to send this out, but here we go:


No worries.  ;-)


There are some things I don't like currently:

helpers/filter patch:
* The 'for' loops building lists should be comprehensions.
* The early return in is_installed does not make sense, use an 'and' instead
* The 'if' in InstalledFilter.apply should go away, simply return the value.


Sure.  SGTM.


target_versions patch:
* We cannot simply create a new Package() object, the Cache object keeps
   a weak reference to all packages. This needs a slight refactoring by
extracting a
   method out of Cache.__getitem__ that creates the apt.Package for the
apt_pkg.Package.

I see.  Looking at cache.py I think the best would be to introduce a 
Cache._rawpkg_to_pkg method.  This would get rid of some code 
duplication in the Cache class as well as similar code is used in 
several places.

Julian, is this what you had in mind?


* I'm not sure if Version.__eq__ or whatever should take the name into
consideration,
   it probably should, this would remove the need for the hack and the
long comment.


I'm not sure about how much code relies on this broken behavior.  If 
this isn't a concern for you I'm happy to fix this.

Bug#733489: Remaining patches for bug #733489

[Michael Schaller]

Done.  Please take another look on the last two patches on this branch:
https://github.com/michael-schaller/python-apt/commits/dev

Node:  With the removal of TargetVersionList I had to duplicate the 
code for the installed_target_versions properties.

Bug#733489: Remaining patches for bug #733489

[Michael Schaller]

Fair points.  I'll update the commit and ping you once it is ready for 
review.

Bug#733489: Remaining patches for bug #733489

[Julian Andres Klode]

On Fri, Aug 21, 2015 at 11:43:41AM +0200, Michael Schaller wrote:
 On 08/20/2015 10:07 PM, Julian Andres Klode wrote:
 On Thu, Aug 20, 2015 at 09:58:11PM +0200, Michael Schaller wrote:
 
 Hi Michael, I think it looks OK, but let me suggest some
 changes:
 
 - Drop the parent_version property and use _version directly
 - Drop the cache property and use _cache directly
 - Drop the TargetVersionList. AFAIK, the only thing different
is it wrapping the list representation in a TargetVersionList
thing, this does not seem to be needed (there's no point
creating our own list type just for some corner-case
cosmetic issue).
 
 Then the patch will be much smaller, and still be able to do
 the same.
 
 
 I can remove the 'parent_version' and 'cache' properties but would like to
 know your reasoning.  From my point of view these properties just make the
 code more consistent with the other properties that are already there.  It
 is unfortunate though that I can't name the 'parent_version' property
 'version' as that would be even more consistent.

There are no parent references in the other objects, and it is
completely unrelated/unneeded to the patch in question anyway.

 
 I would also like to keep the 'TargetVersionList' class as I quite often use
 the interactive Python console with python-apt during development and there
 it helps a lot to have a good '__repr__' implementation.
 
 Take for an example this interactive Python session.  I've replaced ' '
 with a newline for better readability/mailability.
 
 $ python
  import apt
 
 cache = apt.Cache()
 
 cache
 apt.cache.Cache object at 0x7fef993d6210
 
 pkg = cache['bash']
 
 pkg
 Package: name:'bash' architecture='amd64' id:738L
 
 pkg.cache
 apt.cache.Cache object at 0x7fef993d6210
 
 ver = pkg.installed
 
 ver
 Version: package:'bash' version:'4.3-11ubuntu2'
 
 ver.package
 Package: name:'bash' architecture='amd64' id:738L
 
 deps = ver.get_dependencies('Depends')
 
 deps
 [Dependency: [BaseDependency: name:'base-files' relation:'='
 version:'2.1.12' rawtype:'Depends'], Dependency: [BaseDependency:
 name:'debianutils' relation:'=' version:'2.15' rawtype:'Depends']]
 
 deps[0]
 Dependency: [BaseDependency: name:'base-files' relation:'='
 version:'2.1.12' rawtype:'Depends']
 
 deps[0].parent_version
 Version: package:'bash' version:'4.3-11ubuntu2'
 
 deps[0].target_versions
 TargetVersionList: [Version: package:'base-files' version:'7.2ubuntu9',
 Version: package:'base-files:i386' version:'7.2ubuntu9']

The only advantage is the prefix, and that's not enough. This is a
dynamically typed language after all, there's no point having a
special type, that's completely non-Pythonic.

We only have this for versions in VersionList, because we need/want
to treat it like a dictionary.

 
 
 BTW: From my point of view 'Version.__repr__' should also include the
 architecture and the 'id' in 'Package.__repr__' is rather useless.

The architecture *is* included - implicitly, in the name - it is the
pretty-printed package name, that is the name for the native arch,
and name:arch for a foreign arch.

The ID is a bit useless, but there's no point dropping it and it 
can help in some situations where you accidentally have objects
from different caches.

-- 
Julian Andres Klode  - Debian Developer, Ubuntu Member

See http://wiki.debian.org/JulianAndresKlode and http://jak-linux.org/.

Be friendly, do not top-post, and follow RFC 1855 Netiquette.
- If you don't I might ignore you.

Bug#733489: Remaining patches for bug #733489

[Michael Schaller]

On 08/20/2015 10:07 PM, Julian Andres Klode wrote:

On Thu, Aug 20, 2015 at 09:58:11PM +0200, Michael Schaller wrote:

Hi Michael, I think it looks OK, but let me suggest some
changes:

- Drop the parent_version property and use _version directly
- Drop the cache property and use _cache directly
- Drop the TargetVersionList. AFAIK, the only thing different
   is it wrapping the list representation in a TargetVersionList
   thing, this does not seem to be needed (there's no point
   creating our own list type just for some corner-case
   cosmetic issue).

Then the patch will be much smaller, and still be able to do
the same.



I can remove the 'parent_version' and 'cache' properties but would like 
to know your reasoning.  From my point of view these properties just 
make the code more consistent with the other properties that are already 
there.  It is unfortunate though that I can't name the 'parent_version' 
property 'version' as that would be even more consistent.


I would also like to keep the 'TargetVersionList' class as I quite often 
use the interactive Python console with python-apt during development 
and there it helps a lot to have a good '__repr__' implementation.


Take for an example this interactive Python session.  I've replaced ' 
' with a newline for better readability/mailability.


$ python
 import apt

cache = apt.Cache()

cache
apt.cache.Cache object at 0x7fef993d6210

pkg = cache['bash']

pkg
Package: name:'bash' architecture='amd64' id:738L

pkg.cache
apt.cache.Cache object at 0x7fef993d6210

ver = pkg.installed

ver
Version: package:'bash' version:'4.3-11ubuntu2'

ver.package
Package: name:'bash' architecture='amd64' id:738L

deps = ver.get_dependencies('Depends')

deps
[Dependency: [BaseDependency: name:'base-files' relation:'=' 
version:'2.1.12' rawtype:'Depends'], Dependency: [BaseDependency: 
name:'debianutils' relation:'=' version:'2.15' rawtype:'Depends']]


deps[0]
Dependency: [BaseDependency: name:'base-files' relation:'=' 
version:'2.1.12' rawtype:'Depends']


deps[0].parent_version
Version: package:'bash' version:'4.3-11ubuntu2'

deps[0].target_versions
TargetVersionList: [Version: package:'base-files' 
version:'7.2ubuntu9', Version: package:'base-files:i386' 
version:'7.2ubuntu9']



BTW: From my point of view 'Version.__repr__' should also include the 
architecture and the 'id' in 'Package.__repr__' is rather useless.


Julian, what do you think?

Bug#733489: Remaining patches for bug #733489

[Michael Schaller]

Friendly ping to Julian.  I hope your exams went well...

Bug#733489: Remaining patches for bug #733489

[Julian Andres Klode]

On Thu, Aug 20, 2015 at 09:58:11PM +0200, Michael Schaller wrote:
 Friendly ping to Julian.  I hope your exams went well...
 

Hi Michael, I think it looks OK, but let me suggest some
changes:

- Drop the parent_version property and use _version directly
- Drop the cache property and use _cache directly
- Drop the TargetVersionList. AFAIK, the only thing different
  is it wrapping the list representation in a TargetVersionList
  thing, this does not seem to be needed (there's no point
  creating our own list type just for some corner-case
  cosmetic issue).

Then the patch will be much smaller, and still be able to do
the same.
-- 
Julian Andres Klode  - Debian Developer, Ubuntu Member

See http://wiki.debian.org/JulianAndresKlode and http://jak-linux.org/.

Be friendly, do not top-post, and follow RFC 1855 Netiquette.
- If you don't I might ignore you.

Bug#733489: Remaining patches for bug #733489

[Michael Schaller]

On 07/07/2015 09:00 PM, Julian Andres Klode wrote:

On Sat, Jul 04, 2015 at 03:45:48PM +0200, Michael Schaller wrote:

I merged all except for the last 2 commits, as they seem to introduce a
reference cycle from deps to versions. And we can't have that, as that
leads to enormous issues (#745487).

I might be wrong though, we probably create all dependencies lazily,
but it's still not a good idea to have a path up in the API.

I'll take a closer look at this in a few weeks and might redecide
then. University exams next week and in 3 weeks...



Thanks for merging the 4 patches.

Looking at the next commit in line, which would be 
https://github.com/michael-schaller/python-apt/commit/5dfd3c5, there 
shouldn't be an issue.


This commit changes the constructors of Dependency and BaseDependency to 
store the Version object they belong to.  This is in line with the 
Package and Version classes as they handle this the same way.


With this part of the commit you basically change the constructors from 
this:

Package(apt.package.Cache, apt_pkg.Package)
Version(apt.package.Package, apt_pkg.Version)
BaseDependency(apt_pkg.Dependency)
Dependency(list(apt.package.BaseDependency), string)

to this:
Package(apt.package.Cache, apt_pkg.Package)
Version(apt.package.Package, apt_pkg.Version)
BaseDependency(apt.package.Version, apt_pkg.Dependency)
Dependency(list(apt.package.Version, apt.package.BaseDependency), string)

As you can see there is no cycle in this arrangement.  The 
target_versions properties don't introduce a cycle either as they only 
create Version objects that don't backlink to a dependency.



Julian, btw, take your time with your exams.  Those are definitely more 
important than these patches. ;-)



--
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org

Bug#733489: Remaining patches for bug #733489

[Julian Andres Klode]

On Sat, Jul 04, 2015 at 03:45:48PM +0200, Michael Schaller wrote:
 Hi Julian,
 
 I've reviewed and reworked all the remaining patches I have for bug #733489.
 From my point of view 6 reasonably sized patches are remaining.  If you
 prefer single patches then I'll keep them coming. Alternatively here is my
 dev branch with all the remaining patches:
 https://github.com/michael-schaller/python-apt/commits/dev
 
 Hopefully we can close this bug soon. ;-)
 

I merged all except for the last 2 commits, as they seem to introduce a  
reference cycle from deps to versions. And we can't have that, as that
leads to enormous issues (#745487).

I might be wrong though, we probably create all dependencies lazily,
but it's still not a good idea to have a path up in the API.

I'll take a closer look at this in a few weeks and might redecide
then. University exams next week and in 3 weeks...

-- 
Julian Andres Klode  - Debian Developer, Ubuntu Member

See http://wiki.debian.org/JulianAndresKlode and http://jak-linux.org/.

Be friendly, do not top-post, and follow RFC 1855 Netiquette.
- If you don't I might ignore you.


-- 
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org

Bug#733489: Remaining patches for bug #733489

[Michael Schaller]

Hi Julian,

I've reviewed and reworked all the remaining patches I have for bug 
#733489.  From my point of view 6 reasonably sized patches are 
remaining.  If you prefer single patches then I'll keep them coming. 
Alternatively here is my dev branch with all the remaining patches:

https://github.com/michael-schaller/python-apt/commits/dev

Hopefully we can close this bug soon. ;-)

Best,

Michael

PS: Wish you a nice weekend.


--
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org