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

Feature/sync unix symlinks as links #6205

Draft
wants to merge 34 commits into
base: master
Choose a base branch
from

Conversation

taminob
Copy link

@taminob taminob commented Nov 7, 2023

Summary

This PR allows the synchronization of Unix symlinks on Unix-like systems.
The symlinks will be synchronized as links.
I try to provide an MVP implementation which, in the future, can be further extended to improve usability and add more features.

TODO

Features:

  • Allow upload of symlinks via single-file upload (PUT request)
  • Allow upload of symlinks via bulk upload (POST request)
  • Allow download of symlinks
  • Fix server querying which assumes that everything, that is not a directory, is a file
  • GUI option to enable/disable symlink synchronization
    • Hide option on Windows (if not compatible, see next point)
  • Check Windows compatibility (current implementation might work for Windows shortcut files (except probably setModTime and readlink) since Qt handles shortcut files (.lnk) as Windows symlinks)

Known bugs:

  • When enabling symlink synchronization with already existing symlinks, the info message "Files from the ignore list as well as symlinks are not synced." does not disappear until restart
  • Conflict with symlink will show size of 0 for broken symlinks or the size of the symlink target for valid symlinks
  • When downloading a regular file which overwrites a broken symlink, only the write permission for owner is set and the file is not readable
  • Conflict between two (broken?) symlinks different timestamps does result in message "Files from ignore list as well as symbolic links are not synced." for conflict file (although symlink synchronization is enabled)
  • No conflict/synchronization for two (broken?) symlinks with same length and modification time (does also happen for regular files, probably shouldn't be addressed in this PR)

Tasks:

  • Unit testing
  • Regenerate translation files
  • Rewrite git history

Related

Resolves #250 (and probably also #5509).

This change requires nextcloud/server#41321 for the nextcloud server.

Disclaimer

This PR is still in a WIP state, do not test this on a production Nextcloud server.
Also, everything in there is still subject to change and hacky solutions and code full with TODOs will appear in this PR.

@nextcloud-desktop-bot
Copy link

AppImage file: nextcloud-PR-6205-000987f9111afe85b85875b83d37ce6b99699017-x86_64.AppImage

To test this change/fix you can simply download above AppImage file and test it.

Please make sure to quit your existing Nextcloud app and backup your data.

Copy link
Collaborator

@claucambra claucambra left a comment

Choose a reason for hiding this comment

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

Thanks @taminob really appreciate the effort in bringing this forward on both the client and server ends

How do you plan to handle these symlinks on non-Unix systems (mainly Windows)? I think this is the big question as the policy in the client so far has been to prefer restricting filenames and filetypes to those that are universally compatible to avoid issues

@taminob
Copy link
Author

taminob commented Nov 7, 2023

@claucambra I planned to mainly ignore non-Unix systems for now and only ensure that it doesn't break the build and current behavior on those systems. I think that for now it's probably best to just not download symlinks from the server if on a Windows system.
There have been already lengthy discussions around that in the issue and I think it would be best to handle it as a separate feature to slowly make progress regarding symlinks and then continue improving it step by step.

Could be that Windows will be relatively easy due to NTFS symlinks (I have no experience with them) and we only have to provide custom "symlink" and "readlink" implementations.
Alternatively, Qt handles Windows "shortcuts" as the equivalent to symlinks as far as I can see in the documentation.

Also, I'll look into including an option to toggle this feature and (at least for an experimental phase) default it to off so that the current behavior will be kept as-is.

@taminob taminob force-pushed the feature/sync-unix-symlinks-as-links branch from 0e645eb to 4719069 Compare November 23, 2023 20:18
@taminob taminob force-pushed the feature/sync-unix-symlinks-as-links branch 2 times, most recently from b619ba5 to cfe27b5 Compare December 6, 2023 22:38
@taminob taminob force-pushed the feature/sync-unix-symlinks-as-links branch from 489725a to 132aa68 Compare December 8, 2023 00:55
Before, the common assumption was being made that if an item is not a
directory, it must be a file (or the other way around).

Signed-off-by: Tamino Bauknecht <[email protected]>
Use SymLinkUploadDevice for symlinks in bulk uploads and add the header
field X-File-Type to recognize symlinks on the server.

Signed-off-by: Tamino Bauknecht <[email protected]>
Signed-off-by: Tamino Bauknecht <[email protected]>
This is required to be able to calculate the checksum for symlinks
without code duplication.
The commit reverts some of the removal done in commit
dd178f0.

Signed-off-by: Tamino Bauknecht <[email protected]>
This commit reverts the effects of commit
"libsync: Remove auto-exclude for symlinks"
(5812d5c).

Signed-off-by: Tamino Bauknecht <[email protected]>
This reverts commit 73063eb.

Since the checksum calculation on symlinks is required in multiple
places, it is better to move this differentiation to the checksum
calculation class.

Signed-off-by: Tamino Bauknecht <[email protected]>
The issue was introduced in "libsync/common: Store actual type for items".
Additionally, the variable/parameter "directoriesToRemove" was renamed
to "remoteItemsToRemove" since it could now also be a file or symlink.

Signed-off-by: Tamino Bauknecht <[email protected]>
This commit will preserve the permissions set on the symlink if a new
file is downloaded in the same place and the symlink is overwritten.
That fixes a bug which caused the downloaded file to become unreadable
if the symlink was broken and no permissions could be retrieved.

Signed-off-by: Tamino Bauknecht <[email protected]>
…undoing it)

Instead of copying the permissions on the symlink, use the default
permissions in that case. Otherwise, the newly downloaded files might
become executable since symlinks often have with all permissions set.

Signed-off-by: Tamino Bauknecht <[email protected]>
@AkechiShiro
Copy link

Any way we can help move this PR forward so it lands?

@taminob
Copy link
Author

taminob commented Apr 18, 2024

Any way we can help move this PR forward so it lands?

Since this PR depends on nextcloud/server#41321 and the server maintainers are actively ignoring it, I don't think it makes sense to spend any time on this PR here.
I can't think of anything other than upvoting the other PR and pinging the reviewers there.

@blob42
Copy link

blob42 commented Apr 30, 2024

Hi,

Is someone aware of a fork of nextcloud with this feature merged ? If someone would do it and setup some Patreon/donation I would gladly pay for this feature and I am sure many others will

@f1d094
Copy link

f1d094 commented Apr 30, 2024

@taminob - It looks like the server-side portion was quietly approved/merged into 29.0.0 beta 2

@AkechiShiro
Copy link

Thanks for mentioning it @f1d094 I wasn't really sure given the silence from the maintainers side.

@taminob
Copy link
Author

taminob commented May 30, 2024

Hi @claucambra, on server side the proposal was rejected on the basis that the desktop app does not support an apps system to make the symlink support optional. However, I think that it should be possible to extend the desktop app such that the symlink support is in the code base, but can be toggled by the user similar to how it is done for the end-to-end encryption support (or also external storage). What do you think? Is this for you actually a major blocker or shouldn't it be possible to find a solution for that?

@taminob
Copy link
Author

taminob commented Jul 10, 2024

Hi @claucambra, on server side the proposal was rejected on the basis that the desktop app does not support an apps system to make the symlink support optional. However, I think that it should be possible to extend the desktop app such that the symlink support is in the code base, but can be toggled by the user similar to how it is done for the end-to-end encryption support (or also external storage). What do you think? Is this for you actually a major blocker or shouldn't it be possible to find a solution for that?

@claucambra ping

@f1d094
Copy link

f1d094 commented Aug 22, 2024

@claucambra Claudio, after a few months away I received a few pings from other threads and it was brought to my attention that the core-holdup is here. I would like to share with you a cross-post I made for the server team:

First...I should point out that the lack of proper symlink synchronization is already causing a lot of issues and should be considered a bug, not a feature. As currently implemented Nextcloud actively breaks multi-client Linux/Mac Nextcloud setups. Secondly...I'd like to stress that if implemented correctly, it should be invisible to Windows users and transparently supported on Linux. Allow me to make the case:

  1. On the Nextcloud server, there should exist a special filetype, that filetype is "POSIX-SymLink" or whatever.
  2. The special filetype is ignored by all Windows clients. Full stop.
  3. On Mac/Linux systems, it is sent client-side as a symlink. It doesn't matter where it points to, it could be a broken link, or it could point to /etc/shadow...that is not the concern of the client. A link is a link is a link. I can point my links to the moon and if there is no path there, what is the harm? I've just created a broken symlink. The client should never-ever resolve the link or care where it points. It is just a basic filetype.
  4. Deleting the file on the nextcloud server, deletes the symlinks on clients that sync the symlink, as it would any other ordinary file.

Why is this at all complicated? Can you provide a use-case where the above would break any given install or cause comlications? Please explain.

We, as a team of Mac and Linux users, are incredibly frustrated with Nextcloud because of this very broken aspect of the way Nextcloud handles POSIX filesystems. If you claim to support Linux as a client, symlinks must be properly supported. They are a critcial and integral part of computing-on-Linux.

@gschenck
Copy link

Symlinks under Windows are no problem at all with NTFS as the standard file system.

@f1d094
Copy link

f1d094 commented Aug 29, 2024

Symlinks under Windows are no problem at all with NTFS as the standard file system.

I have not used windows in a long time but it was my understanding that symlinks were not enabled by default. Nextcloud can neither anticipate nor require that users have them enabled in order for their client to function. Therefore, they should likely only be an option for Windows. Unless of course this has changed and they are enabled by default now on any version of the operating system currently supported.

@gschenck
Copy link

gschenck commented Sep 6, 2024

Symlinks under Windows are no problem at all with NTFS as the standard file system.

I have not used windows in a long time but it was my understanding that symlinks were not enabled by default.

https://learn.microsoft.com/en-us/previous-versions/windows/it-pro/windows-server-2008-R2-and-2008/cc753194(v=ws.10)

Symbolic links under windows have been available and used for a long time.

@Rello Rello modified the milestone: 3.16.0 Dec 20, 2024
@claucambra
Copy link
Collaborator

claucambra commented Dec 20, 2024

Hi @claucambra, on server side the proposal was rejected on the basis that the desktop app does not support an apps system to make the symlink support optional. However, I think that it should be possible to extend the desktop app such that the symlink support is in the code base, but can be toggled by the user similar to how it is done for the end-to-end encryption support (or also external storage). What do you think? Is this for you actually a major blocker or shouldn't it be possible to find a solution for that?

@claucambra ping

Hi @taminob apologies for the delayed response.

We have discussed this in the desktop team and for us the key holdup in having this incorporated is the lack of automated testing for the changes introduced. If we have some auto tests we are happy to include these changes. From my understanding symlinks can just be synced as innocuous text files on Windows

@taminob
Copy link
Author

taminob commented Dec 22, 2024

Hi @taminob apologies for the delayed response.

We have discussed this in the desktop team and for us the key holdup in having this incorporated is the lack of automated testing for the changes introduced. If we have some auto tests we are happy to include these changes. From my understanding symlinks can just be synced as innocuous text files on Windows

Hi @claucambra, thanks for your response and for discussing this in the team. That is great news. :)

The current state of the PR is far from ready and I'd definitely add tests for the proposed changes before we merge it.
At some point in development after I finished the core functionality, I just stopped finalizing the PR because I didn't feel like it would get merged at all anymore.

Unfortunately, I currently don't have any spare time to finalize this PR and do the necessary adaptions for the server.
As I've written in #250 (comment), I don't have too much time for the next ~10 months. Maybe I'll find some time in March to get started, but I'm not sure if I can wrap everything up then.

Once I have time (or anyone else wants to start earlier), the necessary steps will be:

  • transform current server-core implementation to a server app
  • write tests for this PR (after updating to latest master and resolving conflicts)
  • test the changes of this PR on Windows (and also verify that it still works on Linux)

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.

Syncing symbolic links as reference
8 participants