Skip to content

Conversation

ianoc-stripe
Copy link
Contributor

@johnynek as we've been discussing, i think this would be the simplest change to add a flag to indicate the source jars/inputs to a scala_library rule do not contain any java files

@johnynek
Copy link
Contributor

@ianoc-stripe thanks for taking! this is a help.

@johnynek
Copy link
Contributor

actually, can you add a test similar to this one: https://github.com/bazelbuild/rules_scala/blob/master/test/src/main/scala/scalarules/test/twitter_scrooge/BUILD#L194 to make sure the test saw the _java.jar originally, then it is gone from the classpath?

This would be easy to regress on.

@johnynek
Copy link
Contributor

also a test_expect_failure kind where we have a srcjar with some java inside it (you can even check one in that has one file or something), and show that the build fails if we set the flag but find java.

@johnynek
Copy link
Contributor

👍

This is great!

@johnynek
Copy link
Contributor

the thrift grep test failed on buildkite:


//test/src/main/scala/scalarules/test/twitter_scrooge:java_jar_not_on_classpath FAILED in 3 out of 3 in 0.2s
--


@ittaiz
Copy link
Contributor

ittaiz commented Jun 22, 2018 via email

@johnynek
Copy link
Contributor

johnynek commented Jun 22, 2018 via email

@ianoc-stripe
Copy link
Contributor Author

@johnynek sorry about that, there was a bug with it which caused some target stuff to fail. Its should all be good now.

@johnynek johnynek merged commit dec85a9 into bazel-contrib:master Jun 22, 2018
@johnynek
Copy link
Contributor

This is great, thanks!

Copy link
Contributor

@ittaiz ittaiz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ianoc sorry for the late eyes but I don’t understand why you add the failures in the scalac run and the additional condition in “try to compile java”?
Can you just fail inside “try to compile java” if that code path is taken and the attribute is false? This will reduce logic from the ScalacProcessor.
My guess is this does have value and I’m mistaken but I couldn’t see it

javaFiles = getCommaList(argMap, "JavaFiles");

if (!expectJavaOutput && javaFiles.length != 0) {
throw new RuntimeException("Cannot hava java source files when no expected java output");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ianof sorry for the late eyes but aren’t you missing a test for this case? IIUC this is where java files are on srcs “regularly” (not in srcjar)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is a test for this case: 1d00681#diff-9e850e1e0893c9ea027f87c0b45dffabR188

@johnynek
Copy link
Contributor

So, we can't see in the build rule if the srcjar has java or not, so we can't fail in there. We can in the scalac check, that is the actual execution, but the build rules are running at analysis time, where we don't actually have the srcjar.

Currently we just assume a srcjar may have java. This patch allows you to assert you don't have java, but fail the build if that is false (which can only be detected at build time).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants