Skip generating reports for empty resolves

Review Request #3625 — Created March 29, 2016 and submitted

nhoward_tw
pants
3108, 3114
pants-reviews
benjyw, molsen, zundel

Currently, when no resolve was run, but the user requested a report, ivy resolve will fail looking for the report file.

This changes that behavior so that if a resolve result has no associated artifacts, an html report won't be generated and if all the resolve results were empty, it will log a message to that effect to the console.

Wrote tests to replicate the behavior, replicated the failure and made them pass.

CI away on the PR: 3114.

  • 0
  • 0
  • 0
  • 1
  • 1
Description From Last Updated
  1. 
      
  2. src/python/pants/backend/jvm/tasks/ivy_resolve.py (Diff revision 1)
     
     
     
     
     
     
     

    Filtering twice here makes it more likely that the message will go out of sync with the report generation loop. Maybe just filter the collection once, and then execute both actions based on the content of the filtered list.

  3. 
      
  1. Ship It!
  2. 
      
  1. 
      
  2. This will be a little cryptic for the user. Why was no resolution performed? Is this an error?

    Does it make sense to instead "generate" an empty report (i.e., some hard-coded XML document that our report parsers will understand as representing an empty report) and not log anything? After all, this is a legitimate state to be in, no?

    1. there is code that generates an empty report, but honestly that is just as cryptic when there are multiple resolves happening. Maybe more to the point would be to say something to the effect of, "no resolves through ivy were required, therefore none were performed."

    2. According to the description of this change: "if a resolve result has no associated artifacts, an html report won't be generated". And I'm wondering why not generate an empty report, and have the behavior be uniform with the non-empty case instead of treating it specially.

      At first glance, calling out this case in a log seems a bit arbitrary. I can't think of anywhere else where we log the fact that something is empty, or a no-op, or vacuously true (which isn't to say that there aren't any). If the right result is achieved in the end, and all artifacts are in the right state, why do we need to call this out? Is it actionable? I think I'm not understanding the problem this solves.

    3. I was thinking it would make sense to not generate reports for possible resolves that were not actually run. But, I'm fine with narrowing the change to just fixing the TypeError and retaining the existing behavior. My only concern here is that if managed_jar_dependencies are being used, at least two reports will be generated, one of them empty, only differentiated by a hash in the file name. I think this will be confusing. Maybe I could change the code so that the open flag will only open non-empty reports?

    4. I'm so out of my depth here... Not a part of the codebase I know well any more. :) But why would we get two reports, one empty, if managed_jar_dependencies are being used? And wouldn't we always have to choose one if --open is specified, regardless of whether one is empty?

    5. Anyway, my meta-point is that I'd prefer the behavior of "I resolved N dependencies" to be uniform for all N, including N=0.

    6. We get multiple reports because now pants can perform multiple resolves in the same build. It resolves separate managed_jar_dependencies so that one pants invocation could build targets with dependencies on conflicting versions of the same library.

    7. Right, makes sense. But that's orthogonal to the question of some of them perhaps being empty?

    8. Currently pants shells out and open a report for each conf for each resolution performed. I think this ends up opening a number of tabs or windows depending on your browser settings.

      These reports are the html files generated from xslting the ivy report for that conf. If there is no ivy report, ie there were no JarLibraries for one of the resolves, we could create a temporary, empty ivy report and then generate an empty html report file. This is what the code use to do before.

      There are empty reports in the managed_jar_dependencies case right now, because of how JarDependencyManagement splits targets into subgraphs. It creates a dict where the keys are the managed_jar_dependencies used by each set of targets. Once they've been split, each collection is resolved separately. The problem is that right now, all non-JarLibrary targets end up under the key None. So, if all JarLibraries are pinned using a single managed_jar_dependencies, we'll do two resolves and one of them won't call ivy because it will contain no jars to resolve.

      I'd like to not generate something that looks like pants ran ivy for a resolve when pants didn't. We could have a special html template for that, or we could modify the existing empty template.

      What if I were to go back to generating the empty report, but make it clearer? I could add a description element with a note saying ivy wasn't run and containing the list of the targets and their types. That should be translated into content in the resulting html report.

      We could even collect all the empty reports and generate a single html file for all of them.

    9. I'd be totally down with a fake HTML "report" that says clearly that Ivy didn't run on these targets because blah blah blah, so that the user understands what's going on when they ask to --open. Seems like the least surprising result, rather than requiring the user to squint at console logs to figure out why their browser didn't open anything.

    10. But that said, now that I realize that this is only relevant when the user explicitly asked for --report or --open (and I apologize that it took me this long...), I feel far less strongly about it, so I'm fine if you also want to push this as-is.

  3. 
      
  1. Ship It!
  2. 
      
  1. Ship It!
  2. 
      
  1. Ship It!
  2. 
      
Review request changed

Status: Closed (submitted)

Change Summary:

https://github.com/baroquebobcat/pants/commit/58a191440542ecc0cd1c254fef534ece6b751cd2

Loading...