You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ant.apache.org by jaikiran <gi...@git.apache.org> on 2017/12/09 07:57:21 UTC

[GitHub] ant pull request #49: [master branch] - Fix BZ-58683

GitHub user jaikiran opened a pull request:

    https://github.com/apache/ant/pull/49

    [master branch] - Fix BZ-58683 

    The commit here fixes the issue reported at https://bz.apache.org/bugzilla/show_bug.cgi?id=58683.
    
    This commit along with fixing the issue reported in that bug, also has updated the `Symlink` task to now start relying on the Java 7 support of symlinking through the use of `java.nio.file.Files` APIs. This now allows the task to move away from spawning process(es) for dealing with symlinks and also allows this task to be functional on non-unix systems which support symbolic links.
    
    P.S: For 1.9.x branch, I can create a separate commit, without using Java 7 APIs to fix this bug, if we do want to do that there. Let me know.


You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/jaikiran/ant bz-58683-master

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/ant/pull/49.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #49
    
----
commit 95b38bb8800f7608d8ee94866a0c9c532b06a498
Author: Jaikiran Pai <ja...@gmail.com>
Date:   2017-12-09T07:32:56Z

    BZ-58683 Honour the overwrite=false for existing symlinks. Plus, use Java 7 java.nio.file.Files support for symbolinks, in symlink task

----


---

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org


[GitHub] ant issue #49: [master branch] - Fix BZ-58683

Posted by jaikiran <gi...@git.apache.org>.
Github user jaikiran commented on the issue:

    https://github.com/apache/ant/pull/49
  
    >> Overall I'm in favor of this change and if you want to spend the time on fixing the bugzilla issue in a Java5 friendly way for 1.9.x that would certainly be good - bit not something I'd see as an requirement.
    
    Given that this bug isn't a blocker or (relatively) that critical, I most likely won't backport this to 1.9.x,  unless of course there's a genuine demand to do that (and that the user cannot upgrade to 1.10.x).



---

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org


[GitHub] ant pull request #49: [master branch] - Fix BZ-58683

Posted by bodewig <gi...@git.apache.org>.
Github user bodewig commented on a diff in the pull request:

    https://github.com/apache/ant/pull/49#discussion_r155927463
  
    --- Diff: src/main/org/apache/tools/ant/taskdefs/optional/unix/Symlink.java ---
    @@ -207,26 +198,30 @@ public void recreate() throws BuildException {
             try {
                 if (fileSets.isEmpty()) {
                     handleError(
    -                    "File set identifying link file(s) required for action recreate");
    +                        "File set identifying link file(s) required for action recreate");
                     return;
                 }
    -            Properties links = loadLinks(fileSets);
    -
    -            for (String lnk : links.stringPropertyNames()) {
    -                String res = links.getProperty(lnk);
    -                // handle the case where lnk points to a directory (bug 25181)
    +            final Properties links = loadLinks(fileSets);
    +            for (final String lnk : links.stringPropertyNames()) {
    +                final String res = links.getProperty(lnk);
                     try {
    -                    File test = new File(lnk);
    -                    if (!SYMLINK_UTILS.isSymbolicLink(lnk)) {
    -                        doLink(res, lnk);
    -                    } else if (!test.getCanonicalPath().equals(
    -                        new File(res).getCanonicalPath())) {
    -                        SYMLINK_UTILS.deleteSymbolicLink(test, this);
    -                        doLink(res, lnk);
    -                    } // else lnk exists, do nothing
    -                } catch (IOException ioe) {
    -                    handleError("IO exception while creating link");
    +                    if (Files.isSymbolicLink(Paths.get(lnk)) &&
    +                            Paths.get(lnk).toFile().getCanonicalPath().equals(Paths.get(res).toFile().getCanonicalPath())) {
    +                        // it's already a symlink and the symlink target is the same
    +                        // as the target noted in the properties file. So there's no
    +                        // need to recreate it
    +                        continue;
    +                    }
    +                } catch (IOException e) {
    +                    if (failonerror) {
    +                        throw new BuildException("Failed to recreate symlink " + lnk + " to target " + res, e);
    +                    }
    +                    // log and continue
    +                    log("Failed to recreate symlink " + lnk + " to target " + res, Project.MSG_INFO);
    +                    continue;
                     }
    +                // create the link
    +                this.doLink(res, lnk);
    --- End diff --
    
    I think this should be inside of the `try` block.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org


[GitHub] ant pull request #49: [master branch] - Fix BZ-58683

Posted by jaikiran <gi...@git.apache.org>.
Github user jaikiran closed the pull request at:

    https://github.com/apache/ant/pull/49


---

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org


[GitHub] ant issue #49: [master branch] - Fix BZ-58683

Posted by bodewig <gi...@git.apache.org>.
Github user bodewig commented on the issue:

    https://github.com/apache/ant/pull/49
  
    Looks good to me, thanks!
    
    WRT a Java5 compatible backport, I'd leave this really up to you. I don't see a pressing need for backporting either.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org


[GitHub] ant pull request #49: [master branch] - Fix BZ-58683

Posted by bodewig <gi...@git.apache.org>.
Github user bodewig commented on a diff in the pull request:

    https://github.com/apache/ant/pull/49#discussion_r155927252
  
    --- Diff: WHATSNEW ---
    @@ -27,6 +27,11 @@ Fixed bugs:
        copy happened to be the same source file (symlinked back
        to itself).
     
    + * Bugzilla Report 58683 - Fixed the issue where symlink creation
    --- End diff --
    
    I overlooked this through your earlier changes to WHATSNEW. We use to add the bugzilla issue as last line of the added entry and I think it would be good to keep a common format.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org


[GitHub] ant pull request #49: [master branch] - Fix BZ-58683

Posted by jaikiran <gi...@git.apache.org>.
Github user jaikiran commented on a diff in the pull request:

    https://github.com/apache/ant/pull/49#discussion_r155941076
  
    --- Diff: WHATSNEW ---
    @@ -27,6 +27,11 @@ Fixed bugs:
        copy happened to be the same source file (symlinked back
        to itself).
     
    + * Bugzilla Report 58683 - Fixed the issue where symlink creation
    --- End diff --
    
    Fixed and updated the PR.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org


[GitHub] ant pull request #49: [master branch] - Fix BZ-58683

Posted by jaikiran <gi...@git.apache.org>.
Github user jaikiran commented on a diff in the pull request:

    https://github.com/apache/ant/pull/49#discussion_r155941095
  
    --- Diff: src/main/org/apache/tools/ant/taskdefs/optional/unix/Symlink.java ---
    @@ -207,26 +198,30 @@ public void recreate() throws BuildException {
             try {
                 if (fileSets.isEmpty()) {
                     handleError(
    -                    "File set identifying link file(s) required for action recreate");
    +                        "File set identifying link file(s) required for action recreate");
                     return;
                 }
    -            Properties links = loadLinks(fileSets);
    -
    -            for (String lnk : links.stringPropertyNames()) {
    -                String res = links.getProperty(lnk);
    -                // handle the case where lnk points to a directory (bug 25181)
    +            final Properties links = loadLinks(fileSets);
    +            for (final String lnk : links.stringPropertyNames()) {
    +                final String res = links.getProperty(lnk);
                     try {
    -                    File test = new File(lnk);
    -                    if (!SYMLINK_UTILS.isSymbolicLink(lnk)) {
    -                        doLink(res, lnk);
    -                    } else if (!test.getCanonicalPath().equals(
    -                        new File(res).getCanonicalPath())) {
    -                        SYMLINK_UTILS.deleteSymbolicLink(test, this);
    -                        doLink(res, lnk);
    -                    } // else lnk exists, do nothing
    -                } catch (IOException ioe) {
    -                    handleError("IO exception while creating link");
    +                    if (Files.isSymbolicLink(Paths.get(lnk)) &&
    +                            Paths.get(lnk).toFile().getCanonicalPath().equals(Paths.get(res).toFile().getCanonicalPath())) {
    +                        // it's already a symlink and the symlink target is the same
    +                        // as the target noted in the properties file. So there's no
    +                        // need to recreate it
    +                        continue;
    +                    }
    +                } catch (IOException e) {
    +                    if (failonerror) {
    +                        throw new BuildException("Failed to recreate symlink " + lnk + " to target " + res, e);
    +                    }
    +                    // log and continue
    +                    log("Failed to recreate symlink " + lnk + " to target " + res, Project.MSG_INFO);
    +                    continue;
                     }
    +                // create the link
    +                this.doLink(res, lnk);
    --- End diff --
    
    You are right - the error message was incorrect. I have fixed that and updated the PR.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org


[GitHub] ant pull request #49: [master branch] - Fix BZ-58683

Posted by bodewig <gi...@git.apache.org>.
Github user bodewig commented on a diff in the pull request:

    https://github.com/apache/ant/pull/49#discussion_r155941358
  
    --- Diff: src/main/org/apache/tools/ant/util/SymbolicLinkUtils.java ---
    @@ -30,6 +30,8 @@
      * a symbolic link based on the absent support for them in Java.
      *
      * @since Ant 1.8.0
    + * @deprecated Starting Ant 1.10.2, this utility is deprecated in favour of the symlink
    + *              support introduced in Java {@link java.nio.file.Files} APIs
    --- End diff --
    
    sounds good to me, thanks. I prefer small changes myself as well.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org


[GitHub] ant pull request #49: [master branch] - Fix BZ-58683

Posted by bodewig <gi...@git.apache.org>.
Github user bodewig commented on a diff in the pull request:

    https://github.com/apache/ant/pull/49#discussion_r155928363
  
    --- Diff: src/main/org/apache/tools/ant/util/SymbolicLinkUtils.java ---
    @@ -30,6 +30,8 @@
      * a symbolic link based on the absent support for them in Java.
      *
      * @since Ant 1.8.0
    + * @deprecated Starting Ant 1.10.2, this utility is deprecated in favour of the symlink
    + *              support introduced in Java {@link java.nio.file.Files} APIs
    --- End diff --
    
    the class is used all over the code base (well, at least in a few different places :-). Do you intend to change those other occurrences as well?


---

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org


[GitHub] ant pull request #49: [master branch] - Fix BZ-58683

Posted by bodewig <gi...@git.apache.org>.
Github user bodewig commented on a diff in the pull request:

    https://github.com/apache/ant/pull/49#discussion_r155928313
  
    --- Diff: src/main/org/apache/tools/ant/taskdefs/optional/unix/Symlink.java ---
    @@ -500,18 +502,12 @@ private void doLink(String res, String lnk) throws BuildException {
                 File dir = fs.getDir(getProject());
     
                 Stream.of(ds.getIncludedFiles(), ds.getIncludedDirectories())
    -                .flatMap(Stream::of).forEach(path -> {
    -                    try {
    -                        File f = new File(dir, path);
    -                        File pf = f.getParentFile();
    -                        String name = f.getName();
    -                        if (SYMLINK_UTILS.isSymbolicLink(pf, name)) {
    -                            result.add(new File(pf.getCanonicalFile(), name));
    -                        }
    -                    } catch (IOException e) {
    -                        handleError("IOException: " + path + " omitted");
    +                    .flatMap(Stream::of).forEach(path -> {
    +                    final File f = new File(dir, path);
    +                    if (Files.isSymbolicLink(f.toPath())) {
    +                        result.add(f);
    --- End diff --
    
    The previous code canonicalized the link's parent directory to deal with symlinks further up the tree, I think this is important for consumers of the returned set like `record` that so merges entries from several "directories" that actually are links to the same directory.
    
    To be honest I'm not sure about the real impact but I guess we better keep the existing behavior.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org


[GitHub] ant issue #49: [master branch] - Fix BZ-58683

Posted by bodewig <gi...@git.apache.org>.
Github user bodewig commented on the issue:

    https://github.com/apache/ant/pull/49
  
    Thanks @jaikiran, I've added a buch of inline comments.
    
    Overall I'm in favor of this change and if you want to spend the time on fixing the bugzilla issue in a Java5 friendly way for 1.9.x that would certainly be good - bit not something I'd see as an requirement.
    
    The `SymlinkTest` still ensures tests are skipped on platforms that are not Unix (see the `assume` in `setup`). I'm not sure if we can figure out whether Java supports symlinks on the platform easily but it would certainly be good to test things on non-Unix platforms if we claim the task should work.



---

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org


[GitHub] ant pull request #49: [master branch] - Fix BZ-58683

Posted by jaikiran <gi...@git.apache.org>.
Github user jaikiran commented on a diff in the pull request:

    https://github.com/apache/ant/pull/49#discussion_r155941101
  
    --- Diff: src/main/org/apache/tools/ant/taskdefs/optional/unix/Symlink.java ---
    @@ -448,49 +432,67 @@ private void handleError(String msg) {
         /**
          * Conduct the actual construction of a link.
          *
    -     * <p> The link is constructed by calling <code>Execute.runCommand</code>.
          *
    -     * @param res   The path of the resource we are linking to.
    -     * @param lnk       The name of the link we wish to make.
    +     * @param res The path of the resource we are linking to.
    +     * @param lnk The name of the link we wish to make.
          * @throws BuildException when things go wrong
          */
         private void doLink(String res, String lnk) throws BuildException {
    -        File linkfil = new File(lnk);
    -        String options = "-s";
    -        if (overwrite) {
    -            options += "f";
    -            if (linkfil.exists()) {
    -                try {
    -                    SYMLINK_UTILS.deleteSymbolicLink(linkfil, this);
    -                } catch (FileNotFoundException fnfe) {
    -                    log("Symlink disappeared before it was deleted: " + lnk);
    -                } catch (IOException ioe) {
    -                    log("Unable to overwrite preexisting link or file: " + lnk,
    -                        ioe, Project.MSG_INFO);
    +        final Path link = Paths.get(lnk);
    +        final Path target = Paths.get(res);
    +        final boolean alreadyExists = Files.exists(link);
    +        if (!alreadyExists) {
    +            // if the path (at which the link is expected to be created) isn't already present
    +            // then we just go ahead and attempt to symlink
    +            try {
    +                Files.createSymbolicLink(link, target);
    +            } catch (IOException e) {
    +                if (failonerror) {
    +                    throw new BuildException("Failed to create symlink " + lnk + " to target " + res, e);
                     }
    +                log("Unable to create symlink " + lnk + " to target " + res, e, Project.MSG_INFO);
                 }
    +            return;
    +        }
    +        // file already exists, see if we are allowed to overwrite
    +        if (!overwrite) {
    +            log("Skipping symlink creation, since file at " + lnk + " already exists and overwrite is set to false", Project.MSG_INFO);
    +            return;
    +        }
    +        // we have been asked to overwrite, so we now do the necessary steps
    +
    +        // initiate a deletion *only* if the path is a symlink, else we fail with error
    +        if (!Files.isSymbolicLink(link)) {
    +            throw new BuildException("Cannot overwrite, as symlink, at " + lnk + " since the path already exists and isn't a symlink");
    --- End diff --
    
    Yes, that was an oversight. Fixed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org


[GitHub] ant pull request #49: [master branch] - Fix BZ-58683

Posted by jaikiran <gi...@git.apache.org>.
Github user jaikiran commented on a diff in the pull request:

    https://github.com/apache/ant/pull/49#discussion_r155941213
  
    --- Diff: src/main/org/apache/tools/ant/util/SymbolicLinkUtils.java ---
    @@ -30,6 +30,8 @@
      * a symbolic link based on the absent support for them in Java.
      *
      * @since Ant 1.8.0
    + * @deprecated Starting Ant 1.10.2, this utility is deprecated in favour of the symlink
    + *              support introduced in Java {@link java.nio.file.Files} APIs
    --- End diff --
    
    >> Do you intend to change those other occurrences as well?
    
    I do intend to do necessary changes related to this, in subsequent commit(s), when I get a chance. I didn't want to create one large PR with too many unrelated changes. I added this deprecation note just so that any new code doesn't end up using this class, now that we are on Java 7. But I think, it's better to add this deprecation note when I do get to changing references to this class in some other commits. So I have undone this change and updated the PR.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org


[GitHub] ant pull request #49: [master branch] - Fix BZ-58683

Posted by bodewig <gi...@git.apache.org>.
Github user bodewig commented on a diff in the pull request:

    https://github.com/apache/ant/pull/49#discussion_r155928143
  
    --- Diff: src/main/org/apache/tools/ant/taskdefs/optional/unix/Symlink.java ---
    @@ -448,49 +432,67 @@ private void handleError(String msg) {
         /**
          * Conduct the actual construction of a link.
          *
    -     * <p> The link is constructed by calling <code>Execute.runCommand</code>.
          *
    -     * @param res   The path of the resource we are linking to.
    -     * @param lnk       The name of the link we wish to make.
    +     * @param res The path of the resource we are linking to.
    +     * @param lnk The name of the link we wish to make.
          * @throws BuildException when things go wrong
          */
         private void doLink(String res, String lnk) throws BuildException {
    -        File linkfil = new File(lnk);
    -        String options = "-s";
    -        if (overwrite) {
    -            options += "f";
    -            if (linkfil.exists()) {
    -                try {
    -                    SYMLINK_UTILS.deleteSymbolicLink(linkfil, this);
    -                } catch (FileNotFoundException fnfe) {
    -                    log("Symlink disappeared before it was deleted: " + lnk);
    -                } catch (IOException ioe) {
    -                    log("Unable to overwrite preexisting link or file: " + lnk,
    -                        ioe, Project.MSG_INFO);
    +        final Path link = Paths.get(lnk);
    +        final Path target = Paths.get(res);
    +        final boolean alreadyExists = Files.exists(link);
    +        if (!alreadyExists) {
    +            // if the path (at which the link is expected to be created) isn't already present
    +            // then we just go ahead and attempt to symlink
    +            try {
    +                Files.createSymbolicLink(link, target);
    +            } catch (IOException e) {
    +                if (failonerror) {
    +                    throw new BuildException("Failed to create symlink " + lnk + " to target " + res, e);
                     }
    +                log("Unable to create symlink " + lnk + " to target " + res, e, Project.MSG_INFO);
                 }
    +            return;
    +        }
    +        // file already exists, see if we are allowed to overwrite
    +        if (!overwrite) {
    +            log("Skipping symlink creation, since file at " + lnk + " already exists and overwrite is set to false", Project.MSG_INFO);
    +            return;
    +        }
    +        // we have been asked to overwrite, so we now do the necessary steps
    +
    +        // initiate a deletion *only* if the path is a symlink, else we fail with error
    +        if (!Files.isSymbolicLink(link)) {
    +            throw new BuildException("Cannot overwrite, as symlink, at " + lnk + " since the path already exists and isn't a symlink");
    --- End diff --
    
    should this take `failOnError` into account?


---

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org


[GitHub] ant pull request #49: [master branch] - Fix BZ-58683

Posted by jaikiran <gi...@git.apache.org>.
Github user jaikiran commented on a diff in the pull request:

    https://github.com/apache/ant/pull/49#discussion_r155941080
  
    --- Diff: manual/Tasks/symlink.html ---
    @@ -26,7 +26,7 @@
     
     <h2><a name="symlink">Symlink</a></h2>
     <h3>Description</h3>
    -<p> Manages symbolic links on Unix based platforms. Can be used to
    +<p> Manages symbolic links on platforms that support symbolic links. Can be used to
    --- End diff --
    
    Fixed and updated the PR


---

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org


[GitHub] ant pull request #49: [master branch] - Fix BZ-58683

Posted by bodewig <gi...@git.apache.org>.
Github user bodewig commented on a diff in the pull request:

    https://github.com/apache/ant/pull/49#discussion_r155927271
  
    --- Diff: manual/Tasks/symlink.html ---
    @@ -26,7 +26,7 @@
     
     <h2><a name="symlink">Symlink</a></h2>
     <h3>Description</h3>
    -<p> Manages symbolic links on Unix based platforms. Can be used to
    +<p> Manages symbolic links on platforms that support symbolic links. Can be used to
    --- End diff --
    
    rather "platform where Java supports symbolic links"? I'm sure people will yell at us as their platform supports some kind of symbolic link but Java doesn't support it.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org


[GitHub] ant pull request #49: [master branch] - Fix BZ-58683

Posted by bodewig <gi...@git.apache.org>.
Github user bodewig commented on a diff in the pull request:

    https://github.com/apache/ant/pull/49#discussion_r155928069
  
    --- Diff: src/main/org/apache/tools/ant/taskdefs/optional/unix/Symlink.java ---
    @@ -207,26 +198,30 @@ public void recreate() throws BuildException {
             try {
                 if (fileSets.isEmpty()) {
                     handleError(
    -                    "File set identifying link file(s) required for action recreate");
    +                        "File set identifying link file(s) required for action recreate");
                     return;
                 }
    -            Properties links = loadLinks(fileSets);
    -
    -            for (String lnk : links.stringPropertyNames()) {
    -                String res = links.getProperty(lnk);
    -                // handle the case where lnk points to a directory (bug 25181)
    +            final Properties links = loadLinks(fileSets);
    +            for (final String lnk : links.stringPropertyNames()) {
    +                final String res = links.getProperty(lnk);
                     try {
    -                    File test = new File(lnk);
    -                    if (!SYMLINK_UTILS.isSymbolicLink(lnk)) {
    -                        doLink(res, lnk);
    -                    } else if (!test.getCanonicalPath().equals(
    -                        new File(res).getCanonicalPath())) {
    -                        SYMLINK_UTILS.deleteSymbolicLink(test, this);
    -                        doLink(res, lnk);
    -                    } // else lnk exists, do nothing
    -                } catch (IOException ioe) {
    -                    handleError("IO exception while creating link");
    +                    if (Files.isSymbolicLink(Paths.get(lnk)) &&
    +                            Paths.get(lnk).toFile().getCanonicalPath().equals(Paths.get(res).toFile().getCanonicalPath())) {
    +                        // it's already a symlink and the symlink target is the same
    +                        // as the target noted in the properties file. So there's no
    +                        // need to recreate it
    +                        continue;
    +                    }
    +                } catch (IOException e) {
    +                    if (failonerror) {
    +                        throw new BuildException("Failed to recreate symlink " + lnk + " to target " + res, e);
    +                    }
    +                    // log and continue
    +                    log("Failed to recreate symlink " + lnk + " to target " + res, Project.MSG_INFO);
    +                    continue;
                     }
    +                // create the link
    +                this.doLink(res, lnk);
    --- End diff --
    
    Just now see `doLink` doesn't throw any `IOException`. In this case the error message is wrong as the error occured while trying to check whether it is a (self-)link.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org


[GitHub] ant pull request #49: [master branch] - Fix BZ-58683

Posted by jaikiran <gi...@git.apache.org>.
Github user jaikiran commented on a diff in the pull request:

    https://github.com/apache/ant/pull/49#discussion_r155941155
  
    --- Diff: src/main/org/apache/tools/ant/taskdefs/optional/unix/Symlink.java ---
    @@ -500,18 +502,12 @@ private void doLink(String res, String lnk) throws BuildException {
                 File dir = fs.getDir(getProject());
     
                 Stream.of(ds.getIncludedFiles(), ds.getIncludedDirectories())
    -                .flatMap(Stream::of).forEach(path -> {
    -                    try {
    -                        File f = new File(dir, path);
    -                        File pf = f.getParentFile();
    -                        String name = f.getName();
    -                        if (SYMLINK_UTILS.isSymbolicLink(pf, name)) {
    -                            result.add(new File(pf.getCanonicalFile(), name));
    -                        }
    -                    } catch (IOException e) {
    -                        handleError("IOException: " + path + " omitted");
    +                    .flatMap(Stream::of).forEach(path -> {
    +                    final File f = new File(dir, path);
    +                    if (Files.isSymbolicLink(f.toPath())) {
    +                        result.add(f);
    --- End diff --
    
    When I initially changed this part, while submitting the PR, I had thought about it a bit whether or not to stick with the previous behaviour. Given that it was noted as a "limitation" (in the javadoc), I had decided to change the behaviour. But thinking about it again, I do agree with you that it isn't worth changing the previous behaviour (without knowing the complete impact). So I have switched back to the previous semantic, in this part of the code and updated the PR


---

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org


[GitHub] ant issue #49: [master branch] - Fix BZ-58683

Posted by jaikiran <gi...@git.apache.org>.
Github user jaikiran commented on the issue:

    https://github.com/apache/ant/pull/49
  
    Committed to master branch https://github.com/apache/ant/commit/cefdbd398d8e310b218f9e2ca6f0b7fb13eddbb9



---

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org