This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.

Bug 52886 - AntArtifact enhancements
Summary: AntArtifact enhancements
Status: RESOLVED FIXED
Alias: None
Product: projects
Classification: Unclassified
Component: Ant Project (show other bugs)
Version: 4.x
Hardware: All All
: P2 blocker (vote)
Assignee: David Konecny
URL:
Keywords: API, API_REVIEW_FAST
Depends on: 133727
Blocks: 47788 49653 50092 50484
  Show dependency tree
 
Reported: 2005-01-04 09:43 UTC by David Konecny
Modified: 2008-04-22 23:03 UTC (History)
3 users (show)

See Also:
Issue Type: ENHANCEMENT
Exception Reporter:


Attachments
API diff (62.81 KB, patch)
2005-01-04 09:46 UTC, David Konecny
Details | Diff
generated new Javadoc for ReferenceHelper (54.36 KB, text/html)
2005-01-04 09:47 UTC, David Konecny
Details
full diff (128.84 KB, patch)
2005-01-04 09:47 UTC, David Konecny
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Konecny 2005-01-04 09:43:58 UTC
This is cover issue for three small API change
issues which are so related that I implemented
them at once. These issues are:

*  AntArtifact should provide optional properties
for target execution
<http://www.netbeans.org/issues/show_bug.cgi?id=47788>

* AntArtifact should permit multiple build artifacts
<http://www.netbeans.org/issues/show_bug.cgi?id=50484>

* Foreign project cannot set dependency on
freeform project artifact if build.xml not inside
projdir
<http://www.netbeans.org/issues/show_bug.cgi?id=50092>

Implementation affected both existing APIs and
references schema. Some API methods were
deprecated and replacements were added. The new
schema for references was added and upgrade to new
schema is done carefully only when really needed,
that is if reference to project A is added to
project B and project A build script lies under
project's directory and its artifact has just one
output and no properties then old reference schema
is used whenever possible.

I will attach diff of API changes and also full
diff which includes also changes in other modules.
These are for example in J2SE project type the
artifact chooser lists all outputs of all
artifacts and allows user to chose only some of
them. Unit tests were updated to test apart from
old deprecated methods also the new ones.

Summary of API changes is:

Ant Artifact:

* deprecated FO getArtifactFile; use newly added
FO[] getArtifactFiles()
* deprecated URI getArtifactLocation; use newly
added URI[] getArtifactLocations()
* added Properties getProperties()

ReferenceHelper. Some of these changes (like
cleanup in reference removal methods) are not
strictly necessary for the implementation of the
issue, but they are simplifying the APIs:

* deprecated boolean addReference(AA); use newly
added String addReference(AA, URI) and/or newly
added boolean isReferenced(AA, URI)
* deprecated AA
getForeignFileReferenceAsArtifact(String); use
newly added Object[]
findArtifactAndLocation(String) instread which
returns array with both AA and URI
* deprecated String
createForeignFileReference(AA); was not doing
anything different from addReference(AA); use
directly new String addReference(AA, URI)
* deprecated void
destroyForeignFileReference(String) and boolean
removeReference(String) and boolean
removeReferenec(String, String); all these method
were very similar; use newly added boolean
destroyReference(String)

ReferenceHelper.RawReference:

* deprecated URI getScriptLocation(); use String
getScriptLocationValue()
* added constructor which acccept script location
in new format and also accept properties

Questions/suggestions are welcome.
Comment 1 David Konecny 2005-01-04 09:46:11 UTC
Created attachment 19452 [details]
API diff
Comment 2 David Konecny 2005-01-04 09:47:00 UTC
Created attachment 19453 [details]
generated new Javadoc for ReferenceHelper
Comment 3 David Konecny 2005-01-04 09:47:35 UTC
Created attachment 19454 [details]
full diff
Comment 4 Jaroslav Tulach 2005-01-07 08:35:17 UTC
What level of compatibility you want to keep? Not much I guess, but
how much? 

It just seems to me that future extension like this, that would try to
be compatible might be much more complicated.
Comment 5 David Konecny 2005-01-07 12:26:13 UTC
Yarda, I'm not sure I understand you. What are the levels I can choose
from? And I guess you speak mainly about change in schema? 

Anyway, the solution is 100% backward compatible.
Comment 6 Jaroslav Tulach 2005-01-07 14:38:11 UTC
Just a short note:

100% means everything that worked before will work again. If that was
the goal, I do not think it was reached:

class MyAA extends AntArtifact {
  protected FileObject[] getArtifactFiles() { }
}

would compile and run on 4.0 but not now, now it will not compile and
will not even link. So 100% was not reached. Maybe 99%.

class MyAA2 extends AntArtifact {
  protected Properties getProperties { }
}

is similar, but it will not just compile, it will link and run well on
4.1 if compiled with 4.0. So say 98.5%

Originally I thought the problem will be:

class MyAA2 extends AntArtifact {
}
MyAA2 aa2 = new MyAA2();
aa2.getArtifactLocation();

if this compiled on 4.0, then it would run on 4.0, but on 4.1 it would
throw StackOverFlow. This is not possible, as the method was abstract
in 4.0 and this could not compile. So still 98.5%

But (and that is the reason I said that in future this will not be
that easy), if the method was not abstract (and in 4.1 version it is
not and getArtifactLocations is not as well), the MyAA2 would fail
exactly in the way I described.

If you want to eliminate all of the above mentioned problems, you
should separate API and SPI
(http://openide.netbeans.org/tutorial/api-design.html#design.apiandspi)
for example by having AntArtifact final and taking AntArtifactImpl and
AntArtifactImpl2 interfaces:

pu. fi. cl. AntArtifact {
  public AntArtifact (AntArtifactImpl i) {
  }
  public final URI getArtifactLocation () {
    return i.getArtifactLocation();
  }
}

would be the impl in 4.0 and in 4.1 you would do:

pu. fi. cl. AntArtifact {
  public AntArtifact (AntArtifactImpl i) {
  }
  public AntArtifact (AntArtifactImpl2 i) {
  }
  public final URI getArtifactLocation () {
    if (i instanceof AntArtifactImpl2) {
      return i.getArtifactLocations()[0];
    } else {
      return i.getAtrifactLocation();
    }
  }
  public final URI[] getArtifactLocations() {
    if (i instanceof AntArtifactImpl2) {
      return i.getArtifactLocations();
    } else {
      return new URI[] { i.getAtrifactLocation() };
    }  
  }
}

this approach would be 100% compatible. 

I do not mind much that in this release you reached only 98.5%
compatiblity, but the more users, the more you will want to be 100%
compatible in future. That is why I suggest to adopt API and SPI
separation now, it will be harder with every release.

PS: ReferenceHelper seems to be final. That is good, it will save you
a lot of troubles.
Comment 7 Jesse Glick 2005-01-07 15:24:24 UTC
No subclasses of AntArtifact ought to be adding random
public/protected methods. The only reason to subclass is to override
those methods which are declared as concrete but nonfinal in the
superclass. There is no reason to pass an AntArtifact around
arbitrarily since the class has no real functionality; it is simply an
enhanced struct designed to be returned from AntArtifactQuery and read
by code in another module. Therefore there is no reason to add
additional signature methods to subclasses; there is no one to call them.

As usual, anyone who adds their own accessible methods in a subclass
does so at their own peril - they will not generally be able to
compile the same source against a newer API (don't know the exact
circumstances under which linking will work). No legitimate users of
the API should be affected.
Comment 8 Jesse Glick 2005-01-08 17:41:34 UTC
In ant-project-references2.xsd, why do you declare property-definition
to be an extension of substitutable-text? (1) It isn't substitutable
text, AFAIK, it is a fixed string. (2) substitutable-text is not even
defined in this schema, so an XML parser will just reject the schema
as erroneous before it begins validation.


Re. subclasses of AntArtifact - I think Yarda's scenario, while of
course possible, is extremely unlikely in practice and going out of
our way to prevent it is not a good use of time. Making an
AntArtifactImpl in particular would just make the API less intuitive
for typical users. We could deprecate all of AntArtifact and create an
AntArtifact2 with the new (non-deprecated) signature and update
ReferencesHelper to use only the new class in nondeprecated methods if
this is straightforward, but if not I would rather spend time on
higher-priority issues.


ReferencesHelper.canUseVersion10: is

aa.gSL().gAP().sW(pD.gAP())

safe on Windows and so on? Are we sure all relevant file paths have
already been normalized?


In tests: a tip:

new TreeSet(Arrays.asList(new String[] {"hello"}))

can be simplified to

new TreeSet(Collections.singleton("hello"))

or in this context even

Collections.singleton("hello")


In form.ClassPathUtils: call artifact.getArtifactLocations() only once.


ProjectClassPathExtender changed signatures; make sure impl dep form
-> java/project is updated (java/project/spec-ver.properties). Note
that tzezula already has an issue to make a proper public API from this.


In j2seproject build-impl.xsl, the body of the properties loop could
be simplified to

<property name="{@name}" value="{.}"/>

(I think; try it.)


Otherwise the proposed diff looks good to me. Please make sure
developers of web and j2ee project types have seen the proposed API
change (and just as importantly, the patch to j2seproject) and agree
they can make an analogous change, and that developers of the j2me
project type have seen the API and agree they can use its new features
for properties in an artifact.
Comment 9 David Konecny 2005-01-10 14:34:33 UTC
Re. "Schema" - oops, I copy&pasted it form freeform schema and
overlooked it. I changed base from "substitutable-text" to "xsd:string".

Re. "is aa.gSL().gAP().sW(pD.gAP()) safe or must be normalized" - I
checked SimpleAA and it is safe, it calls APH.resolveFile. The rule is
that io.File must be normalize when created so that any other usage of
it is safe.
Comment 10 David Konecny 2005-01-10 14:50:07 UTC
J2ME and Web team: 

it would be great if you could update your project types to use these
new APIs. In full diff you can see impact on J2SE project type (start
from AntArtifactChooser.java). More or less it is manual change in
couple of places where AntArtifact was used and where also URI must be
used now. Do you think it is doable for you for 4.1 to update your
project type? Do you have any objections against the API?

For now I plan to integrate the change later this week.

Thanks.
Comment 11 Martin Adamek 2005-01-10 15:37:25 UTC
When forwarding issues related to web project, please include also
j2ee module (ejbjarproject)
Comment 12 Adam Sotona 2005-01-10 17:32:27 UTC
wow - 2400 lines of differencies that's pretty big change going across
all the code

Do you have it in some branch ?
Comment 13 David Konecny 2005-01-11 07:48:06 UTC
No branch, just diff. As I said lot of changes outside of ant/project
are about searching for "AA" and replacing it with "AA+URI". Quite
trivial.
Comment 14 David Konecny 2005-01-13 13:49:47 UTC
Integrated:
ant/freeform/src/org/netbeans/modules/ant/freeform/ArtifactProvider.java;
new revision: 1.4; previous revision: 1.3
ant/freeform/test/unit/src/org/netbeans/modules/ant/freeform/ArtifactProviderTest.java;
new revision: 1.4; previous revision: 1.3
ant/project/apichanges.xml;
new revision: 1.4; previous revision: 1.3
ant/project/manifest.mf;
new revision: 1.8; previous revision: 1.7
ant/project/src/org/netbeans/api/project/ant/AntArtifact.java;
new revision: 1.10; previous revision: 1.9
ant/project/src/org/netbeans/modules/project/ant/StandardAntArtifactQueryImpl.java;
new revision: 1.3; previous revision: 1.2
ant/project/src/org/netbeans/modules/project/ant/ant-project-references2.xsd;
initial revision: 1.1
ant/project/src/org/netbeans/spi/project/support/ant/ReferenceHelper.java;
new revision: 1.22; previous revision: 1.21
ant/project/src/org/netbeans/spi/project/support/ant/SimpleAntArtifact.java;
new revision: 1.10; previous revision: 1.9
ant/project/test/unit/src/org/netbeans/api/project/ant/AntArtifactQueryTest.java;
new revision: 1.5; previous revision: 1.4
ant/project/test/unit/src/org/netbeans/spi/project/support/ant/ReferenceHelperTest.java;
new revision: 1.15; previous revision: 1.14
ant/project/test/unit/src/org/netbeans/spi/project/support/ant/SimpleAntArtifactTest.java;
new revision: 1.8; previous revision: 1.7
java/freeform/src/org/netbeans/modules/java/freeform/JavaProjectGenerator.java;
new revision: 1.3; previous revision: 1.2
java/j2seproject/src/org/netbeans/modules/java/j2seproject/classpath/J2SEProjectClassPathExtender.java;
new revision: 1.9; previous revision: 1.8
java/j2seproject/src/org/netbeans/modules/java/j2seproject/resources/build-impl.xsl;
new revision: 1.50; previous revision: 1.49
java/j2seproject/src/org/netbeans/modules/java/j2seproject/ui/LibrariesNode.java;
new revision: 1.6; previous revision: 1.5
java/j2seproject/src/org/netbeans/modules/java/j2seproject/ui/ProjectNode.java;
new revision: 1.5; previous revision: 1.4
java/j2seproject/src/org/netbeans/modules/java/j2seproject/ui/customizer/AntArtifactChooser.java;
new revision: 1.10; previous revision: 1.9
java/j2seproject/src/org/netbeans/modules/java/j2seproject/ui/customizer/J2SEProjectProperties.java;
new revision: 1.37; previous revision: 1.36
java/j2seproject/src/org/netbeans/modules/java/j2seproject/ui/customizer/VisualClassPathItem.java;
new revision: 1.11; previous revision: 1.10
java/j2seproject/src/org/netbeans/modules/java/j2seproject/ui/customizer/VisualClasspathSupport.java;
new revision: 1.18; previous revision: 1.17
form/src/org/netbeans/modules/form/project/ClassPathUtils.java;
new revision: 1.9; previous revision: 1.8
Comment 15 Vince Kraemer 2005-01-15 06:04:36 UTC
deprecating a type safe method for a method which requires casting:

Brilliant!  I love to do Smalltalk programming in Java.

deprecated AA
getForeignFileReferenceAsArtifact(String); use
newly added Object[]
findArtifactAndLocation(String) instread which
returns array with both AA and URI

And I just love having to check for null, length and embedded null-ness, instead of just 
null

returning URI[] instead of URI, etc.
Comment 16 Jesse Glick 2005-01-15 17:52:49 UTC
You do not need to check the return value of RH.fAAL for null or check
its length; it is documented to always return an array of length 2.
Sorry about the casts but Java has no tuple syntax and producing a new
struct class for this method return value seems like overkill.

BTW next time if you want to comment on an API it would be better to
do so while it is still under review and there is a chance to make
stylistic adjustments.