Add 'java-resource' and 'java-test-resource' content type for Resources Roots.

Review Request #2046 - Created April 9, 2015 and submitted

Information
Tejal Desai
pants
fe9b7ec...
Reviewers
pants-reviews
benjyw, ity, patricklaw, stuhood, zundel

Add 'java-resource' and 'java-test-resource' content type for Resources Roots.

I also discovered an error in how resources content types were added. The target.target_base was used as root_dir to add resources instead of resource.target_base.
This was masked by the fact resource target was also included in self.context_targets. The right source root was registered then.

In this review, after fixing above bug, i saw 2 content roots registered for the same resource target. Changed the self.sources to be a set instead of an array.

https://travis-ci.org/pantsbuild/pants/builds/57812381

Patrick Lawson
Eric Ayers
Eric Ayers
Tejal Desai
Review request changed

Status: Closed (submitted)

Eric Ayers

   

As it turns out, this introduced a regression for us. If you have a BUILD file with both java_libarary and junit_tests targets, it is not deterministic that the folder in IntelliJ be marked as a test folder.

  1. hmm, we asked our users to refactor such cases, because Intellij does not allow a source-folder depend on test-folder. We had instances when java_library in src tree depended on a test library target in tests tree (wierd reasons. Refactoring these build files was a mess.)
    In your case, is it always safe to mark the root as test given the above restriction?

  2. Defining both libraries and tests in the same BUILD file I think is a legit use case. Test folders can contain code that is depended on by other targets, and you don't want to define that code as a junit_tests() target. We have many BUILD files like this that define a library for use by other tests as well as tests in this directory.

    Yes, it is always safe to mark the root as test. These sources already have to abide by that restriction due because they still compile with Maven.

    junit_tests(name='test',
    
      sources = [ 
        'FooTest.java',
      ]
      dependencies = [
        ':lib'
      ],
    )
    
    java_library(name='lib',
      sources = [
         'FooTestLibrary.java'
      ],
      provides = artifact(org='com.squareup',
                          name='foo',
                          repo=square,),
    )
    
  3. Ok. I don't see an issue with fixing these to be Test always. Also, right now we still have our own fork of goal idea, so this won't affect us.
    /cc @fedor.korotkov

  4. FYI it's safe to mark it as test source root till another java_library which won't be marked as tests will depend on it. In IntelliJ you can refer to sources from tests.

    BTW it's not following 1:1:1 rule. It's better to move :lib to a separate folder.

  5. I am refactoring to make it work again and adding a test.

    If a folder has code in it, doesn't it need to be a regular source folder or a test folder?

    As far as 'resources' folders go, I see how you could have a BUILD file with only a resources() target, that would determine that this should be a Resources folder. But I don't see how you could determine that this should be a 'test resources' folder vs just a plain 'resources' folder. There is nothing about a resources() target that tells you it is only supposed to be on the classpath during test.

    Based on this, I am thinking I will just remove the 'java-test-resources' setting in the file.

  6. Ok, now that I'm more into the code, I see what it is trying to do, which is find all targets that have resources and pull in the resources targets they depend on and add them as 'resource only' folders. There is still a problem:

    The same resources() target could be depended on by both test and nontest targets
    The BUILD with resources() in it could have code defined in it as well.

    targets that are depended on in 'resources=[]' stanzas should automatically get pulled in as dependencies, shouldn't they? This means they should already be in the list of targts, so I'm not sure why we need to explicitly process the 'resources=' attributes.

  7. Resources belonging to junit test target are marked as java_test_resources. This should be taken care in https://github.com/pantsbuild/pants/blob/master/src/python/pants/backend/project_info/tasks/ide_gen.py#L536

    For e.g. in the below case, foo/test/resources shd be marked as java_test_resources.
    junit_tests(name='test',

    sources = [
    'FooTest.java',
    ]
    dependencies = [
    ':lib'
    ],
    reousrces = [
    'foo/test/resources'
    ]
    )

  8. I see that is the algorithm, but I don't see that it is reliable when you consider targets like these:

    src/foo/BUILD:

    java_library(
      ...
      resources=[
        'resources/foo',
      ],
    )
    

    test/foo/BUILD:

    junit_tests(
      ...
      resources=[
        'resources/foo',
      ],
    )
    

    With the current logic, it depends on the order in which you parse the tree to determine which one wins.

  9. I'll post a PR with testcases and if it isn't right, maybe we can work out better what to do for some of these edge cases.

  10. yep makes sense. We haven't seen a case where same resources are attqched to test as well as lib and the test target does not depend on the sources of the lib. Usually, the test would depend on lib which pull in the resources.
    But again, pants does not stop you from doing that.

Loading...