#1982: Modified fallback link creation on windows#2062
Conversation
* Modified method signature of link in FileAccess.java to return PathLinkType instead of void * Modified implementation of link method in FileAccessImpl.java to return created PathLinkType. Also rewrote fallback on Windows machines to use correct fallback according to type (directory or file). * Modified LnCommandlet.java to check for created PathLinkType in case of set relative flag and log information due to lacking permissions.
oanding-blrng
added
windows
Coverage Report for CI Build 29091073904Coverage decreased (-0.06%) to 72.17%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions163 previously-covered lines in 4 files lost coverage.
Coverage Stats
💛 - Coveralls |
oanding-blrng
changed the title
oanding-blrng
changed the title
…s to use junctions. * Modified mklinkOnWindows() to set option for mklink to /j if dealing with hard links, because option /h doesn't work because of missing permissions. * Moved adding args and running process for mklink into try block to avoid bubbling up exception in runMklink().
…ption-link-not-honered-fix # Conflicts: # CHANGELOG.adoc
|
Good job, you implemented a Windows fallback logic and fixed the issue 1982. I tested the commands with permissions and also ran The Implementation also looks good to me. |
# Conflicts: # CHANGELOG.adoc
hohwille
left a comment
hohwille
left a comment
There was a problem hiding this comment.
@oanding-blrng thanks for your PR. You implemented the story well and perfectly tested and documented various combinations and scenarios. Awesome job 👍
There is one small misunderstanding regarding the log message in the LnCommandlet and the existing log message for the fallback. Also I added some other small review comments for improvement. When resolved ready for merge.
| "Due to lack of permissions, Microsoft's mklink with junction had to be used to create a Symlink. See\n" | ||
| + "https://github.com/devonfw/IDEasy/blob/main/documentation/symlink.adoc for further details. Error was: " | ||
| + e.getMessage()); | ||
|
|
There was a problem hiding this comment.
This message should still stay here.
We might want to adopt it or log different things depending if we have a directory or not.
| LOG.info( | ||
| "Due to lack of permissions, Microsoft's mklink with junction had to be used to create a Symlink. See\n" | ||
| + "https://github.com/devonfw/IDEasy/blob/main/documentation/symlink.adoc for further details."); |
There was a problem hiding this comment.
| LOG.info( | |
| "Due to lack of permissions, Microsoft's mklink with junction had to be used to create a Symlink. See\n" | |
| + "https://github.com/devonfw/IDEasy/blob/main/documentation/symlink.adoc for further details."); | |
| LOG.warn("Windows junction had to be used as fallback that only supports absolute paths. Therefore we cannot create a relative link as requested."); |
| option = directoryTarget ? "/j" : "/d"; | ||
| } else { | ||
| // Manual override of option in case of HARD_LINK, because of missing permissions for `mklink /h ...` | ||
| option = "/j"; |
There was a problem hiding this comment.
C:\>mklink /?
Creates a symbolic link.
MKLINK [[/D] | [/H] | [/J]] Link Target
/D Creates a directory symbolic link. Default is a file
symbolic link.
/H Creates a hard link instead of a symbolic link.
/J Creates a Directory Junction.
Link Specifies the new symbolic link name.
Target Specifies the path (relative or absolute) that the new link
refers to.
If /h (aka /H) was not supported at all, it would not make to keep it in PathLinkType.HARD_LINK.
As @shodiBoy1 recently found out, we already use a hardlink as fallback if mklink did not work.
However this does not even have to happen via mklink at all but can directly happen via Files.createLink.
Maybe it does not even make sense any more to pass PathLinkType to mklinkOnWindows since we only ever want to create a junction here. Then we could also remove PathLinkType.mklinkOption with its getter...
In the end mklink /h ... is the same as Files.createLink and mklink /d ... is more or less the same as Files.createSymbolicLink for a directory but that will fail again with the same permission error. So the only relevant option left for us is always /j.
| throw new IllegalStateException("" + type); | ||
| } | ||
| } catch (FileSystemException e) { | ||
| } catch (FileSystemException | RuntimeException e) { |
There was a problem hiding this comment.
IMHO we should not catch RuntimeException.
If for whatever reason we e.g. provide a type that is neither SYMBOLIC_LINK nor HARD_LINK (so maybe null or we added a 3rd type but forgot to handle it here) then we throw an IllegalStateException.
Catching that (or a NullPointerException) and assuming we have a permission issue and therefore should retry the link creation with a fallback strategy does not make any sense to me.
| } catch (FileSystemException | RuntimeException e) { | |
| } catch (FileSystemException e) { |
# Conflicts: # CHANGELOG.adoc
…info to LOG.debug to improve log information.
6 participants
This PR fixes #1982
Implemented changes according to comments and hints in ticket #1982.
Behavior of
ide lnon Windows without permissions:mklink /j <link> <target>with log information, that mklink junction had to used.mklink /j <link> <target>with log information, that mklink junction had to used.mklink /j <link> <target>with log information about lack of permissions, use of mklink with junction and link to documenation.mklink /j <link> <target>with log information, that mklink junction had to be used.mklink /j <link> <target>with log information about lack of permissions, use of mklink with junction and link to documenation.mklink /j <link> <target>with log information, that mklink junction had to be used + mention of wiki.mklink /d <link> <target> as fallbackreturns withexit code 1due to lack of permissions.Creates hardlink via Java as second fallback.
mklink /d <link> <target> as fallbackreturns withexit code 1due to lack of permissions.Creates hardlink via Java as second fallback.
Behavior of
ide lnon Windows with permissions:mklink /j <link> <target>with log information, that mklink junction had to be used.mklink /j <link> <target>with log information, that mklink junction had to be used + mention of wiki.Implemented changes:
Notes:
mklink /h <link> <target>because I would got "Zugriff verweigert". Creation of hardlinks on files would be possible, but this fallback is not needed, because Java is always able to create hardlinks. This is also the reason, whymklink /jis used as fallback.Testing instructions
Please add conscise, understandable instructions on how a reviewer can test/verify the functionality of your contribution here:
User has not permissions:
C:\Users\user\projects\IDEasy\test.txtC:\Users\user\projects\IDEasyUser has persmissions:
Checklist for this PR
Make sure everything is checked before merging this PR. For further info please also see
our DoD.
mvn clean testlocally all tests pass and build is successful#«issue-id»: «brief summary»(e.g.#921: fixed setup.bat). If no issue ID exists, title only.In Progressand assigned to you or there is no issue (might happen for very small PRs)with
internal