Skip to content
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

Disable SecretService and KWallet backends if the corresponding names are not available on D-Bus #474

Merged
merged 3 commits into from Nov 7, 2020

Conversation

mitya57
Copy link
Collaborator

@mitya57 mitya57 commented Nov 6, 2020

First, check if the name is already taken by some application. If no, check if the service can be activated dynamically.

@takluyver Please review this if you have time. And thanks again for your suggestion!

This fixes #162, #434, #473.

@mitya57 mitya57 requested a review from jaraco November 6, 2020 18:57
@jaraco jaraco closed this Nov 6, 2020
@jaraco jaraco reopened this Nov 6, 2020
@takluyver
Copy link
Contributor

No problem, thank you for doing the work with it. :-)

For some reason I can't see the logs from the Azure pipelines builds that are failing - I don't know if that's a random glitch, or if they're set to private.

@jaraco
Copy link
Owner

jaraco commented Nov 6, 2020

Right - please disregard the Azure and Appveyor integrations. I'm migrating projects to github actions. I see I haven't done that here yet. I'll do that in master and then a rebase ought to clear things up.

@jaraco
Copy link
Owner

jaraco commented Nov 6, 2020

I've rebased to master to fix CI.

@mitya57
Copy link
Collaborator Author

mitya57 commented Nov 6, 2020

Thanks you @takluyver and @jaraco for your comments! I will address them tomorrow.

Copy link
Owner

@jaraco jaraco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regardless the solution you come up with, the general approach seems suitable.

mitya57 added a commit to mitya57/secretstorage that referenced this pull request Nov 7, 2020
@mitya57 mitya57 force-pushed the check-dbus-names branch 2 times, most recently from a934a2a to e002c7c Compare November 7, 2020 10:39
@takluyver
Copy link
Contributor

The code changes LGTM now, though I've not tried them locally.

Just to reiterate what I said on #473: this won't altogether eliminate this kind of hang, but I think that delays when calling keyring functions are more palatable than delays on import. I'm working on changes to Jeepney which will include the ability to set a timeout on blocking method calls, which we could use to limit how long it waits.

@mitya57
Copy link
Collaborator Author

mitya57 commented Nov 7, 2020

this won't altogether eliminate this kind of hang, but I think that delays when calling keyring functions are more palatable than delays on import.

I think there are many cases where keyring is imported but isn't going to be used. So for such users the problem will be solved.

I'm working on changes to Jeepney which will include the ability to set a timeout on blocking method calls, which we could use to limit how long it waits.

I saw that. Thanks!

I have made SecretStorage 3.2.0 release now, so this is ready for merge.

@jaraco jaraco merged commit 26595df into jaraco:master Nov 7, 2020
@mitya57 mitya57 deleted the check-dbus-names branch November 7, 2020 17:55
@jaraco
Copy link
Owner

jaraco commented Nov 7, 2020

Expect this build to release v21.5.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Import of keyring takes 25s - or DBusException
3 participants