Nested source directories not honored on other platform

[davido]

Consider the following content of BUCK build file in gerrit-war sub-directory:

genrule(
  name = 'webapp_assets',
  cmd = 'cd resources/webapp && zip -qr $OUT .',
  cmd_exe = 'cd resources\\webapp && zip -qr %OUT% .',
  srcs = glob(['resources/webapp/**/*']),
  deps = [],
  out = 'webapp_assets.zip',
  visibility = ['//:'],
)

And the following source directory structure:

$ ls -R gerrit-war
gerrit-war:
BUCK  resources
gerrit-war/resources:
webapp
gerrit-war/resources/webapp:
WEB-INF
gerrit-war/resources/webapp/WEB-INF:
web.xml

The following verbose invocation describes the problem:

buck build gerrit-war:webapp_assets --verbose 10
[-] PARSING BUCK FILES...FINISHED 0,0s
rm -f C:\Users\david\projects\buck_test\buck-out\gen\gerrit-war\webapp_assets.zip
mkdir -p 'C:\Users\david\projects\buck_test\buck-out\gen\gerrit-war'
rm -r -f C:\Users\david\projects\buck_test\buck-out\gen\gerrit-war\webapp_assets__tmp && mkdir -p 'C:\Users\david\projects\buck_test\buck-ou
t\gen\gerrit-war\webapp_assets__tmp'
rm -r -f C:\Users\david\projects\buck_test\buck-out\gen\gerrit-war\webapp_assets__srcs && mkdir -p 'C:\Users\david\projects\buck_test\buck-o
ut\gen\gerrit-war\webapp_assets__srcs'
mkdir -p 'C:\Users\david\projects\buck_test\buck-out\gen\gerrit-war\webapp_assets__srcs' && ln -f -s C:\Users\david\projects\buck_test\gerri
t-war\resources\webapp\WEB-INF\web.xml C:\Users\david\projects\buck_test\buck-out\gen\gerrit-war\webapp_assets__srcs\web.xml
(cd 'C:\Users\david\projects\buck_test\buck-out\gen\gerrit-war\webapp_assets__srcs' && OUT='C:\Users\david\projects\buck_test\buck-out\gen\g
errit-war\webapp_assets.zip' cmd.exe /c 'cd resources\webapp && zip -qr %OUT% .')

Path cannot be found.

It seems like mkdir and link steps are broken; I would expect them to be:

mkdir -p 'C:\Users\david\projects\buck_test\buck-out\gen\gerrit-war\webapp_assets__srcs\resources\webapp\WEB-INF' && ln -f -s C:\Users\david\projects\b
uck_test\gerrit-war\resources\webapp\WEB-INF\web.xml C:\Users\david\projects\buck_test\buck-out\gen\gerrit-war\webapp_assets__srcs\resources\webapp\WEB-INF\web.xml

Note: when the same rule is placed in BUCK in the root directory and the content of gerrit-war sub-directory is also moved to the base project directory, then it works as expected:

buck build :webapp_assets --verbose 10
[-] PARSING BUCK FILES...FINISHED 0,0s
rm -f C:\Users\david\projects\buck_test\buck-out\gen\webapp_assets.zip
mkdir -p 'C:\Users\david\projects\buck_test\buck-out\gen'
rm -r -f C:\Users\david\projects\buck_test\buck-out\gen\webapp_assets__tmp && mkdir -p 'C:\Users\david\projects\buck_test\buck-out\gen\webap
p_assets__tmp'
rm -r -f C:\Users\david\projects\buck_test\buck-out\gen\webapp_assets__srcs && mkdir -p 'C:\Users\david\projects\buck_test\buck-out\gen\weba
pp_assets__srcs'
mkdir -p 'C:\Users\david\projects\buck_test\buck-out\gen\webapp_assets__srcs\resources\webapp\WEB-INF' && ln -f -s C:\Users\david\projects\b
uck_test\resources\webapp\WEB-INF\web.xml C:\Users\david\projects\buck_test\buck-out\gen\webapp_assets__srcs\resources\webapp\WEB-INF\web.xm
l
(cd 'C:\Users\david\projects\buck_test\buck-out\gen\webapp_assets__srcs' && OUT='C:\Users\david\projects\buck_test\buck-out\gen\webapp_asset
s.zip' cmd.exe /c 'cd resources\webapp && zip -qr %OUT% .')
[-] BUILDING...FINISHED 0,4s

In this case the content of target zip file is also correct:

$ unzip -t buck-out/gen/webapp_assets.zip
Archive:  buck-out/gen/webapp_assets.zip
    testing: WEB-INF/                 OK
    testing: WEB-INF/web.xml          OK
No errors detected in compressed data of buck-out/gen/webapp_assets.zip.

[davido]

OK, tracked it down. This is path separator mismatch bug, specific to other platform Genrule.addSymlinkCommands():329:

      [...]
      // By the time we get this far, all source paths (the keys in the map) have been converted
      // to paths relative to the project root. We want the path relative to the build target, so
      // strip the base path.
      if (entry.getValue().equals(canonicalPath)) {
        if (localPath.startsWith(basePath)) {
          localPath = localPath.substring(basePathLength);
        } else {
          localPath = canonicalPath.getFileName().toString();
        }
      }
      [...]

The values are:

basePath = "gerrit-war/"
localPath = "gerrit-war\src\main\webapp\WEB-INF\extra\jetty7\gerrit-jetty.sh"

So the condition if (localPath.startsWith(basePath)) is erroneously false.

This diff fixed this:

diff --git a/src/com/facebook/buck/shell/Genrule.java b/src/com/facebook/buck/shell/Genrule.java
old mode 100644
new mode 100755
index 2e6e967..e3dd044
--- a/src/com/facebook/buck/shell/Genrule.java
+++ b/src/com/facebook/buck/shell/Genrule.java
@@ -38,6 +38,7 @@ import com.facebook.buck.step.fs.MkdirStep;
 import com.facebook.buck.step.fs.RmStep;
 import com.facebook.buck.util.BuckConstant;
 import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.CharMatcher;
 import com.google.common.base.Function;
 import com.google.common.base.Joiner;
 import com.google.common.base.Optional;
@@ -317,6 +318,7 @@ public class Genrule extends AbstractBuildRule implements HasOutputName {
   @VisibleForTesting
   void addSymlinkCommands(ImmutableList.Builder<Step> commands) {
     String basePath = getBuildTarget().getBasePathWithSlash();
+    String basePathWithoutTrailingSlash = CharMatcher.is('/').trimTrailingFrom(basePath);
     int basePathLength = basePath.length();

     // Symlink all sources into the temp directory so that they can be used in the genrule.
@@ -330,7 +332,7 @@ public class Genrule extends AbstractBuildRule implements HasOutputName {
       // to paths relative to the project root. We want the path relative to the build target, so
       // strip the base path.
       if (entry.getValue().equals(canonicalPath)) {
-        if (localPath.startsWith(basePath)) {
+        if (localPath.startsWith(basePathWithoutTrailingSlash)) {
           localPath = localPath.substring(basePathLength);
         } else {
           localPath = canonicalPath.getFileName().toString();

[davido]

Fixed meantime by 923579e2089af2972aabe908af90fdba4ec71bf8.