When using soft-excludes, ignore all target defined excludes

Review Request #2340 — Created June 10, 2015 and submitted

nhoward_tw
pants
1663
26750de...
pants-reviews
fkorotkov, ity, jsirois, patricklaw, stuhood, tejal, zundel

Currently, soft-excludes only removes excludes from the ivy resolve arguments when some other target has explicitly included that jar. This means that excludes for transitive dependencies of artifacts remain even with soft-excludes enabled. If a target depends on a transitive dependency of an artifact directly, and some other target excludes it, when both are compiled together, the one with the transitive dependency will fail to compile.

This change causes soft-excludes to just skip all target defined excludes, which makes its behavior easier to reason about.

Excludes are now applied separately, so this shouldn't affect how libraries are excluded from classpaths, only how they are communicated to ivy.

Added unit tests for the new flag on calculate_classpath, CI running off of PR

  • 0
  • 0
  • 2
  • 0
  • 2
Description From Last Updated
TE
  1. ping! open it to pants-review?

  2. 
      
ST
  1. This change causes soft-excludes to just skip all target defined excludes, which makes its behavior easier to reason about.

    Can you clarify where these get applied instead?

    1. excludes for compile_classpath are now applied at the point where the classpath for a target is used. That was part of a previous change. https://rbcommons.com/s/twitter/r/2247/

    2. Ok. Can you make sure that gets explained in the review/commit message?

  2. _exclude_is_not_contained_in_jars is only used here, so you should be able to remove it from the trait.

  3. 
      
NH
ST
  1. Ship It!
  2. 
      
NH
NH
ZU
  1. Ship It!
  2. 
      
BE
  1. Please add Patrick in my stead - this is much more relevant to his recent work.

  2. 
      
NH
PA
  1. lgtm sans minor nit. I don't think this will impact us at all, and nothing sticks out as API breaking.

  2. If you need the copy, list(target.jar_dependencies) is sufficient. If you don't, the variable itself is unnecessary.

    1. Thanks. I updated it from .. = []; for... append, but I think you're right. After looking at it, I don't think we even need the variable. I'll inline it.

  3. 
      
IT
  1. 
      
  2. 
      
NH
NH
  1. Submitted at https://github.com/pantsbuild/pants/commit/ba60b22bf51edafe4a9f0f002e0d1da277ae1296

  2. 
      
NH
Review request changed

Status: Closed (submitted)

Loading...