search

cryptomator / webdav-nio-adapter

Jackrabbit-based servlets running on embedded Jetty to serve a directory specified by a java.nio.file.Path
GNU Affero General Public License v3.0
12 stars 8 forks source link

Fixes https://github.com/cryptomator/cryptomator/issues/722 #21

Closed purejava closed 6 years ago

purejava commented 6 years ago

gvfs-mount has been replaced by gio mount in gvfs 1.37.2 gio mount provides the same functionality

CLAassistant commented 6 years ago

CLA assistant check
All committers have signed the CLA.

purejava commented 6 years ago

Maven tests pass.

As gvfs-mount was implemented as a wrapper around gio mount for quite a while the fix should work for versions of gvfs before 1.37.2 as well.

purejava commented 6 years ago

gvfs-open is deprecated too.

purejava commented 6 years ago

How are your naming conventions?

Should members, classes, constants etc. be renamed from gvfs to gio?

purejava commented 6 years ago

The MR should be complete now.

tobihagemann commented 6 years ago

Thank you for the PR!

I'm not absolutely convinced that gio mount is available on older platforms that Cryptomator 1.4.0 is going to support. Our rule of thumb is to support the ones that are supported by the latest JDK.

If you find references that gio mount is available on Ubuntu Linux 16.04 - LTS and 17.04, I think the PR is fine. I would suggest to rename LinuxGvfsMounter to LinuxGioMounter though (including other references to gvfs if there are any).

Otherwise, I would suggest to create a new LinuxGioMounter and leave the LinuxGvfsMounter as it was before. And then add this new MounterStrategy to the MounterModule.

purejava commented 6 years ago

I'm not absolutely convinced that gio mount is available on older platforms that Cryptomator 1.4.0 is going to support. Our rule of thumb is to support the ones that are supported by the latest JDK.

Thanks for pointing out the rule of thumb.

If you find references that gio mount is available on Ubuntu Linux 16.04 - LTS and 17.04, I think the PR is fine.

And you are absolutely right. Digging through the history of gvfs and glib shows that the gio cmd tool was available with upstream version 2.49.3 of GLib. The change founds its way into Ubuntu with artful (17.10). The binary /usr/bin/gio is available since then.

So at least for Ubuntu 16.04 LTS, which I was checking first, the gio command tool is not available and for this reason the gvfs -utils need to be used.

I would suggest to rename LinuxGvfsMounter to LinuxGioMounter though (including other references to gvfs if there are any).

Otherwise, I would suggest to create a new LinuxGioMounter and leave the LinuxGvfsMounter as it was before. And then add this new MounterStrategy to the MounterModule.

Thank you for these suggestions. I go ahead and change the PR and add a check for both tools and depending on which one is available, one of the installed is used.

purejava commented 6 years ago

I propose this PR. It works with gvfs versions 1.36.2-3 and 1.38.0+3+g7da3b0ba-1. Finally I decided to go with a lightweight renaming, mostly to avoid changing the DEFAULT_GVFS_SCHEME setting and keep backwards compatibility.

If something should be changed, please let me know :)

Edit: forgot to mention testing.

overheadhunter commented 6 years ago

Otherwise, I would suggest to create a new LinuxGioMounter and leave the LinuxGvfsMounter as it was before. And then add this new MounterStrategy to the MounterModule.

I second that. Both Gvfs and Gio can extend an abstract VfsMountingStrategy.

That way we have less if/else in the code and distinguish solely based on the polymorph isApplicable method. The preferred mounter is then chosen via the order in which they are added to the list of available mounters in MounterModule.

purejava commented 6 years ago

I second that. Both Gvfs and Gio can extend an abstract VfsMountingStrategy.

That way we have less if/else in the code and distinguish solely based on the polymorph isApplicable method. The preferred mounter is then chosen via the order in which they are added to the list of available mounters in MounterModule.

Here we go.

I tested the change with gvfs versions 1.36.2-3 and 1.38.0+3+g7da3b0ba-1 again. Further I simulated missing gio for Ubuntu 16.04 LTS. It's using the LinuxGvfsMounter in that case as expected.

purejava commented 6 years ago

Can you make MountImpl protected, since this class name is also used by other Mounters within the same package?

Thanks @overheadhunter for the review and the hint! I'll add a commit with that improvement.