I went back and read up both these bugzilla reports again and I now
realize that supporting one doesn't necessarily mean not supporting the
other. BZ-43426 is about letting existing file be overwritten
(irrespective of the type of the file) if the overwrite flag is true.
BZ-58683 is about not overwriting any kind of file, if overwrite flag is
false (before the changes I introduced, it was in fact overwriting files
even when overwrite was false, a side-effect of the ln -s usage).
So I have now pushed a fix[1] on top of my previous changes, which
should accommodate both these use cases (and continue to use Java 7 APIs
for symlinking). No change has been done to the existing test and it now
passes. Do let me know, if this is good enough to have BZ-58683 fixed
and yet maintain backward compatibility with existing released versions
of Ant. If you do see any issues with this set of changes, I am willing
to undo them and go back to the original state that was before I started
these changes.
[1]
https://github.com/apache/ant/commit/b3c7d5dc451960986a94d24785a2c1d24b0b0d6a
-Jaikiran
On 10/12/17 5:15 PM, Stefan Bodewig wrote:
[@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:
<quote>
Overwrite existing links or not.
</quote>
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 <symlink> to
replace regular files.
Stefan
---------------------------------------------------------------------
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
