ticket:11039: optionally build ICU4J jar without ICU runtime data

ticket:11039: optionally build ICU4J jar without ICU runtime data

Make all Ant targets for copying data conditional on a new property
named icu4c.data.path which, if set, inhibits all data copying and
updates the ICUConfig.properties file to use this path.

Use in this way:
$ ant -Dicu4c.data.path=/tmp/icu/build/data/out/tmp check

R=markus.icu@gmail.com

Committed: http://bugs.icu-project.org/trac/changeset/36996

[markus.icu, 2015-01-30 03:48:19]

I would like Yoshito to review this too.

[markus.icu, 2015-01-30 03:53:25]

One minor comment, otherwise looks great!

https://codereview.appspot.com/198130043/diff/1/main/classes/core/build.xml
File main/classes/core/build.xml (right):

https://codereview.appspot.com/198130043/diff/1/main/classes/core/build.xml#n...
main/classes/core/build.xml:72: <target name="update-properties"
if="icu4j.data.path" description="Update properties file">
The name update-properties seems too generic. I would call it update-icuconfig
or maybe specifically set-icuconfig-datapath.

[roubert (google), 2015-01-30 11:21:06]

https://codereview.appspot.com/198130043/diff/1/main/classes/core/build.xml
File main/classes/core/build.xml (right):

https://codereview.appspot.com/198130043/diff/1/main/classes/core/build.xml#n...
main/classes/core/build.xml:72: <target name="update-properties"
if="icu4j.data.path" description="Update properties file">
On 2015/01/30 03:53:25, markus.icu wrote:
> The name update-properties seems too generic. I would call it update-icuconfig
> or maybe specifically set-icuconfig-datapath.

Done.

[yoshito, 2015-01-30 15:53:41]

Some additional comments from me.

https://codereview.appspot.com/198130043/diff/20001/main/classes/core/build.xml
File main/classes/core/build.xml (right):

https://codereview.appspot.com/198130043/diff/20001/main/classes/core/build.x...
main/classes/core/build.xml:28: <target name="jar" depends="compile, copy,
set-icuconfig-datapath, @jar" description="Create the project's jar file"/>
I think "set-icuconfig-datapath" should be triggered by "copy-data" target
conditionally. See my comments in "copy-data" target.

https://codereview.appspot.com/198130043/diff/20001/main/classes/core/build.x...
main/classes/core/build.xml:34: <target name="copy-data"
unless="icu4j.data.path" description="Extract pre-built ICU core data files and
copy them to the project's binary directory">
"copy-data" is a public target and explicitly called from eclipse project. Thus,
you can run test, etc on Eclipse IDE. Because some of you may want such
configuration also on Eclipse IDE while developing something, my suggestion is
to call set-icuconfig-datapath conditionally from this target, instead of "jar"
target.

https://codereview.appspot.com/198130043/diff/20001/main/classes/core/build.x...
main/classes/core/build.xml:72: <target name="set-icuconfig-datapath"
if="icu4j.data.path" description="Set dataPath in ICUConfig.properties">
I think this type of ant target should be treated as an internal target. So I
would not use description="...." here. See the discussion in this link
[http://stackoverflow.com/questions/5754321/difference-between-private-and-public-targets-in-ant-scripts]

[roubert (google), 2015-01-30 16:30:30]

I've now also renamed the property to icu4c.data.path so that it doesn't clash
with the existing icu4j.data.path property defined in the
main/shared/build/common.properties file to give the relative path to the ICU4J
data directory.

https://codereview.appspot.com/198130043/diff/20001/main/classes/core/build.xml
File main/classes/core/build.xml (right):

https://codereview.appspot.com/198130043/diff/20001/main/classes/core/build.x...
main/classes/core/build.xml:28: <target name="jar" depends="compile, copy,
set-icuconfig-datapath, @jar" description="Create the project's jar file"/>
On 2015/01/30 15:53:41, yoshito wrote:
> I think "set-icuconfig-datapath" should be triggered by "copy-data" target
> conditionally. See my comments in "copy-data" target.

Done.

https://codereview.appspot.com/198130043/diff/20001/main/classes/core/build.x...
main/classes/core/build.xml:34: <target name="copy-data"
unless="icu4j.data.path" description="Extract pre-built ICU core data files and
copy them to the project's binary directory">
On 2015/01/30 15:53:41, yoshito wrote:
> "copy-data" is a public target and explicitly called from eclipse project.
Thus,
> you can run test, etc on Eclipse IDE. Because some of you may want such
> configuration also on Eclipse IDE while developing something, my suggestion is
> to call set-icuconfig-datapath conditionally from this target, instead of
"jar"
> target.

Done.

https://codereview.appspot.com/198130043/diff/20001/main/classes/core/build.x...
main/classes/core/build.xml:72: <target name="set-icuconfig-datapath"
if="icu4j.data.path" description="Set dataPath in ICUConfig.properties">
On 2015/01/30 15:53:41, yoshito wrote:
> I think this type of ant target should be treated as an internal target. So I
> would not use description="...." here. See the discussion in this link
>
[http://stackoverflow.com/questions/5754321/difference-between-private-and-public-targets-in-ant-scripts]

Done.

[yoshito, 2015-01-30 16:46:19]

Reviewed patch#3. I guess it does not work.. can you verify?

https://codereview.appspot.com/198130043/diff/40001/main/classes/core/build.xml
File main/classes/core/build.xml (right):

https://codereview.appspot.com/198130043/diff/40001/main/classes/core/build.x...
main/classes/core/build.xml:34: <target name="copy-data"
depends="set-icuconfig-datapath" unless="icu4c.data.path" description="Extract
pre-built ICU core data files and copy them to the project's binary directory">
If my understanding is correct, unless="icu4c.data.path" is evaluated first.
When "icu4c.data.path" is defined, the dependency "set-icuconfig-datapath" is
not invoked.

I think you probably need to create one extra target for 'regular' operation -
for example:

<target name="copy-data" depends="copy-data-default, set-icu-config-datapath"
... />

Then,

<target name="copy-data-default" unless="icu4c.data.path">
  <!-- normal copy operation -->
</target>

<target name="set-icuconfig-datapath" if="icu4c.data.path">
  <!-- update ICUConfig.properties -->
</target>

I did not try this yet - can you verify?

[yoshito, 2015-01-30 16:52:53]

On 2015/01/30 16:46:19, yoshito wrote:
> Reviewed patch#3. I guess it does not work.. can you verify?
> 
>
https://codereview.appspot.com/198130043/diff/40001/main/classes/core/build.xml
> File main/classes/core/build.xml (right):
> 
>
https://codereview.appspot.com/198130043/diff/40001/main/classes/core/build.x...
> main/classes/core/build.xml:34: <target name="copy-data"
> depends="set-icuconfig-datapath" unless="icu4c.data.path" description="Extract
> pre-built ICU core data files and copy them to the project's binary
directory">
> If my understanding is correct, unless="icu4c.data.path" is evaluated first.
> When "icu4c.data.path" is defined, the dependency "set-icuconfig-datapath" is
> not invoked.
> 
> I think you probably need to create one extra target for 'regular' operation -
> for example:
> 
> <target name="copy-data" depends="copy-data-default, set-icu-config-datapath"
> ... />
> 
> Then,
> 
> <target name="copy-data-default" unless="icu4c.data.path">
>   <!-- normal copy operation -->
> </target>
> 
> <target name="set-icuconfig-datapath" if="icu4c.data.path">
>   <!-- update ICUConfig.properties -->
> </target>
> 
> I did not try this yet - can you verify?

Oops.. it looks depends="..." is evaluated first. So your change should be fine.
Sorry.

[roubert (google), 2015-01-30 16:54:06]

https://codereview.appspot.com/198130043/diff/40001/main/classes/core/build.xml
File main/classes/core/build.xml (right):

https://codereview.appspot.com/198130043/diff/40001/main/classes/core/build.x...
main/classes/core/build.xml:34: <target name="copy-data"
depends="set-icuconfig-datapath" unless="icu4c.data.path" description="Extract
pre-built ICU core data files and copy them to the project's binary directory">
Ant dependencies are not restricted by target conditionals:

https://ant.apache.org/manual/targets.html

"Important: the if and unless attributes only enable or disable the target to
which they are attached. They do not control whether or not targets that a
conditional target depends upon get executed."

I have also tested these changes by building with and without icu4c.data.path
set, and it works as expected.

[markus.icu, 2015-01-30 16:56:29]

LGTM

Thanks Fredrik and Yoshito!!

[roubert (google), 2015-01-30 16:59:31]

Committed patchset #3 (id:40001) manually as 36996 (presubmit successful).