Submitted at r3646.
On Wed, Sep 10, 2008 at 11:14 PM, Freeland Abbott wrote:
> I actually looked at the TODO to figure out a way to consolidate most of
> that stuff, which would achieve much the same effect. But I decided I
> didn't want to expand this patch...
>
> (A subdir of ${project.build} actually hits the same problem, since we copy
> ${project.build}... I think you meant a sibling of it?)
>
>
>
> On Wed, Sep 10, 2008 at 7:10 PM, Scott Blum <[EMAIL PROTECTED]> wrote:
>
>> 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
-~----------~----~----~----~------~----~------~--~---
