search

cocagne / txdbus

Native Python implementation of DBus for Twisted
MIT License
61 stars 38 forks source link

Fix issue #62 - Invalid name "pn" in router.py. #74

Closed exvito closed 7 years ago

exvito commented 7 years ago

Here's the PR fixing #62 like I commented in #64.

Let's wait for CI to go green and then please review and merge of ok.

exvito commented 7 years ago

Yikes... Codecov is rightfully complaining that this PR is changing untested code... sigh

Ideas? :)

exvito commented 7 years ago

Hmmm... New test failing everywhere on Travis, with the same error output:

[ERROR]
Traceback (most recent call last):
Failure: twisted.internet.defer.TimeoutError: <tests.test_client_against_native_bus.SignalTester testMethod=test_path_namespace_rule_match> (test_path_namespace_rule_match) still running at 120.0 secs
tests.test_client_against_native_bus.SignalTester.test_path_namespace_rule_match
===============================================================================
[ERROR]
Traceback (most recent call last):
Failure: txdbus.error.RemoteError: org.freedesktop.DBus.Error.MatchRuleInvalid: Unknown key "path_namespace" in match rule
tests.test_client_against_native_bus.SignalTester.test_path_namespace_rule_match

It runs fine on my dev system. Suspicion: Travis dbus version is old enough that it does not support path_namespace in rules. :/

Will investigate more.

WhyNotHugo commented 7 years ago

Can you somehow detect DBus version via DBus itself? Maybe we can skip the test if the version mismatches (or is will known incompatible version).

exvito commented 7 years ago

That feels complex and fragile. I'm trying to see:

exvito commented 7 years ago

Ok, current Travis is running DBus 1.4.18. Looking at the source, no references to "path_namespace" are found at all, which makes sense.

Will try to ask Travis to run under a different distribution. AFAICT, it defaults to Ubuntu 12.04 which includes DBus 1.4.18. Requesting Ubuntu 14.04 may be possible which is nice because it includes DBus 1.6.18 which, from peeking at the sources, includes references to "path_namespace".

Let's see how it goes...

exvito commented 7 years ago

Hmmmm... Learning Travis through reading docs, trial and error.

It seems we've been running container based which has its own way of installing additional whitelisted packages. Apparently DBus isn't on that list (https://github.com/travis-ci/apt-source-whitelist) so we'll have to 'sudo: required' and go back to VM-based (I guess?) infra.

Let's see how it goes. :/

exvito commented 7 years ago

Last failure says "dbus" is installed but our command "dbus-run-session" is missing (recommended instead of "dbus-launch" see [1] and [2]). I guess I'll have to do a quick VM install to check which package contains that...

Will try @hobarrera suggestion, going back to "sudo: false" in the meantime.

[1] https://patchwork.kernel.org/patch/9338383/ [2] https://mail.gnome.org/archives/networkmanager-list/2016-January/msg00023.html

exvito commented 7 years ago

More investigation on Ubuntu 14.04 filelists:

Result:

WhyNotHugo commented 7 years ago

Thanks for all the time and research put into this! 🎉

exvito commented 7 years ago

Ok, tests went green, finally! :)

This was quite a journey for a single source-code line fix... I welcome reviewing and merging if ok. Thanks.

exvito commented 7 years ago

Ok, I'll proceed to merging after @hobarrera's review. Merging will close #62.