[GitHub] ant pull request #49: [master branch] - Fix BZ-58683
Github user jaikiran closed the pull request at: https://github.com/apache/ant/pull/49 --- - To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org For additional commands, e-mail: dev-h...@ant.apache.org
[GitHub] ant pull request #49: [master branch] - Fix BZ-58683
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-unsubscr...@ant.apache.org
For additional commands, e-mail: dev-h...@ant.apache.org
[GitHub] ant pull request #49: [master branch] - Fix BZ-58683
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-unsubscr...@ant.apache.org
For additional commands, e-mail: dev-h...@ant.apache.org
[GitHub] ant pull request #49: [master branch] - Fix BZ-58683
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-unsubscr...@ant.apache.org
For additional commands, e-mail: dev-h...@ant.apache.org
[GitHub] ant pull request #49: [master branch] - Fix BZ-58683
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.
*
- * The link is constructed by calling
Execute.runCommand.
*
- * @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-unsubscr...@ant.apache.org
For additional commands, e-mail: dev-h...@ant.apache.org
[GitHub] ant pull request #49: [master branch] - Fix BZ-58683
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-unsubscr...@ant.apache.org
For additional commands, e-mail: dev-h...@ant.apache.org
[GitHub] ant pull request #49: [master branch] - Fix BZ-58683
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 @@ Symlink Description - Manages symbolic links on Unix based platforms. Can be used to + 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-unsubscr...@ant.apache.org For additional commands, e-mail: dev-h...@ant.apache.org
[GitHub] ant pull request #49: [master branch] - Fix BZ-58683
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-unsubscr...@ant.apache.org For additional commands, e-mail: dev-h...@ant.apache.org
[GitHub] ant pull request #49: [master branch] - Fix BZ-58683
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-unsubscr...@ant.apache.org
For additional commands, e-mail: dev-h...@ant.apache.org
[GitHub] ant pull request #49: [master branch] - Fix BZ-58683
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-unsubscr...@ant.apache.org
For additional commands, e-mail: dev-h...@ant.apache.org
[GitHub] ant pull request #49: [master branch] - Fix BZ-58683
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.
*
- * The link is constructed by calling
Execute.runCommand.
*
- * @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-unsubscr...@ant.apache.org
For additional commands, e-mail: dev-h...@ant.apache.org
[GitHub] ant pull request #49: [master branch] - Fix BZ-58683
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-unsubscr...@ant.apache.org
For additional commands, e-mail: dev-h...@ant.apache.org
[GitHub] ant pull request #49: [master branch] - Fix BZ-58683
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-unsubscr...@ant.apache.org
For additional commands, e-mail: dev-h...@ant.apache.org
[GitHub] ant pull request #49: [master branch] - Fix BZ-58683
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 @@ Symlink Description - Manages symbolic links on Unix based platforms. Can be used to + 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-unsubscr...@ant.apache.org For additional commands, e-mail: dev-h...@ant.apache.org
[GitHub] ant pull request #49: [master branch] - Fix BZ-58683
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-unsubscr...@ant.apache.org For additional commands, e-mail: dev-h...@ant.apache.org
[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 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-unsubscr...@ant.apache.org For additional commands, e-mail: dev-h...@ant.apache.org
