You are viewing a plain text version of this content. The canonical link for it is here.
Posted to ivy-user@ant.apache.org by jeff <je...@yahoo.com> on 2007/04/21 09:58:02 UTC
code review please: issue 475
http://issues.apache.org/jira/browse/IVY-475
could someone review this fix?
essentially, i added a check in IvyInstall to see if there are any unresolved dependencies, and if so, throw a BuildException. this is the default behavior, but it can be over ridden by setting haltunresolved=false on the install task.
i also added a new test class, IvyInstallTest, that tests the missing dependency case for haltunresolved true and false.
also, i found another test case that was failing. see IBibilioResolverTest.java line 117 for the failed test, and IBiblioResolver.java (diffs) for the fix.
diffs attached, thanks.
p.s., i do have apache SVN write access, which i think applies to all projects? so if it's okay here, should be able to commit myself, if the change is acceptable.
---------------------------------
Ahhh...imagining that irresistible "new car" smell?
Check outnew cars at Yahoo! Autos.
Re: code review please: issue 475
Posted by Xavier Hanin <xa...@gmail.com>.
On 4/24/07, jeff <je...@yahoo.com> wrote:
> xavier,
>
> i've already signed the apache contributor agreements and i've contributed code to other project. please feel free to check up on that. regardless, i attached the patch to the issue also (which makes more sense anyway).
>
> i actually attached it twice, as the first time i didn't check the "grant license" checkbox, which i assume is relevant.
>
> thanks!
Thanks for your contribution! I'll try to find time to apply the patch
tonight. Will keep you informed via the issue.
Xavier
Re: code review please: issue 475
Posted by jeff <je...@yahoo.com>.
xavier,
i've already signed the apache contributor agreements and i've contributed code to other project. please feel free to check up on that. regardless, i attached the patch to the issue also (which makes more sense anyway).
i actually attached it twice, as the first time i didn't check the "grant license" checkbox, which i assume is relevant.
thanks!
Xavier Hanin <xa...@gmail.com> wrote: On 4/23/07, jeff wrote:
>
> *Xavier Hanin * wrote:
>
>
> I don't know apache rules very well, but I would be surprised if you have
> write access to our project. Indeed AFAIK commiting directly is considered
> as being a commiter, which requires a vote in our PPMC.
>
> i suspect i have write access, because when i was approved for another
> project, it was just "apache svn write access", and not specific to the
> project. but ya, i don't really know how apache works it either.
>
> but anyway, i realize i am not a committer on this project so i won't even
> try. i am attaching a patch file. would you mind applying it? i'd really
> like this to be in the next release, so if you have a 2.x release branch,
> could you put it there, as well as the trunk?
For attaching the patch I'd prefer using the issue in JIRA. I don't know
Apache rules well enough to be sure, but I know that there is no problem to
apply patches when they are attached in JIRA with the checkbox saying it's a
contribution. It's also easier to keep track of your contribution.
And about the branch, 2.0 developments are occurring on the trunk, so no
problem.
Cheers,
Xavier
> thanks!
>
> ------------------------------
> Ahhh...imagining that irresistible "new car" smell?
> Check out new cars at Yahoo! Autos.
>
>
--
Learn Ivy at ApacheCon: http://www.eu.apachecon.com/
Manage your dependencies with Ivy!
http://incubator.apache.org/ivy/
---------------------------------
Ahhh...imagining that irresistible "new car" smell?
Check outnew cars at Yahoo! Autos.
Re: code review please: issue 475
Posted by Xavier Hanin <xa...@gmail.com>.
On 4/23/07, jeff <je...@yahoo.com> wrote:
>
> *Xavier Hanin <xa...@gmail.com>* wrote:
>
>
> I don't know apache rules very well, but I would be surprised if you have
> write access to our project. Indeed AFAIK commiting directly is considered
> as being a commiter, which requires a vote in our PPMC.
>
> i suspect i have write access, because when i was approved for another
> project, it was just "apache svn write access", and not specific to the
> project. but ya, i don't really know how apache works it either.
>
> but anyway, i realize i am not a committer on this project so i won't even
> try. i am attaching a patch file. would you mind applying it? i'd really
> like this to be in the next release, so if you have a 2.x release branch,
> could you put it there, as well as the trunk?
For attaching the patch I'd prefer using the issue in JIRA. I don't know
Apache rules well enough to be sure, but I know that there is no problem to
apply patches when they are attached in JIRA with the checkbox saying it's a
contribution. It's also easier to keep track of your contribution.
And about the branch, 2.0 developments are occurring on the trunk, so no
problem.
Cheers,
Xavier
> thanks!
>
> ------------------------------
> Ahhh...imagining that irresistible "new car" smell?
> Check out new cars at Yahoo! Autos.<http://us.rd.yahoo.com/evt=48245/*http://autos.yahoo.com/new_cars.html;_ylc=X3oDMTE1YW1jcXJ2BF9TAzk3MTA3MDc2BHNlYwNtYWlsdGFncwRzbGsDbmV3LWNhcnM->
>
>
--
Learn Ivy at ApacheCon: http://www.eu.apachecon.com/
Manage your dependencies with Ivy!
http://incubator.apache.org/ivy/
Re: code review please: issue 475
Posted by jeff <je...@yahoo.com>.
Xavier Hanin <xa...@gmail.com> wrote:
This looks pretty fine. One minor thing: I think haltonunresolved instead of
haltunresolved would be more consistent with the naming used in other cases
(like haltonfailure from the resolve task).
agreed, and i actually had it has you described in the code, i just mistyped in the email.
also, i found another test case that was failing. see
> IBibilioResolverTest.java line 117 for the failed test, and
> IBiblioResolver.java (diffs) for the fix.
Yes, I have fixed the problem for the release, but haven't backported the
fix to the trunk yet. I will do that when I port some changes done for the
release to the trunk. BTW, the fix is in the test and not in the
IBiblioResolver class, since we have agreed to use repo1.maven.org instead
of ibiblio as default root (ibiblio is now considered only as a mirror of
repo1.maven.org).
ok, i will back that out of the changes.
I don't know apache rules very well, but I would be surprised if you have
write access to our project. Indeed AFAIK commiting directly is considered
as being a commiter, which requires a vote in our PPMC.
i suspect i have write access, because when i was approved for another project, it was just "apache svn write access", and not specific to the project. but ya, i don't really know how apache works it either.
but anyway, i realize i am not a committer on this project so i won't even try. i am attaching a patch file. would you mind applying it? i'd really like this to be in the next release, so if you have a 2.x release branch, could you put it there, as well as the trunk?
thanks!
---------------------------------
Ahhh...imagining that irresistible "new car" smell?
Check outnew cars at Yahoo! Autos.
Re: code review please: issue 475
Posted by Xavier Hanin <xa...@gmail.com>.
Hello Jeff,
On 4/21/07, jeff <je...@yahoo.com> wrote:
>
> http://issues.apache.org/jira/browse/IVY-475
>
> could someone review this fix?
Sure!
essentially, i added a check in IvyInstall to see if there are any
> unresolved dependencies, and if so, throw a BuildException. this is the
> default behavior, but it can be over ridden by setting haltunresolved=false
> on the install task.
This looks pretty fine. One minor thing: I think haltonunresolved instead of
haltunresolved would be more consistent with the naming used in other cases
(like haltonfailure from the resolve task).
i also added a new test class, IvyInstallTest, that tests the missing
> dependency case for haltunresolved true and false.
Great!
also, i found another test case that was failing. see
> IBibilioResolverTest.java line 117 for the failed test, and
> IBiblioResolver.java (diffs) for the fix.
Yes, I have fixed the problem for the release, but haven't backported the
fix to the trunk yet. I will do that when I port some changes done for the
release to the trunk. BTW, the fix is in the test and not in the
IBiblioResolver class, since we have agreed to use repo1.maven.org instead
of ibiblio as default root (ibiblio is now considered only as a mirror of
repo1.maven.org).
diffs attached, thanks.
>
> p.s., i do have apache SVN write access, which i think applies to all
> projects? so if it's okay here, should be able to commit myself, if the
> change is acceptable.
I don't know apache rules very well, but I would be surprised if you have
write access to our project. Indeed AFAIK commiting directly is considered
as being a commiter, which requires a vote in our PPMC.
Xavier
------------------------------
> Ahhh...imagining that irresistible "new car" smell?
> Check out new cars at Yahoo! Autos.<http://us.rd.yahoo.com/evt=48245/*http://autos.yahoo.com/new_cars.html;_ylc=X3oDMTE1YW1jcXJ2BF9TAzk3MTA3MDc2BHNlYwNtYWlsdGFncwRzbGsDbmV3LWNhcnM->
>
>
--
Learn Ivy at ApacheCon: http://www.eu.apachecon.com/
Manage your dependencies with Ivy!
http://incubator.apache.org/ivy/
Re: code review please: issue 475
Posted by jeff <je...@yahoo.com>.
hmmm, apache seems to have dropped my attachment, here are the diffs again ...
Index: test/java/org/apache/ivy/ant/IvyInstallTest.java
===================================================================
--- test/java/org/apache/ivy/ant/IvyInstallTest.java (revision 0)
+++ test/java/org/apache/ivy/ant/IvyInstallTest.java (revision 0)
@@ -0,0 +1,105 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+package org.apache.ivy.ant;
+
+import java.io.File;
+import java.io.IOException;
+
+import junit.framework.TestCase;
+
+import org.apache.ivy.TestHelper;
+import org.apache.ivy.core.IvyPatternHelper;
+import org.apache.tools.ant.BuildException;
+import org.apache.tools.ant.Project;
+import org.apache.tools.ant.taskdefs.Delete;
+
+
+public class IvyInstallTest extends TestCase {
+ private static final String IVY_RETRIEVE_PATTERN = "build/test/lib/[organisation]/[module]/ivy-[revision].xml";
+ private static final String RETRIEVE_PATTERN = "build/test/lib/[conf]/[artifact]-[revision].[type]";
+ private File _cache;
+ private IvyInstall _install;
+ private Project _project;
+
+ protected void setUp() throws Exception {
+ createCache();
+ cleanTestLib();
+ _project = new Project();
+ _project.setProperty("ivy.settings.file", "test/repositories/ivysettings.xml");
+
+ _install = new IvyInstall();
+ _install.setProject(_project);
+ _install.setCache(_cache);
+ }
+
+ private void createCache() {
+ _cache = new File("build/cache");
+ _cache.mkdirs();
+ }
+
+ protected void tearDown() throws Exception {
+ cleanCache();
+ cleanTestLib();
+ }
+
+ private void cleanCache() {
+ Delete del = new Delete();
+ del.setProject(new Project());
+ del.setDir(_cache);
+ del.execute();
+ }
+
+ private void cleanTestLib() {
+ Delete del = new Delete();
+ del.setProject(new Project());
+ del.setDir(new File("build/test/lib"));
+ del.execute();
+ }
+
+ public void testDependencyNotFoundFailure() {
+ _install.setOrganisation("xxx");
+ _install.setModule("yyy");
+ _install.setRevision("zzz");
+ _install.setFrom("test");
+ _install.setTo("1");
+
+ try {
+ _install.execute();
+ fail("unknown dependency, failure expected (haltunresolved=true)");
+ } catch (BuildException be) {
+ // success
+ be.printStackTrace();
+ }
+ }
+
+ public void testDependencyNotFoundSuccess() {
+ _install.setOrganisation("xxx");
+ _install.setModule("yyy");
+ _install.setRevision("zzz");
+ _install.setFrom("test");
+ _install.setTo("1");
+ _install.setHaltonunresolved(false);
+
+ try {
+ _install.execute();
+ } catch (BuildException be) {
+ be.printStackTrace();
+ fail("unknown dependency, failure unexepected (haltunresolved=false)");
+ }
+ }
+}
Index: src/java/org/apache/ivy/plugins/resolver/IBiblioResolver.java
===================================================================
--- src/java/org/apache/ivy/plugins/resolver/IBiblioResolver.java (revision 530909)
+++ src/java/org/apache/ivy/plugins/resolver/IBiblioResolver.java (working copy)
@@ -73,7 +73,7 @@
super.setM2compatible(m2compatible);
if (m2compatible) {
if (_root == null) {
- _root = "http://repo1.maven.org/maven2/";
+ _root = "http://www.ibiblio.org/maven2/";
}
if (_pattern == null) {
_pattern = "[organisation]/[module]/[revision]/[artifact]-[revision](-[classifier]).[ext]";
Index: src/java/org/apache/ivy/ant/IvyInstall.java
===================================================================
--- src/java/org/apache/ivy/ant/IvyInstall.java (revision 530909)
+++ src/java/org/apache/ivy/ant/IvyInstall.java (working copy)
@@ -25,12 +25,13 @@
import org.apache.ivy.plugins.matcher.PatternMatcher;
import org.apache.ivy.util.filter.FilterHelper;
import org.apache.tools.ant.BuildException;
+import org.apache.ivy.core.report.ResolveReport;
+import org.apache.tools.ant.BuildException;
-
/**
* Allow to install a module or a set of module from repository to another one.
- *
- *
+ *
+ *
* @author Xavier Hanin
*
*/
@@ -38,13 +39,14 @@
private String _organisation;
private String _module;
private String _revision;
- private File _cache;
+ private File _cache;
private boolean _overwrite = false;
private String _from;
private String _to;
private boolean _transitive;
private String _type;
private String _matcher = PatternMatcher.EXACT;
+ private boolean _haltOnUnresolved = true;
public void execute() throws BuildException {
Ivy ivy = getIvyInstance();
@@ -58,12 +60,12 @@
if (_module == null && PatternMatcher.EXACT.equals(_matcher)) {
throw new BuildException("no module name provided for ivy publish task: It can either be set explicitely via the attribute 'module' or via 'ivy.module' property or a prior call to <resolve/>");
} else if (_module == null && !PatternMatcher.EXACT.equals(_matcher)) {
- _module = PatternMatcher.ANY_EXPRESSION;
+ _module = PatternMatcher.ANY_EXPRESSION;
}
if (_revision == null && PatternMatcher.EXACT.equals(_matcher)) {
throw new BuildException("no module revision provided for ivy publish task: It can either be set explicitely via the attribute 'revision' or via 'ivy.revision' property or a prior call to <resolve/>");
} else if (_revision == null && !PatternMatcher.EXACT.equals(_matcher)) {
- _revision = PatternMatcher.ANY_EXPRESSION;
+ _revision = PatternMatcher.ANY_EXPRESSION;
}
if (_from == null) {
throw new BuildException("no from resolver name: please provide it through parameter 'from'");
@@ -72,13 +74,25 @@
throw new BuildException("no to resolver name: please provide it through parameter 'to'");
}
ModuleRevisionId mrid = ModuleRevisionId.newInstance(_organisation, _module, _revision);
+ ResolveReport report;
try {
- ivy.install(mrid, _from, _to, _transitive, doValidate(settings), _overwrite, FilterHelper.getArtifactTypeFilter(_type), _cache, _matcher);
+ report = ivy.install(mrid, _from, _to, _transitive, doValidate(settings), _overwrite, FilterHelper.getArtifactTypeFilter(_type), _cache, _matcher);
} catch (Exception e) {
throw new BuildException("impossible to install "+ mrid +": "+e, e);
}
+
+ if (report.getUnresolvedDependencies().length > 0 && isHaltonunresolved()) {
+ throw new BuildException("could not resolve dependencies - see output for details");
+ }
}
-
+
+ public boolean isHaltonunresolved() {
+ return _haltOnUnresolved;
+ }
+ public void setHaltonunresolved(boolean haltOnUnresolved) {
+ _haltOnUnresolved = haltOnUnresolved;
+ }
+
public File getCache() {
return _cache;
}
@@ -134,11 +148,11 @@
public void setType(String type) {
_type = type;
}
-
+
public String getMatcher() {
return _matcher;
}
-
+
public void setMatcher(String matcher) {
_matcher = matcher;
}
Index: src/java/org/apache/ivy/core/install/InstallEngine.java
===================================================================
--- src/java/org/apache/ivy/core/install/InstallEngine.java (revision 530909)
+++ src/java/org/apache/ivy/core/install/InstallEngine.java (working copy)
@@ -17,6 +17,7 @@
*/
package org.apache.ivy.core.install;
+import com.sun.org.apache.bcel.internal.generic.ARRAYLENGTH;
import java.io.File;
import java.io.IOException;
import java.util.Arrays;
Index: doc/doc/use/install.html
===================================================================
--- doc/doc/use/install.html (revision 530991)
+++ doc/doc/use/install.html (working copy)
@@ -56,6 +56,8 @@
<td>No, defaults to false</td></tr>
<tr><td>matcher</td><td>the name of the matcher to use to find the modules to install</td>
<td>No, defaults to exact</td></tr>
+ <tr><td>haltonunresolved</td><td>true to fail build on unresolved dependencies</td>
+ <td>No, defaults to true</td></tr>
</tbody>
</table>
<h1>Examples</h1>
jeff <je...@yahoo.com> wrote: http://issues.apache.org/jira/browse/IVY-475
could someone review this fix?
essentially, i added a check in IvyInstall to see if there are any unresolved dependencies, and if so, throw a BuildException. this is the default behavior, but it can be over ridden by setting haltunresolved=false on the install task.
i also added a new test class, IvyInstallTest, that tests the missing dependency case for haltunresolved true and false.
also, i found another test case that was failing. see IBibilioResolverTest.java line 117 for the failed test, and IBiblioResolver.java (diffs) for the fix.
diffs attached, thanks.
p.s., i do have apache SVN write access, which i think applies to all projects? so if it's okay here, should be able to commit myself, if the change is acceptable.
---------------------------------
Ahhh...imagining that irresistible "new car" smell?
Check out new cars at Yahoo! Autos.
---------------------------------
Ahhh...imagining that irresistible "new car" smell?
Check outnew cars at Yahoo! Autos.