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

Get password storage running on GNOME #10274

Closed
koppor opened this issue Sep 1, 2023 · 15 comments · Fixed by #10332
Closed

Get password storage running on GNOME #10274

koppor opened this issue Sep 1, 2023 · 15 comments · Fixed by #10332

Comments

@koppor
Copy link
Member

koppor commented Sep 1, 2023

@credmond
Copy link
Contributor

credmond commented Sep 2, 2023

Hi -- the fault is not the keyring library, but a clash in dependencies. I can successfully use the keyring library in a simpler project, but actually I initially encountered the same problem when also using another library with a different ASM lib version.

Can be fixed by ensuring only one (and correct) version of ASM is on classpath.

See the clashes:

image

See the error caught, swallowed, and null returned:

image

I'm not a Gradle expert (Maven is my strength) and I don't know how to tweak transitive dependencies, etc., or the usages of the conflicting libs, etc. within Jabref -- so I'll leave the fix up to someone else, but that's definitely the problem / solution; so I hope that helps.

Here's how it should look (note, different project!):

image

Happy to re-test, anytime.

@Siedlerchr
Copy link
Member

TODO investitage: Either we exclude them or we try to enforce only one valid version with strictly.

@koppor
Copy link
Member Author

koppor commented Sep 5, 2023

Hi -- the fault is not the keyring library, but a clash in dependencies.

I wonder why "Scope: All" is checked. Isn't the issue at the Runtime?

asm doesn't appear at as Runtime Dependency at all?

image

@credmond
Copy link
Contributor

credmond commented Sep 5, 2023

Hi -- the fault is not the keyring library, but a clash in dependencies.

I wonder why "Scope: All" is checked. Isn't the issue at the Runtime?

Scope: All is checked because that's just the IntelliJ default view, it doesn't mean anything...

asm doesn't appear at as Runtime Dependency at all?

Well, I'm a Maven guy -- not Gradle, but I'm pretty sure it's the same in Gradle, a "runtime" scope means a dependency is not needed to compile (i.e., won't be on compile classpath), but is needed to be available at runtime (will be on runtime classpath). "Compile" scope in Maven, is a default (meaning lib will be on both compile and runtime classpaths).

A "compile" scope dependency (again, usually the default in Maven) will not show up as "runtime" in that IntelliJ tool, at least in Maven -- even though it's going to mean it is on both classpaths. I see this dep on my Maven project, correctly, under compile time scope, and not runtime.

I am surprised it isn't showing up as a compile dependency in the Jabref project IntelliJ either though (I just did a quick check). I don't understand all of those different scopes in Gradle. Might be related!

Easiest thing to do, in my view, is log the actual Java command that Gradle is using to run the project, and seeing what's libs are there... and run it yourself, and tweak / change versions as necessary, by hand, to see what fixes the issue.

@koppor
Copy link
Member Author

koppor commented Sep 5, 2023

Hi -- the fault is not the keyring library,

I am not sure about this.

In the CI of keyring, com.github.javakeyring.BackendNotSupportedException: No available keyring backend found is also risen. See
https://github.com/javakeyring/java-keyring/actions/runs/6026456707/job/16349436333

Here's how it should look (note, different project!):

Do you know which version of the keyring library is used there?

@koppor
Copy link
Member Author

koppor commented Sep 5, 2023

After the commit javakeyring/java-keyring@b81ba14, the CI tests of the keyring library broke.

Thus, the update from secret-service 1.0.0 to 1.8.1-jdk17 seems to broke something.

@koppor
Copy link
Member Author

koppor commented Sep 5, 2023

Intermediate result: Version 1.8.0-jdk17 broke things:

image

@koppor
Copy link
Member Author

koppor commented Sep 5, 2023

CHANGELOG for 1.8.x: https://github.com/swiesend/secret-service/blob/master/CHANGELOG.md#181-jdk17---2023-01-17

There is also a PR in progress trying to fix things: javakeyring/java-keyring#91

@credmond
Copy link
Contributor

credmond commented Sep 5, 2023

That failing CI test may or may not be related, I actually don't think it is. Revert back to 1.0.3, for example, it still won't work.

The screenshots were from 1.0.3 (on my own project, and the Jabref branch was still on 1.0.3 when I ran it). I had this exact same issue a few weeks ago and I just can't remember EXACTLY how I fixed it. It was library related, I do know that.

@credmond
Copy link
Contributor

credmond commented Sep 5, 2023

That "No available keyring backend found" error is not very helpful as any root exception gets caught & ignored...

Ok, I rolled back (my own project) to re-create the problem I had, and it's very well hidden, and not logged. It's a root cause is this:

image

Same place, slightly different error to Jabref. And this only happens when a library Lombok is present as a runtime library. Lombok is not meant to be on the runtime classpath (it's a preprocessor, I'm sure you know it).

And the error occurs, because Lombok has this shaded version of the asm library:

image

I fixed this because by making Lombok's scope provided, as it should be.

1.0.3 and 1.0.4 of the keyring library work fine for me on Gnome on my own project. So something slightly different actually, is going on with Jabref. But I bet it's a library issue -- it has to be.

@credmond
Copy link
Contributor

credmond commented Sep 5, 2023

So, turns out we get different errors in 1.0.3, and 1.0.4. I think this is down to a module not being present in module-info.java.

For example, 1.0.4, you get this:

image

... which can be fixed by adding this to module-info.java:

requires org.freedesktop.dbus;

I have confirmed this works on Gnome.

....however 1.0.4 is not officially released yet and clearly has other problems.

@credmond
Copy link
Contributor

credmond commented Sep 5, 2023

Interestingly, if I use Jabref and 1.0.3, to overcome the jnr-ffi error I pasted much earlier, by doing this:

requires org.jnrproject.ffi;

and...

implementation 'com.github.jnr:jnr-ffi:2.2.13'

... I get back to a ClassNotFound error (exact same as the error I get with Lombok on my own project), looking for ClassVisitor. And this can be solved with an additional:

requires org.objectweb.asm;

...and so, 1.0.3 actually then works on Gnome!

So clearly Java modules is making life difficult when using non-modular projects such as java-keyring. I'd say there's some conflicts also in Jabref and some load order behaviour causing this.

I'd like to fully understand the load order / behaviour causing this. E.g., I do not need either of these entries in module-info.java on a personal project. Doesn't make sense.

But anyway, this comment and the previous are two ways to get the library 1.0.4, and 1.0.3, working on Gnome, at least. We probably need proper unit tests!

@koppor
Copy link
Member Author

koppor commented Sep 5, 2023

So, turns out we get different errors in 1.0.3, and 1.0.4. I think this is down to a module not being present in module-info.java.

For example, 1.0.4, you get this:

image

... which can be fixed by adding this to module-info.java:

requires org.freedesktop.dbus;

Done at #10274 (comment).

I have confirmed this works on Gnome.

Looking forward to your test of the build of PR 10274.

....however 1.0.4 is not officially released yet

I don't get that. In my view, a release on Maven central is a official release: https://central.sonatype.com/artifact/com.github.javakeyring/java-keyring/1.0.4. OK, the tag is missing at https://github.com/javakeyring/java-keyring/tags. I'll ask the maintainer.

and clearly has other problems.

It's not clear to me. From 1.0.3 to 1.0.4, there were "only" dependencies updated (see the diff javakeyring/java-keyring@java-keyring-1.0.3...5ccf275).

@koppor
Copy link
Member Author

koppor commented Sep 5, 2023

We probably need proper unit tests!

I think, a good start could be to have CI-based unit tests for secret-service. I tried at swiesend/secret-service#43, but I failed.

@credmond
Copy link
Contributor

credmond commented Sep 5, 2023

Ah ok, I didn't know it was officially released, I felt that the failing CI tests (and lack of tag) were an indicator it wasn't released yet. I thought maybe it was coming from JitPack or something...though, in hindsight, it wouldn't have been called "1.0.4", so that was a dumb idea!

Confirmed that PR works! Well done.

@github-project-automation github-project-automation bot moved this from Normal priority to Done in Features & Enhancements Sep 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants