This LGTM. The one thing I might tweak involves the changes to
/distro-source/<platform>/build.xml. Basically, the change you made
highlights the brittleness of slurping in all of ${project.build}, since we
might continue to introduce tools like revision filtering that add new stuff
to ${project.build} automatically.
Perhaps we should tweak the distro source build to put the copied files into
a subdirectory, like ${project.build}/filtered.
On Wed, Sep 10, 2008 at 2:14 PM, Freeland Abbott <[EMAIL PROTECTED]
> wrote:
> On Mon, Sep 8, 2008 at 10:56 AM, Scott Blum <[EMAIL PROTECTED]> wrote:
>
>> Let me rephrase the question: couldn't we have two sentinel files instead
>> of four? Right now you have two sentinel files for dev/core and two for
>> distro-source; each could get by with only one sentinel file whose name
>> composited gwt.version with gwt.svnrev.filename.
>>
>
> Ah. Yes, we could have two sentinels rather than four, sorry---I
> misunderstood which pair(s) you wanted to fold.
>
>
>> Detailed review:
>>
>> At a high level, I'm not sure why you went the route of making things so
>> general. Why does gwt.svninfo need to take parameters to say what props to
>> read the values into? Why not simply always read them into {gwt.svnrev} and
>> ${gwt.svnrev.filename}? For that matter, why does gwt.revfilter need to
>> take parameters for those values? I think both of these should use
>> hardcoded property names; the other advantage to this is the ability to
>> override these values on the command line and know you've gotten them
>> globally.
>>
>
> I like macros not to have side effects. Not strongly enough to contest it,
> so the new patch has hardcoded output properties, but that was the
> motivation. (It also would avoid any namespace issues when, inevitably,
> this gets cut-and-pasted into the "common".ant.xml of incubator,
> gwt-google-apis, etc... though I know those examples have no namespace
> issues today anyway.)
>
>
>> Ideally, those properties could be computed at the top level without
>> actually having to remember to call the macro to get them defined, but I'm
>> not sure that that's worth the effort.
>>
>
> While setting the value should be pretty "fast," if you're working just in
> user you may rather not have it set since it won't ever be used. (We only
> have two use instances; I'd swing the other way with a third one.) So,
> yeah, I passed on this one: you need to call <gwt.getsvninfo/> in any
> subproject that cares. That does, however, establish common naming for the
> sentinels and creates the sentinel directory, which cleans up some stuff.
>
> The distro-source used to skip sentinels entirely, so there's also some
> added exclusion there now that they're creating 'em.
>
>
>
>> /dev/common.ant.xml
>> - Why do you need to exclude About.properties from both source
>> directories, since it will only exist in dev/core?
>>
>
> I'd expected common to be common-inclusive-of-core, not just
> common-to-plaforms-only. ("Expected" is maybe too strong; "programmed to
> allow for" is perhaps better---but if core included common, then inside core
> "src" would be an alias for ${gwt.core.root}/src and would need the
> exclusion.) Today, not needed, so out it goes.
>
>
>> /distro-source/common.ant.xml
>> - Again, 24-25 & 40-41 can be merged to a single sentinel file.
>> - 21: What's with the echo here?
>>
>
> Yup, and debuggage now gone.
>
--~--~---------~--~----~------------~-------~--~----~
http://groups.google.com/group/Google-Web-Toolkit-Contributors
-~----------~----~----~----~------~----~------~--~---
