CopySpec.remapTarget is gone!

[Steve Appling]

CopySpec.remapTarget is gone and we were actually using it.  The replacement
rename method that takes a closure isn't equivalent.  It only takes a string
file name as a parameter (and as the return), so I can't do things like flatten
a directory tree during a copy.  The previous remapTarget method (admittedly a
poor name) took and returned File objects so you could tell the directory the
source files were in (and control the directory of the target).  We were also
using the fact that you could previously return a null to not copy the file. I
would like these abilities back in some form.

--
Steve Appling
Automated Logic Research Team

---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email

[Adam Murdoch-2]

Steve Appling wrote:

> CopySpec.remapTarget is gone and we were actually using it.  The
> replacement rename method that takes a closure isn't equivalent.  It
> only takes a string file name as a parameter (and as the return), so I
> can't do things like flatten a directory tree during a copy.  The
> previous remapTarget method (admittedly a poor name) took and returned
> File objects so you could tell the directory the source files were in
> (and control the directory of the target).  We were also using the
> fact that you could previously return a null to not copy the file. I
> would like these abilities back in some form.
>

They will be back. I moved remapTarget() out of the way to make it
easier to change the archive tasks to use CopySpec. Once that is done, I
am planning on adding remapping back in some form.

I'd like to separate how you specify whether a file should be included
in the result and how you specify the target path for a file.

To include and exclude files, we have
PatternFilterable.include(Spec<FileTreeElement>) and include(Closure),
plus the equivalent exclude() methods. FileTreeElement provides a bunch
of meta info about the source file. So you can do something like:

include { FileTreeElement e ->
e.file.text.contains("environment=${targetEnvironment}") }

To specify the target path, we would add a general mapTo() method. You
would provide a mapping from FileTreeElement to the destination relative
path for the file. The path would be interpreted relative to the root of
the destination (whether that is a directory or a zip file or whatever).
Something like:

mapTo { FileTreeElement e ->
   return e.path.replaceAll('-TARGET.zip', "-${version}.zip")
}

We might also replace what is currently rename() with a mapPath()
method, where you provide a mapping from String relative path to String
relative path:

mapName { it.replaceAll('-TARGET.zip', "-${version}.zip") }


--
Adam Murdoch
Gradle Developer
http://www.gradle.org


---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email

[John Murph]

I'm not sure that separating include/exclude rules from the remapping is a good idea.  On the surface it sure sounds reasonable, but I think it would make our use case much uglier.  Our remap code is sorta like:

remapTarget { File inputFile ->
  if (not a special file)
     return inputFile

  perform phase one renaming // fairly complicated
  perform phase two renaming // also complicated

  if (resultName is not valid) // did phase one and phase two result in the desired format?
     return null
  else
     return resultName
}

The point is that phase one and phase two renaming are not simple replaceAll kinds of functions.  This means that if we were to split up these concepts we would has an include/exclude closure that performed the logic and then checks if the resultName is valid.  Then we need a mapTo closure that performed the logic and returned the result (with an assert failure if the result name is not valid?).  Now, we could pull this common stuff into a method, but it just seems an overly complicated way of getting the desired behavior.  It also seems like it would affect performance when processing over 30,000 files.


--
John Murph
Automated Logic Research Team

[Adam Murdoch-2]

John Murph wrote:

> I'm not sure that separating include/exclude rules from the remapping
> is a good idea.  On the surface it sure sounds reasonable, but I think
> it would make our use case much uglier.  Our remap code is sorta like:
>
> remapTarget { File inputFile ->
>   if (not a special file)
>      return inputFile
>
>   perform phase one renaming // fairly complicated
>   perform phase two renaming // also complicated
>
>   if (resultName is not valid) // did phase one and phase two result
> in the desired format?
>      return null
>   else
>      return resultName
> }
>
> The point is that phase one and phase two renaming are not simple
> replaceAll kinds of functions.  This means that if we were to split up
> these concepts we would has an include/exclude closure that performed
> the logic and then checks if the resultName is valid.  Then we need a
> mapTo closure that performed the logic and returned the result (with
> an assert failure if the result name is not valid?).  Now, we could
> pull this common stuff into a method, but it just seems an overly
> complicated way of getting the desired behavior.  It also seems like
> it would affect performance when processing over 30,000 files.
>

This is a reasonable use case. I'm not sold on the solution, however.  
My concern is this:

There are currently 3 primitive operations you can apply to a CopySpec:
- selecting: decide whether a given file should be excluded from the result
- mapping: determine the destination path for a given file
- filtering: filter the content of a given file when copying it

Each of these operations is logically independent. But sometimes, as in
your case, a given piece of build logic means that more than 1 of these
operations need to be applied to a given file.

The problem with combining selecting and mapping in the mapTo() method
is that it only solves the problem for the particular combination of
operations that you happen to need. It doesn't solve it for builds that
need to map and filter or select and filter. This becomes more of a
problem as we add more operations, such as
- signing: determine the signature for a given file
- permissions: determine the owner, group and permissions for a given file

So, merging the primitive operations into discrete combinations is not a
general solution, nor a particularly backwards compatible one.

I think instead, we want a single 'do everything' method which you can
pass a closure (or Action) which can apply any of the primitive
operations to the given file, however it wants. The existing selecting,
mapping and filtering method would remain simply as convenience methods.

This method might look something like:

eachFile { FileCopyDetails details ->
    if (not a special file) {
        return
    }

    perform phase one renaming
    perform phase two renaming

    if (resultName is not valid) {
        details.exclude()
    }
    else {
        details.setPath(resultName)
    }
}

FileCopyDetails would extend FileVisitDetails and add methods to allow
you to:
- exclude the file from the result
- set the destination name or the path for the file
- set the filter to use when reading or writing to the file


--
Adam Murdoch
Gradle Developer
http://www.gradle.org


---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email

[John Murph]

On Wed, Oct 28, 2009 at 9:31 PM, Adam Murdoch <a@...> wrote:

 <snip>

This method might look something like:

eachFile { FileCopyDetails details ->

  if (not a special file) {
      return
  }

  perform phase one renaming
  perform phase two renaming

  if (resultName is not valid) {
      details.exclude()
  }
  else {
      details.setPath(resultName)
  }
}

FileCopyDetails would extend FileVisitDetails and add methods to allow you to:
- exclude the file from the result
- set the destination name or the path for the file
- set the filter to use when reading or writing to the file

That sounds like a good solution to me.  Especially since we are needing to filter as well in our current use-case.  We are doing that by having two different froms in the copySpec.  Both has the same remapTarget closure, but one excludes files that we need to filter and the other includes files we need to filter and sets up a filter.  Like this:

copy.from(fromDir) {
    filteredFiles.each { exclude it }
    remapTarget remapClosure
}
copy.from(fromDir) {
    filteredFiles.each { include it }
    remapTarget remapClosure
    filter(ReplaceTokens, tokens:replacedTokens)
}

This is because filtering binary files often corrupts them, so we only wanted to filter some of the files.  We didn't like this way is it requires the copy to make two passes through the from directory, however.  Your idea will allow us to do this in one pass, and it's nice and explicit.  I like it.  I guess this is a case of "state your problem, not your solution".  Sorry about that.


--
John Murph
Automated Logic Research Team