https://pantsbuild.org/ logo
#development
Title
# development
b

bored-art-40741

09/29/2021, 11:43 PM
Hmm, it looks like all javac and junit tests are broken at HEAD :\
I'm going to work on root causing and fixing this, since it's hard to develop a parallel Scala integration when the Java backend it's based on is broken
f

fast-nail-55400

09/29/2021, 11:52 PM
we still have the @maybe_skip_jdk_test stuff in place because of the flakiness
was there any response to your coursier fix?
we should probably just fork coursier for now and distribute our own binary
b

bored-art-40741

09/29/2021, 11:53 PM
I also put a small amount of effort into figuring out how to cut a Coursier binary, and I have no clue
It looks like they've changed build systems since the last time they cut a binary
w

witty-crayon-22786

09/29/2021, 11:54 PM
sbt magic…
b

bored-art-40741

09/29/2021, 11:54 PM
They switched away from sbt :\
To Mill
w

witty-crayon-22786

09/29/2021, 11:54 PM
oh my. i’ll ping archambault
b

bored-art-40741

09/29/2021, 11:55 PM
I could also easily apply my patch to a known-good release commit if someone can figure out how to cut a binary off of that
w

witty-crayon-22786

09/29/2021, 11:56 PM
b

bored-art-40741

09/29/2021, 11:57 PM
Nice, thanks!
Hm, well, one usability wart I'm immediately hitting here is that fallible compiled classfiles in javac appear to completely clobber the actual error that caused the failure
I'm not sure if I'm missing something here
f

fast-nail-55400

09/30/2021, 12:14 AM
in the meantime, should I add a non-blocking JDK-only shard to CI?
b

bored-art-40741

09/30/2021, 12:15 AM
That probably would have prevented this regression, at least
w

witty-crayon-22786

09/30/2021, 12:18 AM
@bored-art-40741: the errors are rendered via logging, which isn't disabled by default in tests. Yea, that needs improving.
I can take a look at fixing them in a few hours.
b

bored-art-40741

09/30/2021, 12:24 AM
Does coarsening not trigger dependency inference?
I've set it up so it should definitely depend on another file target based on inference, but I'm getting back the file target I passed in and nothing else
Ahh no it doesn't, does it, because of
_DependencyMappingRequest.expanded_targets = False
Huh, but even getting expanded targets first doesn't seem to pick it up
ohhh I bet it's because I don't have the dep inference rules installed in the rule runner, duh
And now I'm seeing
Copy code
E       ValueError: Encountered 7 rule graph errors:
E         No installed rules return the type FirstPartyJavaTargetsMappingMarker, and it was not provided by potential callers of @rule(pants.backend.java.dependency_inference.package_mapper:79:map_first_party_java_targets_to_symbols(FirstPartyJavaTargetsMappingMarker) -> FirstPartyJavaMappingImpl, gets=[Get(Targets, AddressSpecs), Get(SourceFiles, SourceFilesRequest), Get(JavaSourceDependencyAnalysis, SourceFiles)]).
E           If that type should be computed by a rule, ensure that that rule is installed.
which I feel like someone else recently encountered
I think probably my test needs to explicitly register the union marker, though I'm not 100% why given that the dep inference tests don't
FirstPartyPythonTargetsMappingMarker
is never mentioned outside the module where it's defined, and that's what I based the Java dep inference on
Yeah I'm actually stuck on this one. I can sort of vaguely understand why it's mad about not having a rule for that marker, but the classic
QueryRule(FirstPartyJavaTargetsMappingMarker, ()),
didn't do the trick, and that more or less exhausts my ability to debug union rule errors
In case you're curious to repro, this is basically all I did to trigger the rule errors:
Copy code
diff --git a/src/python/pants/backend/java/compile/javac_test.py b/src/python/pants/backend/java/compile/javac_test.py
index 4e929be6d..f3de375b0 100644
--- a/src/python/pants/backend/java/compile/javac_test.py
+++ b/src/python/pants/backend/java/compile/javac_test.py
@@ -16,6 +16,10 @@ from pants.backend.java.compile.javac import (
     JavacCheckRequest,
 )
 from pants.backend.java.compile.javac import rules as javac_rules
+from pants.backend.java.dependency_inference.package_mapper import (
+    FirstPartyJavaTargetsMappingMarker,
+)
+from pants.backend.java.dependency_inference.rules import rules as dep_inference_rules
 from pants.backend.java.target_types import JavaSourcesGeneratorTarget
 from pants.backend.java.target_types import rules as target_types_rules
 from pants.build_graph.address import Address
@@ -54,11 +58,14 @@ def rule_runner() -> RuleRunner:
             *util_rules(),
             *target_types_rules(),
             *coursier_rules(),
+            *dep_inference_rules(),
w

witty-crayon-22786

09/30/2021, 1:57 AM
will look in a sec.
to be clear: are all tests broken at HEAD, or is that fixed at this point…?
b

bored-art-40741

09/30/2021, 1:58 AM
Still broken, this is where I got stuck trying to fix them
w

witty-crayon-22786

09/30/2021, 1:59 AM
k. will check it out.
@bored-art-40741: i see one junit failure: does that sound right?
(and i caused it… oof.)
b

bored-art-40741

09/30/2021, 2:11 AM
Also javac
I'm here for reference:
Copy code
commit 5ff76bf86e891081b388151f09af6752219e5bd0 (HEAD -> main, upstream/main)
Author: Stu Hood <stuhood@gmail.com>
Date:   Wed Sep 29 15:09:39 2021 -0700

    [internal] Record metadata on engine-aware params (#13040)
And of course I have this diff locally:
Copy code
$ git diff -- src/python/pants/jvm/testutil.py
diff --git a/src/python/pants/jvm/testutil.py b/src/python/pants/jvm/testutil.py
index aab60bf62..3cb4bd69e 100644
--- a/src/python/pants/jvm/testutil.py
+++ b/src/python/pants/jvm/testutil.py
@@ -1,12 +1,9 @@
 # Copyright 2021 Pants project contributors (see CONTRIBUTORS.md).
 # Licensed under the Apache License, Version 2.0 (see LICENSE).
 
-import os
-
-import pytest
-
 
 def maybe_skip_jdk_test(func):
-    return pytest.mark.skipif("PANTS_RUN_JDK_TESTS" not in os.environ, reason="Skip JDK tests")(
-        func
-    )
+    return func
+    # return pytest.mark.skipif("PANTS_RUN_JDK_TESTS" not in os.environ, reason="Skip JDK tests")(
+    #     func
+    # )
w

witty-crayon-22786

09/30/2021, 2:14 AM
(fwiw, to run the tests without deleting the decorator, can set
--extra-env-vars='PANTS_RUN_JDK_TESTS=1'
)
b

bored-art-40741

09/30/2021, 2:14 AM
ah yeah I was looking for that
w

witty-crayon-22786

09/30/2021, 2:14 AM
am fixing the junit failure now, will look at javac next
test --extra-env-vars=…
b

bored-art-40741

09/30/2021, 2:19 AM
So on a clean checkout, I show 2 failing java tests and 7 failing junit tests
w

witty-crayon-22786

09/30/2021, 2:32 AM
@bored-art-40741: i see only one for `junit`: what types of errors are you seeing there?
b

bored-art-40741

09/30/2021, 2:33 AM
snippet:
Copy code
___________________ test_vintage_and_jupiter_simple_success ____________________

rule_runner = RuleRunner(build_root=/tmp/RuleRunner.lndredeu/BUILD_ROOT)

    @maybe_skip_jdk_test
    def test_vintage_and_jupiter_simple_success(rule_runner: RuleRunner) -> None:
        combined_lockfile = CoursierResolvedLockfile(
            entries=(*JUNIT4_RESOLVED_LOCKFILE.entries, *JUNIT5_RESOLVED_LOCKFILE.entries)
        )
        rule_runner.write_files(
            {
                "coursier_resolve.lockfile": combined_lockfile.to_json().decode("utf-8"),
                "BUILD": dedent(
                    """\
                    jvm_artifact(
                      name='junit_junit',
                      group='junit',
                      artifact='junit',
                      version='4.13.2',
                    )
                    jvm_artifact(
                      name='org.junit.jupiter_junit-jupiter-api',
                      group='org.junit.jupiter',
                      artifact='junit-jupiter-api',
                      version='5.7.2',
                    )
                    coursier_lockfile(
                        name = 'lockfile',
                        requirements = [
                            ':junit_junit',
                            ':org.junit.jupiter_junit-jupiter-api',
                        ],
                        sources = [
                            "coursier_resolve.lockfile",
                        ],
                    )
    
                    junit_tests(
                        name='example-test',
                        dependencies= [':lockfile'],
                    )
                    """
                ),
                "JupiterTest.java": dedent(
                    """
                    package org.pantsbuild.example;
    
                    import static org.junit.jupiter.api.Assertions.assertEquals;
                    import org.junit.jupiter.api.Test;
    
                    class JupiterTest {
                        @Test
                        void testHello(){
                          assertEquals("Hello!", "Hello!");
                       }
                    }
                    """
                ),
                "VintageTest.java": dedent(
                    """
                    package org.pantsbuild.example;
    
                    import junit.framework.TestCase;
    
                    public class VintageTest extends TestCase {
                       public void testGoodbye(){
                          assertTrue("Hello!" == "Hello!");
                       }
                    }
                    """
                ),
            }
        )
    
        test_result = run_junit_test(rule_runner, "example-test", "JupiterTest.java")
    
        assert test_result.exit_code == 0
        # TODO: Once support for parsing junit.xml is implemented, use that to determine status so we can remove the
        #  hack to use ASCII test output in the `run_junit_test` rule.
>       assert re.search(r"Finished:\s+testHello", test_result.stdout) is not None
E       AssertionError: assert None is not None
E        +  where None = <function search at 0x7f7ccf9140d0>('Finished:\\s+testHello', '\nThanks for using JUnit! Support its development at <https://junit.org/sponsoring>\n\nTest execution started. Number o...  ]\n[         0 tests aborted         ]\n[         0 tests successful      ]\n[         0 tests failed          ]\n\n')
E        +    where <function search at 0x7f7ccf9140d0> = re.search
E        +    and   '\nThanks for using JUnit! Support its development at <https://junit.org/sponsoring>\n\nTest execution started. Number o...  ]\n[         0 tests aborted         ]\n[         0 tests successful      ]\n[         0 tests failed          ]\n\n' = TestResult(exit_code=0, stdout='\nThanks for using JUnit! Support its development at <https://junit.org/sponsoring>\n\nT...st.java.example-test/TEST-junit-jupiter.xml,.JupiterTest.java.example-test/TEST-junit-vintage.xml)), extra_output=None).stdout

src/python/pants/backend/java/test/junit_test.py:662: AssertionError
---------------------------- Captured stderr setup -----------------------------
Preserving rule runner temporary directories at /tmp/RuleRunner.lndredeu.
- generated xml file: /tmp/process-execution4N3Xju/src.python.pants.backend.java.test.junit_test.py.tests.xml -
=========================== short test summary info ============================
FAILED src/python/pants/backend/java/test/junit_test.py::test_vintage_simple_success
FAILED src/python/pants/backend/java/test/junit_test.py::test_vintage_simple_failure
FAILED src/python/pants/backend/java/test/junit_test.py::test_vintage_success_with_dep
FAILED src/python/pants/backend/java/test/junit_test.py::test_jupiter_simple_success
FAILED src/python/pants/backend/java/test/junit_test.py::test_jupiter_simple_failure
FAILED src/python/pants/backend/java/test/junit_test.py::test_jupiter_success_with_dep
FAILED src/python/pants/backend/java/test/junit_test.py::test_vintage_and_jupiter_simple_success
============================== 7 failed in 1.77s ===============================
w

witty-crayon-22786

09/30/2021, 2:33 AM
…this is probably because these tests are using
--system
, isn’t it.
b

bored-art-40741

09/30/2021, 2:34 AM
Can't be helping
Oh yeah, almost certainly now that I think about it
w

witty-crayon-22786

09/30/2021, 2:34 AM
@bored-art-40741: so, yea:
test_vintage_and_jupiter_simple_success
is suspicious. i don’t actually know why that one ever passed. it tries to run tests in two different files… but those are separate targets, so they’ll run in separate invokes
b

bored-art-40741

09/30/2021, 2:35 AM
This is that original issue I saw where it doesn't find classfiles that were compiled with a newer JVM than the one running the tests
w

witty-crayon-22786

09/30/2021, 2:39 AM
@ancient-vegetable-10556, @fast-nail-55400, @bored-art-40741: so, while these are disabled (and i’m going to switch priorities tonight to getting them re-enabled), we must be running tests before landing things
👍 2
b

bored-art-40741

09/30/2021, 2:40 AM
yup
w

witty-crayon-22786

09/30/2021, 2:40 AM
a fair amount of this is probably from the inference (afaict), but until they’re re-enabled, we need to be checking one another.
test_vintage_and_jupiter_simple_success
went from succeeding to failing because inference converted it into two invokes, etc
back in 30, and then will focus on getting the tests reenabled by default
b

bored-art-40741

09/30/2021, 2:41 AM
Yeah, some of those I expected, and they were basically designed to codify the non-inference behavior
Same with one or two javac tests that are failing as expected
w

witty-crayon-22786

09/30/2021, 3:37 AM
ok, i think i know why the fetches are colliding. will get a patch out tonight.
(because we don’t override the cache directory, every test instance uses the global cache… by default they would all be isolated. fixing that will mean that every test will suddenly have its own cache, which will also need resolving.)
seems to work locally, but let’s see: https://github.com/pantsbuild/pants/pull/13046