Skip to content
This repository has been archived by the owner on Nov 10, 2023. It is now read-only.

"Assuming foo.src.zip is a JAR and renaming to foo.src.jar" suggestion should be "... and renaming to foo-sources.jar" #855

Closed
davido opened this issue Aug 23, 2016 · 7 comments
Closed

"Assuming foo.src.zip is a JAR and renaming to foo.src.jar" suggestion should be "... and renaming to foo-sources.jar" #855

davido opened this issue Aug 23, 2016 · 7 comments

Comments

@davido
Copy link
Contributor

davido commented Aug 23, 2016

After upgrading to 6a19cf9 we've got this warning:

Assuming edit.src.zip is a JAR and renaming to edit.src.jar in //gerrit-patch-jgit:edit_src. Change the extension of the binary_jar to '.jar' to remove this warning.

on this rule:

genrule(
  name = 'jgit_edit_src',
  cmd = 'unzip -qd $TMP $(location //lib/jgit/org.eclipse.jgit:jgit_src) ' +
    'org/eclipse/jgit/diff/Edit.java;' +
    'cd $TMP;' +
    'zip -Dq $OUT org/eclipse/jgit/diff/Edit.java',
  out = 'edit.src.zip',
)

After renaming the output in the above rule to edit.src.jar, the above warning is disappeared and buck build gerrit still works.

However, after that renaming, buck test is failing with this error:

BUILD FAILED: //gerrit-patch-jgit:Edit failed on step javac_jar with an exception:
no source files
java.lang.IllegalStateException: no source files
    at com.sun.tools.javac.main.Main.error(Main.java:186)
    at com.sun.tools.javac.main.Main.compile(Main.java:428)
    at com.sun.tools.javac.api.JavacTaskImpl.doCall(JavacTaskImpl.java:129)
    at com.sun.tools.javac.api.JavacTaskImpl.call(JavacTaskImpl.java:138)
    at com.facebook.buck.jvm.java.Jsr199Javac.buildWithClasspath(Jsr199Javac.java:247)
    at com.facebook.buck.jvm.java.Jsr199Javac.buildWithClasspath(Jsr199Javac.java:155)
    at com.facebook.buck.jvm.java.JavacStep.tryBuildWithFirstOrderDeps(JavacStep.java:154)
    at com.facebook.buck.jvm.java.JavacStep.execute(JavacStep.java:137)
    at com.facebook.buck.jvm.java.JavacDirectToJarStep.execute(JavacDirectToJarStep.java:113)
    at com.facebook.buck.step.DefaultStepRunner.runStepForBuildTarget(DefaultStepRunner.java:63)
    at com.facebook.buck.rules.CachingBuildEngine.executeCommandsNowThatDepsAreBuilt(CachingBuildEngine.java:1254)
    at com.facebook.buck.rules.CachingBuildEngine.access$4(CachingBuildEngine.java:1235)
    at com.facebook.buck.rules.CachingBuildEngine$3$1.call(CachingBuildEngine.java:306)
    at com.facebook.buck.rules.CachingBuildEngine$3$1.call(CachingBuildEngine.java:1)
    at com.facebook.buck.util.concurrent.WeightedListeningExecutorService$1.apply(WeightedListeningExecutorService.java:65)
    at com.facebook.buck.util.concurrent.WeightedListeningExecutorService$1.apply(WeightedListeningExecutorService.java:1)
    at com.google.common.util.concurrent.Futures$AsyncChainingFuture.doTransform(Futures.java:1442)
    at com.google.common.util.concurrent.Futures$AsyncChainingFuture.doTransform(Futures.java:1433)
    at com.google.common.util.concurrent.Futures$AbstractChainingFuture.run(Futures.java:1408)
    at com.google.common.util.concurrent.Futures$2$1.run(Futures.java:1177)
    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
    at java.lang.Thread.run(Thread.java:745)

To reproduce, clone Gerrit Code Review from here: [1].
@Coneko
Copy link

Coneko commented Aug 23, 2016

cc @shs96c

@Coneko
Copy link

Coneko commented Aug 23, 2016

Maybe zip appends .zip to the file automatically, so even if you set the output to be edit.src.jar you end up with edit.src.jar.zip?

We used export_file to rename the files from .zip to .jar.

@davido
Copy link
Contributor Author

davido commented Aug 24, 2016

OK, I looked into it and found the root cause: Jsr199Javac.java:391 reads:

if (pathString.endsWith(".java")) {
        // For an ordinary .java file, create a corresponding JavaFileObject.
        Iterable<? extends JavaFileObject> javaFileObjects = fileManager.getJavaFileObjects(
            absolutifier.apply(path).toFile());
        compilationUnits.add(Iterables.getOnlyElement(javaFileObjects));
      } else if (pathString.endsWith(SRC_ZIP) || pathString.endsWith(SRC_JAR)) {

where the definitions of the constants are:

  String SRC_ZIP = ".src.zip";
  String SRC_JAR = "-sources.jar";

so after following the suggestion form the warning:

Assuming edit.src.zip is a JAR and renaming to edit.src.jar [...]

and renaming edit.src.zip to edit.src.jar the rule is broken. The correct suggestion should be:

Assuming edit.src.zip is a JAR and renaming to edit-sources.jar [...]

Moreover, looking at the current documentation, i see that this new warning/suggestion isn't reflected there as well:

srcs: (defaults to []) The set of .java files to compile for this rule.
If any of the files in this list end in .src.zip, then the entries in the
ZIP file that end in .java will be included as ordinary inputs to 
compilation.

[1] https://buckbuild.com/rule/java_library.html

@davido
Copy link
Contributor Author

davido commented Aug 24, 2016 •
edited
Loading

Unrelated to this issue, but it worth noting, that Bazel requires another suffix: .srcjar in java_library rule[1]:

Source files of type .srcjar are unpacked and compiled.
(This is useful if you need to generate a set of .java files with a genrule.)

It would be nice if Buck could also support it, so that identical rules can be used in both tool chains.

[1] http://bazel.io/docs/be/java.html#java_library

@davido davido changed the title "Assuming foo.zip is a JAR and renaming to foo.jar" warning "Assuming foo.src.zip is a JAR and renaming to foo.src.jar" suggestion should be "... and renaming to foo-sources.jar" Aug 24, 2016
@Coneko
Copy link

Coneko commented Aug 24, 2016

Oh I see what is going on: you have a genrule creating a source jar and use it both as sources and as binary in a prebuilt_jar.

Let's see what we can do.

@Coneko
Copy link

Coneko commented Aug 24, 2016

cc @marcinkosiba, @shs96c

@davido
Copy link
Contributor Author

davido commented Sep 20, 2016

It was fixed by renaming the input file.

@davido davido closed this as completed Sep 20, 2016
lucamilanesio pushed a commit to GerritCodeReview/gerrit that referenced this issue Sep 21, 2016
@davido @dpursehouse
Update Buck to latest version
0b774c4 
This version fixed a major issue: [1] that was a reason of frustration
of many plugin developers: Not cache sources files under symbolic link.
Now for all such source files, the warning is issued:

"
Disabling caching for target //plugins/wip:wip__plugin, because one or
more input files are under a symbolic link
({plugins/wip=/home/davido/projects/wip}). This will severely impact
performance! To resolve this, use separate rules and declare
dependencies instead of using symbolic links.
"

To suppress this warning we add project.allow_symlink option. This
doesn't have any impact for gerrit core but silences the warning above
when plugins are built in gerrit tree mode.

As pointed out in this issue: [2], we are using some artifacts as source
to the java_library() rule as well as binary_jar for prebuilt_ja rule.
To avoid the warning, we rename sources to have "-sources.jar" suffix
and we rename *.zip to end with .jar in other places.

"
Assuming edit.src.zip is a JAR and renaming to edit.src.jar in
//gerrit-patch-jgit:edit_src. Change the extension of the binary_jar to
'.jar' to remove this warning.
"

source_under_test attribute was removed from java_test() rule.
Replication and cookbook-plugin are updated as well.

local.properties support was removed, but we use it only for download
process customization in our own python script, so that we can keep it
usage and not need to move it to .buckconfig.local.

[1] facebook/buck#341
[2] facebook/buck#855

Change-Id: Idf76cc71c21df43e808179b645f9175767b322a8
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@davido @Coneko