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

Archiver follows symlinks on Windows 7 #47

Closed
petr-ujezdsky opened this issue Jun 28, 2016 · 27 comments
Closed

Archiver follows symlinks on Windows 7 #47

petr-ujezdsky opened this issue Jun 28, 2016 · 27 comments
Assignees
Labels
Milestone

Comments

@petr-ujezdsky
Copy link

We have many symlinks pointing to huge directories within our project. Because this archiver follows symlinks on Windows machines, the build stalls when eg. copying standard resources.

I have found the bug at AbstractArchiver.java:

    private boolean isSymlinkSupported()
    {
        return Os.isFamily( Os.FAMILY_UNIX ) && Java7Reflector.isAtLeastJava7();
    }

This should be changed to something like

return (Os.isFamily( Os.FAMILY_UNIX ) || Os.isFamily( Os.FAMILY_WINDOWS )) && Java7Reflector.isAtLeastJava7();

And the next strange thing is the usage of this method:

collection.setFollowingSymLinks( !isSymlinkSupported() );

When the OS does not support symlinks then the collection should follow them?

petr-ujezdsky added a commit to petr-ujezdsky/plexus-archiver that referenced this issue Jun 28, 2016
…ymlinks are not followed on Windows 7+ systems anymore
@krosenvold
Copy link
Member

Unfortunately it is not that simple. Traditionally java has not had the capability to create, modify or delete windows symlinks. I though this was still the case, at least it is not the case for Os.FAMILY_WINDOWS in general, maybe a certain subset of these. If you can make unit tests that demonstrate symlink capabiltity on your given Windows version we can open for that (or find other technical docs describing when/what the limitations of the windows symlink implementation are).

The default behaviour for years on end has been to follow symlinks like normal directories, since conventional wisdom has been that symlinks act somewhat like directories, so the code you are looking at is not odd from that perspective.

@hellpf
Copy link

hellpf commented Jun 28, 2016

Hi, from my point of view, the behaviour of working with symlinks should be the same across all OS (osx/linux/windows). If you are not sure, just add option to the pom.xml.

@krosenvold
Copy link
Member

krosenvold commented Jun 28, 2016

There's always room for surprises ! Please clone plexus-io and run on your local os (mvn clean install). I just added some "capability" tests that may reveal the level of windows support. I'll see if I can find some windows boxes of my own

@plamentotev
Copy link
Member

Hi,
Both Java 7 and Java 8 could create symlinks on Windows 7 and Windows 10. But keep in mind that by default you need admin privileges to create symlinks on Windows.

@plamentotev
Copy link
Member

I could confirm that on Java 8 + Windows 10 all tests are passing.

@krosenvold
Copy link
Member

This seems like a pretty cool thing to fix. Historically our symlink support has been weak/nonexistant and some of our current approaches are not good enough. I'm preparing a small post fot the maven developers list where we can discuss how the different issues should be handled. Please join the discussion there.

@petr-ujezdsky
Copy link
Author

The default behaviour for years on end has been to follow symlinks like normal directories [...]

This is actually not true when you look at the code

collection.setFollowingSymLinks( !isSymlinkSupported() );

When you have machine that supports symlinks (eg. linux for your current implementation), the symlinks are NOT followed.

I do not know if this is intentional

Historically our symlink support has been weak/nonexistant

but from what you said this could be bug as well.

@krosenvold
Copy link
Member

When you have machine that supports symlinks (eg. linux for your current implementation), the symlinks are NOT followed.

Yes :) That's because historic behaviour was changed about a year ago. On platforms that support symlinks we NEVER follow them, because we are able to represent them correctly on the roundtrip.

So we never "follow" symlnks (resolve them) when symlinks are supported, simply because we handle the link properly. I am still uncertain if there is any decent use case for resolving/following symlinks on platforms that support it. I'm still also working on the writeup I promised yesterday :)

@petr-ujezdsky
Copy link
Author

Hi, any news?

@michael-o
Copy link
Member

michael-o commented May 26, 2017

Locally applied:

diff --git a/src/main/java/org/codehaus/plexus/archiver/AbstractArchiver.java b/src/main/java/org/codehaus/plexus/archiver/AbstractArchiver.java
index dbcf07e..709701a 100755
--- a/src/main/java/org/codehaus/plexus/archiver/AbstractArchiver.java
+++ b/src/main/java/org/codehaus/plexus/archiver/AbstractArchiver.java
@@ -335,7 +335,7 @@ public void addFileSet( @Nonnull final FileSet fileSet )
         // The PlexusIoFileResourceCollection contains platform-specific File.separatorChar which
         // is an interesting cause of grief, see PLXCOMP-192
         final PlexusIoFileResourceCollection collection = new PlexusIoFileResourceCollection();
-        collection.setFollowingSymLinks( !isSymlinkSupported() );
+        collection.setFollowingSymLinks( isSymlinkSupported() );

         collection.setIncludes( fileSet.getIncludes() );
         collection.setExcludes( fileSet.getExcludes() );

and I get this

[ERROR] testSymlinkDirArchiver(org.codehaus.plexus.archiver.SymlinkTest)  Time elapsed: 0.476 s  <<< ERROR!
org.codehaus.plexus.archiver.ArchiverException: Unable to create directory or parent directory of /usr/home/mosipov/Projekte/plexus-archiver/target/output/dirarchiver-symlink/symLinkToDirOnTheOutside/FileInDirOnTheOutside.txt
        at org.codehaus.plexus.archiver.SymlinkTest.testSymlinkDirArchiver(SymlinkTest.java:103)
[INFO]
[INFO] Results:
[INFO]
[ERROR] Errors:
[ERROR]   SymlinkTest.testSymlinkDirArchiver:103 » Archiver Unable to create directory o...
[INFO]
[ERROR] Tests run: 143, Failures: 0, Errors: 1, Skipped: 1

The test looks broken to me because the target directory and file do not exist:

target/output/
├── dirarchiver-symlink
│   ├── aDirWithALink
│   │   └── backOutsideToFileX -> ../fileX.txt
│   ├── aRegularDir
│   │   └── aRegularFile.txt
│   ├── fileR.txt
│   ├── fileW.txt
│   ├── fileX.txt
│   ├── symDir -> targetDir
│   ├── symLinkToDirOnTheOutside -> ../dirOnTheOutside
│   └── targetDir
│       └── targetFile.txt
├── symlinks.tar
├── symlinks.zip
├── untaredSymlinks
│   ├── aDirWithALink
│   │   └── backOutsideToFileX -> ../fileX.txt
│   ├── aRegularDir
│   │   └── aRegularFile.txt
│   ├── fileR.txt
│   ├── fileW.txt
│   ├── fileX.txt
│   ├── symDir -> targetDir
│   ├── symLinkToDirOnTheOutside -> ../dirOnTheOutside
│   ├── symLinkToFileOnTheOutside -> ../onTheOutside.txt
│   ├── symR -> fileR.txt
│   ├── symW -> fileW.txt
│   ├── symX -> fileX.txt
│   └── targetDir
│       └── targetFile.txt
└── unzippedSymlinks
    ├── aDirWithALink
    │   └── backOutsideToFileX -> ../fileX.txt
    ├── aRegularDir
    │   └── aRegularFile.txt
    ├── fileR.txt
    ├── fileW.txt
    ├── fileX.txt
    ├── symDir -> targetDir
    ├── symLinkToDirOnTheOutside -> ../dirOnTheOutside
    ├── symLinkToFileOnTheOutside -> ../onTheOutside.txt
    ├── symR -> fileR.txt
    ├── symW -> fileW.txt
    ├── symX -> fileX.txt
    └── targetDir
        └── targetFile.txt

@michael-o
Copy link
Member

michael-o commented May 26, 2017

I need to revert my previous statement. The offending piece of code collection.setFollowingSymLinks( !isSymlinkSupported() ); is correct.

Origin:

$ tree /usr/home/mosipov/Projekte/plexus-archiver/src/testesources/symlinks/src
/usr/home/mosipov/Projekte/plexus-archiver/src/test/resources/symlinks/src
├── aDirWithALink
│   └── backOutsideToFileX -> ../fileX.txt
├── aRegularDir
│   └── aRegularFile.txt
├── fileR.txt
├── fileW.txt
├── fileX.txt
├── symDir -> targetDir/
├── symLinkToDirOnTheOutside -> ../dirOnTheOutside/
├── symLinkToFileOnTheOutside -> ../onTheOutside.txt
├── symR -> fileR.txt
├── symW -> fileW.txt
├── symX -> fileX.txt
└── targetDir
    └── targetFile.txt

5 directories, 10 files

Target:

$ tree target/output/dirarchiver-symlink
target/output/dirarchiver-symlink
├── aDirWithALink
│   └── backOutsideToFileX -> ../fileX.txt
├── aRegularDir
│   └── aRegularFile.txt
├── fileR.txt
├── fileW.txt
├── fileX.txt
├── symDir -> targetDir
├── symLinkToDirOnTheOutside -> ../dirOnTheOutside
├── symLinkToFileOnTheOutside -> ../onTheOutside.txt
├── symR -> fileR.txt
├── symW -> fileW.txt
├── symX -> fileX.txt
└── targetDir
    └── targetFile.txt

4 directories, 11 files

If you have symlinks followed, it will resolve them as if there are no symlinks.

I am inclined to throw that away altogether and pass false by default.

@plamentotev
Copy link
Member

@michael-o I don't quite follow you. Why do you want to replace

collection.setFollowingSymLinks( !isSymlinkSupported() );

with

collection.setFollowingSymLinks( false );

@michael-o
Copy link
Member

Simple reason, people don't understand what this parameter is for. The purpose is for the PlexusIoCollection: it completely resolves links, thus omitted them from the archive. Since this is disabled on Unix-like and cannot be configured, I cannot see a reason keeping the boolean condition.

@petr-ujezdsky
Copy link
Author

Am I getting things right that with

collection.setFollowingSymLinks( false );

the archiver will simply copy symlinks as they are (treating the symlink as a 'file'), and with

collection.setFollowingSymLinks( true );

the archiver will traverse the symlink and copy all files within it (treating the symlink as a directory)?

What I want to ultimately achieve is to make plexus skip traversing into symlinks on my Windows 7+ system. As I have stated in the issue on top, we have symlinks (created at build) to huge directory trees (made from another symlinks, resolved tree could be like 100 000+ files) that we do not want to be processed by plexus archiver.

As of now the isSymlinkSupported() returns false for Windows so the case collection.setFollowingSymLinks( true ); is used, hence the problem with symlinks traversing.
Your hardcoded collection.setFollowingSymLinks( false ); is actually very similar to what I have originally proposed by making the isSymlinkSupported() return true for Windows.

The only big issue I can see is that symlinks on Windows 7+ can be created only under admin privileges as @plamentotev mentioned above. Even if you add the right for creating symlinks to the standard user he still can not create them (Microsoft, why u do this??).

@michael-o
Copy link
Member

Correct, collection.setFollowingSymLinks( false ); will solve your issue. I don't see a reason to add a Windows test because isUnix() || isWindows() is virtually a true literal.

@petr-ujezdsky
Copy link
Author

Awesome. But I may be careful with

because isUnix() || isWindows() is virtually a true literal.

because as @krosenvold mentioned, not every Windows version supports symlinks.
From what I have found in the last 5 minutes of searching (here and there) the mklink functionality in Windows has arrived with Vista version. The XP does not support them out of the box.
But this should not affect your solution as the symlink simply can not exist on those machines in the first place.

@michael-o
Copy link
Member

No one will likely use XP or Vista for serious development or deployment. I'd consider Windows 7 as minimum.

@michael-o
Copy link
Member

OK guys, if I receive no objections on the next couple of days, I will go on and resolve this issue by settings false by default.

@plamentotev
Copy link
Member

My main concern is about the file path separator. If you set setFollowingSymLinks to false it will create a symlink even on Windows but the path it points to will be something like

link -> src\fileR.txt

I've tried to create a simple zip file. If you extract it on Linux the symlink won't work. It points to src\fileR.txt and on Linux the correct path is src/fileR.txt.

My other concern is that a lot of tools does not support symlinks on Windows. For example some archivers will just extract the sylink as regular file. In my example link will be a regular file that contains only the text

 src\fileR.txt

Maybe we should discuss it on the Maven Dev list before making Plexus Archiver not following symlink by default on Windows. After all this would affect the Assembly plug-in and it's used to create distribution packages for various OSes including Windows. Imagine a project that creates a distribution package with symbolic links for Linux and uses the same files to create the Windows package. With the current implementation the Windows package will contains the duplicated files instead of symlinks(correct me if I'm wrong here). In many cases that could be a desired behavior because many (if not most) archivers on Windows will not process the package correctly otherwise. If we change the Plexus Archiver default behavior we may broke projects like this one. I'm not saying that such projects are common (or even that exist), I'm just saying that we should be careful.

As a side note, looks like the symlinks support on Windows is improving. Microsoft is introducing some improvements with the Windows 10 Creators Update. They will allow non-admin user to create symlinks.

@plamentotev
Copy link
Member

btw @krosenvold mentioned that he is preparing a post for the Maven developers list. But I don't know if he has done so. If there is such discussion on the list it would be useful if somebody post a link to it.

@plamentotev
Copy link
Member

plamentotev commented Jun 11, 2017

And also I want to remind that src\fileR.txt is perfectly valid file name on Linux and the link points to it. It's just that we expect(or at least I) to point to fileR.txt inside the src directory.

@michael-o
Copy link
Member

@plamentotev Please go ahead and raise it on the dev list.

@plamentotev
Copy link
Member

FYI: Here is the post on the Maven dev list.

@petr-ujezdsky
Copy link
Author

And what about making it configurable? With maven-resources-plugin cooperation I could imagine something like this in the pom.xml:

<resources>
	<resource>
		<directory>my/small/dir</directory>
		<filtering>true</filtering>
	</resource>
	<resource>
		<directory>my/huge/dir/with/many/symlinks</directory>
		<filtering>false</filtering>
		<followSymlinks>false</followSymlinks>
	</resource>
</resources>

For distribution packages you could simply leave current configuration or explicitly set <followSymlinks>true</followSymlinks>. The obvious caveat I can see are the <includes> and <excludes> properties that might be useless for <followSymlinks>false</followSymlinks> (the included / excluded file could be inside some linked directory).

@michael-o
Copy link
Member

That's a good proposal. @petr-ujezdsky Can you check what needs to be changed? I guess several components have to change..maybe even some model :-(

@petr-ujezdsky
Copy link
Author

I might find some time for investigation in like 3 weeks from now. I am currently at vacation :-)

@michael-o
Copy link
Member

michael-o commented Jun 23, 2017

@plamentotev No reaction to your thread. I guess no one objects this. If won't receive any objection, I will turn this off by the end of the week.

@michael-o michael-o self-assigned this Jun 24, 2017
@michael-o michael-o added the bug label Jun 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants