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

Removed check for long download paths over 260 characters on windows,… #17

Closed
wants to merge 1 commit into from

Conversation

Tastaturtaste
Copy link

… since Qt uses the long path api options in windows and thus handles them fine. (Addressing #15)
The changes were tested on Windows 10.0.19041.

@t-schroeder
Copy link
Collaborator

The problem was never that Sync-my-L2P couldn't write files with long paths to disk. It was that other programs couldn't open those files. (see e.g. RobertKrajewski#138) However, I tested this with a few different programs and all but one could open files with long paths (even though long path support is not enabled in my registry). But then there are also users that still run older Windows versions like Windows 7. We'd need to do some further testing.

Is there a concrete reason why you want to remove the path shortening other than because it works in your use case?

@Tastaturtaste
Copy link
Author

Tastaturtaste commented Aug 6, 2021

Is there a concrete reason why you want to remove the path shortening other than because it works in your use case?

I don't quite understand what you mean with "path shortening" that apparently should happen when downloading files to locations where the path exceeds 260 characters. In my case I get the following message (in german, emphasis mine):

Pfad zu lang!
Pfad zur Datei enthält mehr als 260 Zeichen und kann daher nicht erstellt werden. Bitte ändere den Downloadverzeichnis auf einen Pfad mit weniger Zeichen! Datei wird übersprungen.

My problem with this approach is that files get skipped if the path gets to long. This prevents everyone from opening them, since they are not synced. Downloading them would enable everyone on modern windows machines to use these files and even people on older windows versions would at least have the files backed up for the future.
It should at least be possible to enable this either manually.

In generaI don't think it is the responsibility of sync-my-l2p or any other syncing or downloading program to make sure that other programs can handle long paths to the files. But not downloading the files prevents other programs from even trying or the user from fixing the problem themselves. Therefor I think a warning message like the one I cited above on old windows versions would be good as long as the files get downloaded anyways. For many people that would just work since many programs indeed do work while opening files with long paths. Some people maybe cannot open the files with their favorite programs, but they would at least have them available in the future, know what the problem is and could attempt to work around them.

@t-schroeder
Copy link
Collaborator

I misunderstood what the problem was. Sync-my-L2P cuts off folder names after 75 characters to not run into the 260 character path length limit as often.

On the current version of Windows 10 the 260 character path length limit doesn't seem to apply even without the LongPathsEnabled registry flag. So it seems for recent Windows versions we could just remove the check entirely. But there are still users on older Windows versions. I haven't tested this yet on Windows 7, for example, so have no idea how it would behave there. Removing the check entirely as suggested here could result in an error after which no further files would get downloaded. Unfortunately I can't test this on older Windows versions at the moment (not enough disk space for VMs). Are you able to test this change on Windows 7 with/without the LongPathsEnabled flag?

… since Qt uses the long path api options in windows and thus handles them fine.
@Tastaturtaste
Copy link
Author

Just to make sure we are on the same page about the issue now:
As far as I can tell when using QT there should be absolutely no issue with opening or creating files with more than 260 characters on any windows version, since QT internally transforms all paths into using the universal naming convention (UNC) with the "\\?\" prefix as per Maximum Path Length Limitation:

The Windows API has many functions that also have Unicode versions to permit an extended-length path for a maximum total path length of 32,767 characters. [...] To specify an extended-length path, use the "\\?\" prefix. For example, "\\?\D:\very long path".

Since the unicode api is supported since Windows XP as far as I can tell from the requirements of various unicode win32 functions, this should work on Win 7 without a problem. The forum entry linked above refering to the QT implementation is specifically about Win 7.
There should be no problem with long paths while downloading files on any windows version at least since and including Windows XP.

The check I removed in my pull request was added as an answer to issue RobertKrajewski#138, which criticizes that files written with a path longer than 260 characters cannot be opened by other programs.
But to make sure that other programs can open the files Sync-my-L2P downloads it would be necessary to check every other program used to do that which I don't think is reasonable.

To the topic of testing my changes on Win 7:
I am very confident that there are no problems on Windows 7 as per the section I wrote above. Nonetheless I would like to test that and have already set up a VM running Win 7 SP1. Sadly at the moment I cannot log in to moodle with my own compiled fork, since I get a

ERROR 16:23:04.173 Could not check version:
TLS initialization failed

in the log when running the program and I don't know the reason and how to fix this. This was not the case when I posted the original pull request. I don't know if I did something different while compiling now or if something on the server side changed. If you are able to give me a hint I would be happy to try again.

@t-schroeder
Copy link
Collaborator

t-schroeder commented Sep 10, 2021

That error comes when the program doesn't find the OpenSSL library. I put precompiled versions for Windows into the subfolder windows/lib. Copy them to the same folder as your exe and it should work.

I was trying to reproduce the 260 character issue. I could only produce an error when saving a file whose name I artificially made longer than 260 characters. In that case you'd get an error saving the file and the sync would not download any further files. But that's an abstract case. There will probably never be a file that's itself over 260 characters. But that is the reason I just want to test this on older Windows version. Because if for some reason saving one file fails, you can't download any other files that come after. But you are probably right that it shouldn't be a problem. Thanks for taking the time to test this.

Edit:
You should also install the Visual C++ Redistributable. It's not checked in. It gets added when building the installer. When you install Sync-my-L2P it's extracted as vc_redist.{x86,x64}.exe in the installation path.

@Tastaturtaste
Copy link
Author

That error comes when the program doesn't find the OpenSSL library. I put precompiled versions for Windows into the subfolder windows/lib. Copy them to the same folder as your exe and it should work.

Thank you, copying the given libraries resolved my issue and I can now run my own compiled branch and log in.

I was trying to reproduce the 260 character issue. I could only produce an error when saving a file whose name I artificially made longer than 260 characters. In that case you'd get an error saving the file and the sync would not download any further files. [...]

Yes, there is still a limitation with every component of a path, so everything separated by "\" can be no longer than a value that is specific to the filesystem which can be queried from the windows api:

This type of path is composed of components separated by backslashes, each up to the value returned in the lpMaximumComponentLength parameter of the GetVolumeInformation function (this value is commonly 255 characters).

While it seems pretty unlikely that one file or one folder name is longer than this value, it would be possible to check for this instead for the overall path length.

Regarding the test:
I installed the current version straight from the github release on my VM and at the end of the installation a window for the redistributable installer opens and seems to install the redistributable. But even then apparently the MSVCP140.dll is missing which prevents me from running the program. Inspecting the installation directory there is a vc_redist.x64.exe which I can run. Doing so gives me the option to "repair" the redistributable installation, which resolves the issue allowing me to run the program. I don't know if that is a problem with my VM setup or the installer on Win 7. I got no warning during the installation. I guess this is what you meant with your edit, but I think it is a little strange that one has to manually install/"repair" the redistributable.

Trying to log in I get a message that moodle is not reachable. In the log I get entries stating:

L2P nicht erreichbar. Genauer Fehler: SSL handshake failed

Running my own compiled version gives the same error.
Since this is a complete fresh install of Sync-my-L2P with the distributed installer, I think the mistake may lie with my VM, but I am no expert with VMs and don't really now what to look out for. If you have another bit of advice for me that would be very nice.

@t-schroeder
Copy link
Collaborator

I installed the current version straight from the github release on my VM and at the end of the installation a window for the redistributable installer opens and seems to install the redistributable. But even then apparently the MSVCP140.dll is missing which prevents me from running the program. Inspecting the installation directory there is a vc_redist.x64.exe which I can run. Doing so gives me the option to "repair" the redistributable installation, which resolves the issue allowing me to run the program. I don't know if that is a problem with my VM setup or the installer on Win 7. I got no warning during the installation. I guess this is what you meant with your edit, but I think it is a little strange that one has to manually install/"repair" the redistributable.

Yeah, I don't know what problem the Redistributable installer has. The Sync-my-L2P installer calls it with the parameters "/install /passive /norestart". That should install it only if it's not already installed. Did you restart your machine in between? Maybe that's necessary.

Anyway, I moved another large VM to an external harddrive to get some space for a Windows 7 VM. Got the program to run there. I implemented a solution on the longpaths branch. Only on Windows versions older than Windows 10 1607 does it now check for paths over 260 characters. On older versions you get a dialog box when there's a file with a long path. There's also a new setting under Options -> Others to control this with.

I was trying to understand how the LongPathsSupported registry key plays into this but on my Win10 (build 19042) machine it doesn't make a difference. And neither does it on the win7 vm. So I just ignored registry keys in the implementation entirely.

If you want to test it you can build from the longpaths branch. Not sure about the "SSL handshake failed" error you're getting. Maybe comment out the L2P availability check and see if the rest works?

@t-schroeder
Copy link
Collaborator

I've created a new release. Can you please verify that the files are no longer being skipped with the new version?

@Tastaturtaste
Copy link
Author

I tested the new version 2.4.5 and it works as expected. Thank you for your effort and time!

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.

2 participants