dep-usage: output aliases information

Review Request #3984 — Created June 8, 2016 and submitted

peiyu
pants
3569
pants-reviews
nhoward_tw, stuhood, zundel

Output alias information to declared dependencies allows offline
script to determine which those aliases can be further cleaned up:

1) aliases without any dependency being used can be removed.
2) aliases with some dependencies' products uesd can be replaced by
those depedencies.
3) aliases with all their dependencies used won't change.

It outputs information in json like this:

  "a/b/c":
    "cost": 30862, 
    "cost_transitive": 4095740906, 
    "dependencies": [
      {
        "aliases": [
          "finagle/finagle-http:finagle-http"
        ],       
        "dependency_type": "declared", 
        "products_used": 0, 
        "products_used_ratio": 0.0, 
        "target": "finagle/finagle-http/src/main/java:java"
      },   
      {
        "aliases": [
          "finagle/finagle-http:finagle-http"
        ],       
        "dependency_type": "declared", 
        "products_used": 14, 
        "products_used_ratio": 0.024013722126929673, 
        "target": "finagle/finagle-http/src/main/scala:scala"
      },
      {
        "aliases": [
          "finagle/finagle-stats:finagle-stats"
        ], 
        "dependency_type": "declared", 
        "products_used": 0, 
        "products_used_ratio": 0.0, 
        "target": "finagle/finagle-stats/src/main/scala:scala"
      },
     ...

In this example, we know
1) we can completely get rid of finagle/finagle-stats:finagle-stats because its resolved aliases do not provide any products
2) finagle/finagle-http:finagle-http is partially used, we can replace it with finagle/finagle-http/src/main/scala:scala, essentially removed the unused finagle/finagle-http/src/main/java:java

https://travis-ci.org/peiyuwang/pants/builds/135994166 passed
https://travis-ci.org/peiyuwang/pants/builds/137144212 passed

  • 0
  • 0
  • 3
  • 0
  • 3
Description From Last Updated
NH
  1. Looks pretty good. I've got a few comments below and one general comment.

  2. Should this also check for aliases declared via the alias macro https://rbcommons.com/s/twitter/r/3939/?

    1. thanks for the comment, looks like so, added a check now.

  3. I'd like to see a test case with transitive aliases, since this is change affects how recursive traversal of aliases works.

    1. Modified the test case to reflect this, one alias is still non-transitive, the other one is transitive. The implementation keeps track of the top level nested aliases.

  4. nit: please use set literals. Also, could you please flip the args to assertEquals so that expected is on the left, like it is below?

    1. fixed and fixed.

  5. nit: Please use set literals here.

  6. Do you think it would be worth while for this feature to retain path information in the case of Target dependencies pointing at other Target dependencies?

    hat way you could partially inline used alias targets in cases where a transitive alias was largely used, but some of the root declared alias were not.

ST
  1. I might be misunderstanding, but would this be cleaner as:

    "dependencies": [
      {
        "dependency_type": "declared", 
        "products_used": 0, 
        "products_used_ratio": 0.0, 
        "target": "finagle/finagle-http/src/main/java:java",
        "aliases": ["finagle/finagle-http"]
      }
    ]
    

    ...ie, just labelling individual dependencies as having come from particular aliases?

    1. Good point, changed.

  2. Please update the method doc to indicate what is being yielded now.

    1. documented now.

  3. json support booleans

    1. removed separate aliases section, this flag is not needed any more.

  4. 
      
PE
PE
PE
ST
  1. Thanks!

    Spelling/clarity issue in review description: 3. aliases with all their dependencie used will stay for the benefit.

  2. 
      
PE
PE
Review request changed

Status: Closed (submitted)

Change Summary:

Merged as 36cb5752b12289ceefa4312673f813ca9892d5b3

Loading...