[GitHub] ant issue #50: Use newer Maven Ant tasks

[twogee]

Github user twogee commented on the issue:

https://github.com/apache/ant/pull/50
  
I hope the third time is the charm 😄

A couple of wishes for antunit: it should have ant/ant-launcher as 
"provided" dependencies and it should have a "tempdir" attribute similar to 
junit task.


---

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

Re: Potential breaking change to symlink task [ was Re: Jenkins build became unstable: Ant-Build-Matrix-master-Linux » JDK 1.8 (latest) #977]

[Stefan Bodewig]

[@Steve I'm trying to drag you in as you've been the one who brought in
the change that gets contested by
https://bz.apache.org/bugzilla/show_bug.cgi?id=58683 and maybe you
recall the details better than we do.]

On 2017-12-10, Jaikiran Pai wrote:

> On 10/12/17 3:09 PM, Stefan Bodewig wrote:
>> On 2017-12-10, Jaikiran Pai wrote:

>>> I'll investigate why this is failing (local tests pass for me) and fix it.

>> Target testCreateOverFile in the antunit test explicitly tries to
>> replace a file with a link, doing exactly what the bugzilla report says
>> is a bug. So the behavior seems to have been intentional.

>> We now need to figure out whether the bug report was genuine (and list
>> the change as breaking) or we want to revert part of your fix, change
>> the documentation and change the bugzilla issue's resolution to a
>> WONTFIX.

> The symlink task documentation[1] states this for the "overwrite" attribute:

> 
> Overwrite existing links or not.
> 

> It does explicitly mention that it's meant for overwriting links. The
> test here, like you note, tries to overwrite a non-symlink file and
> IMO it should error out, like it does with this change. After all, the
> entire symlink task is meant to just deal with symlinks and
> overwriting non-symlinks doesn't seem right. Having said that, I do
> agree that this is a change in behaviour and we should list this as
> such, if we do decide to let this change stay.

> The reason why that existing test was introduced in first place and
> the feature to let the symlink task overwrite a non-symlink file goes
> back to this issue
> https://bz.apache.org/bugzilla/show_bug.cgi?id=43426. The rationale in
> that description seems to be that the Unix ln -sf apparently allows
> overwriting non-symlink file with a symlink. Given that context, I'm
> not really sure how we should proceed. Should we make this breaking
> change or should we let the prior behaviour continue?

> I'm in favour of this breaking change, but I won't mind switching back
> to the prior behaviour if you and others think that's a better thing
> to do.

Thank you for digging into the history, Jaikiran.

43426 is explicitly listed as a fixed bug (in 1.8.0) so I'm leaning
towards keeping and documenting the behavior of allowing  to
replace regular files.

Stefan

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

Potential breaking change to symlink task [ was Re: Jenkins build became unstable: Ant-Build-Matrix-master-Linux » JDK 1.8 (latest) #977]

[Jaikiran Pai]

On 10/12/17 3:09 PM, Stefan Bodewig wrote:

On 2017-12-10, Jaikiran Pai wrote:


I'll investigate why this is failing (local tests pass for me) and fix it.


Target testCreateOverFile in the antunit test explicitly tries to
replace a file with a link, doing exactly what the bugzilla report says
is a bug. So the behavior seems to have been intentional.

We now need to figure out whether the bug report was genuine (and list
the change as breaking) or we want to revert part of your fix, change
the documentation and change the bugzilla issue's resolution to a
WONTFIX.


The symlink task documentation[1] states this for the "overwrite" attribute:


Overwrite existing links or not.


It does explicitly mention that it's meant for overwriting links. The 
test here, like you note, tries to overwrite a non-symlink file and IMO 
it should error out, like it does with this change. After all, the 
entire symlink task is meant to just deal with symlinks and overwriting 
non-symlinks doesn't seem right. Having said that, I do agree that this 
is a change in behaviour and we should list this as such, if we do 
decide to let this change stay.



The reason why that existing test was introduced in first place and the 
feature to let the symlink task overwrite a non-symlink file goes back 
to this issue https://bz.apache.org/bugzilla/show_bug.cgi?id=43426. The 
rationale in that description seems to be that the Unix ln -sf 
apparently allows overwriting non-symlink file with a symlink. Given 
that context, I'm not really sure how we should proceed. Should we make 
this breaking change or should we let the prior behaviour continue?


I'm in favour of this breaking change, but I won't mind switching back 
to the prior behaviour if you and others think that's a better thing to do.



[1] https://ant.apache.org/manual/Tasks/symlink.html

-Jaikiran

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

Re: Jenkins build became unstable: Ant-Build-Matrix-master-Linux » JDK 1.8 (latest) #977

[Jaikiran Pai]

You are right, that indeed was the issue (for one of that failing test). 
I have pushed a commit with this change to fix that one.


-Jaikiran


On 10/12/17 3:13 PM, Stefan Bodewig wrote:

On 2017-12-10, Stefan Bodewig wrote:


testCreateDoubleHanging is related to
https://bz.apache.org/bugzilla/show_bug.cgi?id=38199 judging from the
history. Here the link points to a non-existent file and Files.exists
returns false in Java8.

fixed by

@@ -37,6 +37,7 @@ import java.io.IOException;
  import java.io.InputStream;
  import java.io.PrintStream;
  import java.nio.file.Files;
+import java.nio.file.LinkOption;
  import java.nio.file.Path;
  import java.nio.file.Paths;
  import java.util.ArrayList;
@@ -440,7 +441,7 @@ public class Symlink extends DispatchTask {
  private void doLink(String res, String lnk) throws BuildException {
  final Path link = Paths.get(lnk);
  final Path target = Paths.get(res);
-final boolean alreadyExists = Files.exists(link);
+final boolean alreadyExists = Files.exists(link, 
LinkOption.NOFOLLOW_LINKS);
  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


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




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

[GitHub] ant issue #50: Use newer Maven Ant tasks

[bodewig]

Github user bodewig commented on the issue:

https://github.com/apache/ant/pull/50
  
:+1: 


---

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

Re: Jenkins build became unstable: Ant-Build-Matrix-master-Linux » JDK 1.8 (latest) #977

[Stefan Bodewig]

On 2017-12-10, Stefan Bodewig wrote:

> testCreateDoubleHanging is related to
> https://bz.apache.org/bugzilla/show_bug.cgi?id=38199 judging from the
> history. Here the link points to a non-existent file and Files.exists
> returns false in Java8.

fixed by

@@ -37,6 +37,7 @@ import java.io.IOException;
 import java.io.InputStream;
 import java.io.PrintStream;
 import java.nio.file.Files;
+import java.nio.file.LinkOption;
 import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.util.ArrayList;
@@ -440,7 +441,7 @@ public class Symlink extends DispatchTask {
 private void doLink(String res, String lnk) throws BuildException {
 final Path link = Paths.get(lnk);
 final Path target = Paths.get(res);
-final boolean alreadyExists = Files.exists(link);
+final boolean alreadyExists = Files.exists(link, 
LinkOption.NOFOLLOW_LINKS);
 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


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

Re: Jenkins build became unstable: Ant-Build-Matrix-master-Linux » JDK 1.8 (latest) #977

[Stefan Bodewig]

On 2017-12-10, Jaikiran Pai wrote:

> I'll investigate why this is failing (local tests pass for me) and fix it.

Are you using Java9 locally? The test only fail for Java8 in Jenkins,
this could explain the difference. The antunit tests fail for me with
Java 8 as well, while the JUnit test passes.

testCreateDoubleHanging is related to
https://bz.apache.org/bugzilla/show_bug.cgi?id=38199 judging from the
history. Here the link points to a non-existent file and Files.exists
returns false in Java8.

Target testCreateOverFile in the antunit test explicitly tries to
replace a file with a link, doing exactly what the bugzilla report says
is a bug. So the behavior seems to have been intentional.

We now need to figure out whether the bug report was genuine (and list
the change as breaking) or we want to revert part of your fix, change
the documentation and change the bugzilla issue's resolution to a
WONTFIX.

Stefan

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

[GitHub] ant issue #50: Use newer Maven Ant tasks

[twogee]

Github user twogee commented on the issue:

https://github.com/apache/ant/pull/50
  
This is exactly why I was looking into it -- those things tend to 
deteriorate over time 😄

To sum it up, I'll revert stripping, restore CPL and correct the README 
(that tests rely on JUnit 3 being first on classpath). BTW, pom.xml should be 
updated with 4.12 as well, I suppose.


---

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

Re: Jenkins build became unstable: Ant-Build-Matrix-master-Linux » JDK 1.8 (latest) #977

[Jaikiran Pai]

I'll investigate why this is failing (local tests pass for me) and fix it.

-Jaikiran


On 10/12/17 2:29 PM, Apache Jenkins Server wrote:

See 





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

[GitHub] ant issue #50: Use newer Maven Ant tasks

[bodewig]

Github user bodewig commented on the issue:

https://github.com/apache/ant/pull/50
  
Thank you.

The junit 4 situation is a bit weird, even more than I recalled it to be. I 
didn't remember us stripping classes from the jar and don't think this is true 
for the 4.11 jar in our repo right now (I think we currently rely on junit3 
being on the classpath first). I wouldn't bother trying to strip classes in 
fetch.xml.

We probably need both licenses for JUnit as 3.8.2 sill uses the CPL.

Oh my. Sorry for being a pain.


---

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

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

[jaikiran]

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-unsubscr...@ant.apache.org
For additional commands, e-mail: dev-h...@ant.apache.org

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

[jaikiran]

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 issue #49: [master branch] - Fix BZ-58683

[bodewig]

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-unsubscr...@ant.apache.org
For additional commands, e-mail: dev-h...@ant.apache.org

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

[bodewig]

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 issue #49: [master branch] - Fix BZ-58683

[jaikiran]

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-unsubscr...@ant.apache.org
For additional commands, e-mail: dev-h...@ant.apache.org

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

[jaikiran]

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

[jaikiran]

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

[jaikiran]

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

[jaikiran]

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

[jaikiran]

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

[jaikiran]

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