New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GUdev pygobject migration #5

Merged
merged 3 commits into from Aug 9, 2014

Conversation

Projects
None yet
2 participants
@javihernandez
Collaborator

javihernandez commented Apr 6, 2012

Hi Felix!

I've been trying to use Udev Discover under openSUSE 12.1 and I turned into an issue.
I know that actually, Udev Discover is partially ported to the new GObject Introspection based bindings, but gudev migration is missing, because it was not possible to get done when the migration happened. Since pygobject doesn't support this situation from the 2.90.3 version, udev-discover needs this patch if we want to keep it working in newest distributions.

So, here's a patch! ;)
I've ported the remaining code, and all seems fine to me, but I have a question.

I removed the 'subsystems' argument while I'm getting some 'GUdev.Client' objects (see lines below)

  • self.client = gudev.Client(subsystems)
  • self.client = GUdev.Client()

Don't know the real impact of this, so it will be great if you make a review.

JFYI, I'm using GUdev from git (version 182) because there were some missing annotations, like the 'Device.get_parent' one.

Best regards!

@fontanon

This comment has been minimized.

Show comment
Hide comment
@fontanon

fontanon Apr 7, 2012

Setting ' ' as argument for gudev.Client tells him to deal with every subsystem. I've to check the replacement in GUdev.Client keeps the effect.

fontanon commented on udevdiscover/devicefinder.py in 521f84b Apr 7, 2012

Setting ' ' as argument for gudev.Client tells him to deal with every subsystem. I've to check the replacement in GUdev.Client keeps the effect.

@fontanon

This comment has been minimized.

Show comment
Hide comment
@fontanon

fontanon Apr 7, 2012

The subsystems argument is the way to allow the user to specify whether a subsystem must be ignored or dealt with.
This has a severe impact on an udev-discover's key feature: the ability to choose a deal with a slice of the kernel subsystems only.

fontanon commented on udevdiscover/devicefinder.py in 521f84b Apr 7, 2012

The subsystems argument is the way to allow the user to specify whether a subsystem must be ignored or dealt with.
This has a severe impact on an udev-discover's key feature: the ability to choose a deal with a slice of the kernel subsystems only.

@fontanon

This comment has been minimized.

Show comment
Hide comment
@fontanon

fontanon Apr 7, 2012

Owner

Awsome!
The point is that I figure some of this changes will break an udev-discover's key feature: user's ability to choose a slice of kernel subsystems to deal with. I've done some comments on the patch, please check it.

So. which is the best environment to try myself the patch working? Any OS with GUdev r182?

Thanks!

Owner

fontanon commented Apr 7, 2012

Awsome!
The point is that I figure some of this changes will break an udev-discover's key feature: user's ability to choose a slice of kernel subsystems to deal with. I've done some comments on the patch, please check it.

So. which is the best environment to try myself the patch working? Any OS with GUdev r182?

Thanks!

@javihernandez

This comment has been minimized.

Show comment
Hide comment
@javihernandez

javihernandez Apr 7, 2012

Collaborator

Hey, thanks for your quick reply! ;)

Since my two boxes are running Gnome 3.2 environments, I have no way of trying the latest udev-discover stable release.

About my environment, I'm under openSUSE 12.1, with udev 1 from git, and kmod 2 from git too (as a required dependency for udev). You can also try with Fedora 16 or with the upcoming Ubuntu 12.04. Don't know about the minimum required udev version in each distribution, but you'll have to ensure that the udev version you're using includes this pretty commit 3

About the behaviour in GUdev.Client method, Sorry, I overlooked the "new" method, so I've switched (and pushed to this pull request related branch) all the GUdev.Client calls into GUdev.Client.new ones. In this way, we can pass the "subsystems" parameter and all seems to work as expected.

Anyway, I think that pinging udev developers about the use of the gudev bindings will be very useful here, AFAIK we should be able to use the GUdev.Client method directly.

Regards!

Collaborator

javihernandez commented Apr 7, 2012

Hey, thanks for your quick reply! ;)

Since my two boxes are running Gnome 3.2 environments, I have no way of trying the latest udev-discover stable release.

About my environment, I'm under openSUSE 12.1, with udev 1 from git, and kmod 2 from git too (as a required dependency for udev). You can also try with Fedora 16 or with the upcoming Ubuntu 12.04. Don't know about the minimum required udev version in each distribution, but you'll have to ensure that the udev version you're using includes this pretty commit 3

About the behaviour in GUdev.Client method, Sorry, I overlooked the "new" method, so I've switched (and pushed to this pull request related branch) all the GUdev.Client calls into GUdev.Client.new ones. In this way, we can pass the "subsystems" parameter and all seems to work as expected.

Anyway, I think that pinging udev developers about the use of the gudev bindings will be very useful here, AFAIK we should be able to use the GUdev.Client method directly.

Regards!

@fontanon

This comment has been minimized.

Show comment
Hide comment
@fontanon

fontanon Apr 14, 2012

Owner

I've added you as collaborator. Please feel free to merge.
What would be more convenient for merge the pull request?:
1.- Into master
2.- Into a new branch for git version of udev

Best regards!

Owner

fontanon commented Apr 14, 2012

I've added you as collaborator. Please feel free to merge.
What would be more convenient for merge the pull request?:
1.- Into master
2.- Into a new branch for git version of udev

Best regards!

@javihernandez

This comment has been minimized.

Show comment
Hide comment
@javihernandez

javihernandez Apr 15, 2012

Collaborator

Hi!

Thanks for giving me such privileges!

About where to push ...

Since we need a very recent version of udev, we should wait until we know when distributions are planning to include such udev version, so we just can create a new branch as you've suggested and including an up-to-date README file with the specific branch information. I think 'udev-178' could be the branch name. Ideas?

About the use of GUdev.Client, I talked with pygobject developers and told me to file a bug in gudev. I did it, and got a response 1 from David Zeuthen, so "the best practice" in passing the arguments still needs to be discussed a little bit. Anyway, we're ok to go with the "GUdev.Client.new" method. ;)

Regards!

Collaborator

javihernandez commented Apr 15, 2012

Hi!

Thanks for giving me such privileges!

About where to push ...

Since we need a very recent version of udev, we should wait until we know when distributions are planning to include such udev version, so we just can create a new branch as you've suggested and including an up-to-date README file with the specific branch information. I think 'udev-178' could be the branch name. Ideas?

About the use of GUdev.Client, I talked with pygobject developers and told me to file a bug in gudev. I did it, and got a response 1 from David Zeuthen, so "the best practice" in passing the arguments still needs to be discussed a little bit. Anyway, we're ok to go with the "GUdev.Client.new" method. ;)

Regards!

@fontanon

This comment has been minimized.

Show comment
Hide comment
@fontanon

fontanon Apr 22, 2012

Owner

Will you create this new branch? What's the next step?

Owner

fontanon commented Apr 22, 2012

Will you create this new branch? What's the next step?

fontanon added a commit that referenced this pull request Aug 9, 2014

@fontanon fontanon merged commit be77bf0 into fontanon:master Aug 9, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment