New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ivy.updates command that looks for 3rd party jar updates with Ivy #4386

Merged
merged 1 commit into from Mar 30, 2017

Conversation

Projects
None yet
3 participants
@cheister
Contributor

cheister commented Mar 25, 2017

Problem

There is no easy way see third party jar updates in Pants along the lines of Maven's display-version-updates

Solution

Ivy has an Ant task called checkdepsupdate that does the same thing. This PR creates a wrapper around that Ant task and adds a new goal updates.ivy to generate a report of 3rd party updates.

Result

With this new goal you can now see the third party updates for a given set of targets

> ./pants updates.ivy tests/java::

...

14:29:03 00:01   [updates]
14:29:03 00:01     [ivy]
14:29:03 00:01       [dependency-update-checker]

...

Dependency updates available:
                       args4j#args4j  2.32 -> 2.33
                       com.google.code.findbugs#jsr305  3.0.0 -> 3.0.1
                       com.google.guava#guava  18.0 -> 21.0
                       com.google.guava#guava-testlib  18.0 -> 21.0
                       com.squareup.burst#burst-junit4  1.1.0 -> 1.1.1
                       commons-io#commons-io  2.4 -> 2.5
                       org.easymock#easymock  3.3.1 -> 3.4
                       org.mockito#mockito-core  2.2.16 -> 2.7.19
                       org.scala-lang#scala-library  2.11.8 -> 2.12.1
                       org.scalatest#scalatest_2.11  3.0.0 -> 3.2.0-SNAP4
@cheister

This comment has been minimized.

Show comment
Hide comment
@cheister

cheister Mar 25, 2017

Contributor

This update will need to happen in two steps because there are some placeholders for the new jar that the task refers to that are not yet published.

Contributor

cheister commented Mar 25, 2017

This update will need to happen in two steps because there are some placeholders for the new jar that the task refers to that are not yet published.

@cheister cheister requested review from baroquebobcat, stuhood and jsirois Mar 26, 2017

@stuhood

Thanks Chris... looks useful.

Since this is relatively small, and ivy is still quite embedded, I'm ok with making it non-contrib. But it's definitely worth asking: should this be contrib?

Show outdated Hide outdated ...y/pushdb/org.pantsbuild/ivy-dependency-update-checker/publish.properties
revision.major.org.pantsbuild%ivy-depdendency-update-checker=0
revision.minor.org.pantsbuild%ivy-depdendency-update-checker=0
revision.patch.org.pantsbuild%ivy-depdendency-update-checker=1
revision.snapshot.org.pantsbuild%ivy-depdendency-update-checker=false

This comment has been minimized.

@stuhood

stuhood Mar 27, 2017

Member

depdendency

But also, I don't think that this should have been published at all yet, so this file probably shouldn't be here?

@stuhood

stuhood Mar 27, 2017

Member

depdendency

But also, I don't think that this should have been published at all yet, so this file probably shouldn't be here?

This comment has been minimized.

@cheister

cheister Mar 28, 2017

Contributor

K, I'll remove this until we're ready to publish the jar.

@cheister

cheister Mar 28, 2017

Contributor

K, I'll remove this until we're ready to publish the jar.

}
}
if (listOfMissingDependencyOnLatest.size() > 0) {

This comment has been minimized.

@stuhood

stuhood Mar 27, 2017

Member

Refactoring this such that the return code of the binary can be meaningful (ie, returning non-zero if updates are available) might be interesting.

@stuhood

stuhood Mar 27, 2017

Member

Refactoring this such that the return code of the binary can be meaningful (ie, returning non-zero if updates are available) might be interesting.

This comment has been minimized.

@cheister

cheister Mar 28, 2017

Contributor

Right now a non-zero exit code means the check failed somehow. Maybe have different return codes mean different types of errors? Although, having updates available isn't really an error.

@cheister

cheister Mar 28, 2017

Contributor

Right now a non-zero exit code means the check failed somehow. Maybe have different return codes mean different types of errors? Although, having updates available isn't really an error.

Show outdated Hide outdated src/java/org/pantsbuild/tools/ivy/DependencyUpdateChecker.java
private String[] confs = { "default" };
/** Should be set to false for unit testing via {@link #setCallSystemExitOnFinish} */
private static boolean callSystemExitOnFinish = true;

This comment has been minimized.

@stuhood

stuhood Mar 27, 2017

Member

Might be good to make this volatile for forwards compatibility.

@stuhood

stuhood Mar 27, 2017

Member

Might be good to make this volatile for forwards compatibility.

Show outdated Hide outdated tests/java/org/pantsbuild/tools/junit/impl/ConsoleRunnerImplTest.java
@@ -1,3 +1,6 @@
// Copyright 2015 Pants project contributors (see CONTRIBUTORS.md).

This comment has been minimized.

@stuhood

stuhood Mar 27, 2017

Member

2017

@stuhood

This comment has been minimized.

@cheister

cheister Mar 28, 2017

Contributor

Since I was adding this retroactively, I wasn't sure if it should be 2015 or 2017. I can make it 2017

@cheister

cheister Mar 28, 2017

Contributor

Since I was adding this retroactively, I wasn't sure if it should be 2015 or 2017. I can make it 2017

return ivyInstance;
}
public ResolveReport getResolvedReport() throws ParseException, IOException {

This comment has been minimized.

@stuhood

stuhood Mar 28, 2017

Member

Memoizing like this is a bit odd. Would seem more straightforward to just calculate the report once and hold it in a local variable rather than calling this repeatedly.

@stuhood

stuhood Mar 28, 2017

Member

Memoizing like this is a bit odd. Would seem more straightforward to just calculate the report once and hold it in a local variable rather than calling this repeatedly.

This comment has been minimized.

@cheister

cheister Mar 28, 2017

Contributor

Fixed

@cheister

cheister Mar 28, 2017

Contributor

Fixed

@stuhood

Thanks Chris.

Please wait till @baroquebobcat reviews this, as he might have more to say about ivy.

@baroquebobcat

This looks like it'll be a really handy new feature. I've got a bunch of comments, mostly suggestions, with a few more major ones.

Major ones

  • How would you feel about changing the name of the goal to outdated? I think it better reflects what this does.
  • Tightening up of regex handling for the exclude-coordinates flag.
  • Clean up jvm_tool registration a bit.
  • A few Qs around test cases.

I don't consider the refactoring related comments blockers.

Show outdated Hide outdated src/java/org/pantsbuild/tools/ivy/DependencyUpdateChecker.java
if (!originalDependency.getResolvedId().getRevision()
.equals(latest.getResolvedId().getRevision())) {
// is this dependency a transitive dependency or a direct dependency
// (unfortunatly the .isTranstive() method doesn't have the same meaning)

This comment has been minimized.

@baroquebobcat

baroquebobcat Mar 28, 2017

Contributor

nit: s/unfortunatly/unfortunately/

@baroquebobcat

baroquebobcat Mar 28, 2017

Contributor

nit: s/unfortunatly/unfortunately/

Show outdated Hide outdated src/java/org/pantsbuild/tools/ivy/DependencyUpdateChecker.java
// (unfortunatly the .isTranstive() method doesn't have the same meaning)
boolean isTransitiveDependency =
latest.getDependencyDescriptor(latest.getRoot()) == null;
if ((!isTransitiveDependency) || (isTransitiveDependency && showTransitive)) {

This comment has been minimized.

@baroquebobcat

baroquebobcat Mar 28, 2017

Contributor

I think this set of ifs would be easier to read as guard clauses. ie

for (Object dependencyIvyNode...) {
  IvyNode originalDependency = ...
  if (!originalDependency.getModuleId().equals(latest.getModuleId())) {
    continue;
  }
  if (originalDependency.getResolvedId().getRevision()
      .equals(latest.getResolvedId().getRevision())) {
  continue;
  }
  boolean isTransitiveDependency =...
  if (isTransitiveDependency && !showTransitive) {
    continue;
  }
  sb.append....
@baroquebobcat

baroquebobcat Mar 28, 2017

Contributor

I think this set of ifs would be easier to read as guard clauses. ie

for (Object dependencyIvyNode...) {
  IvyNode originalDependency = ...
  if (!originalDependency.getModuleId().equals(latest.getModuleId())) {
    continue;
  }
  if (originalDependency.getResolvedId().getRevision()
      .equals(latest.getResolvedId().getRevision())) {
  continue;
  }
  boolean isTransitiveDependency =...
  if (isTransitiveDependency && !showTransitive) {
    continue;
  }
  sb.append....
Show outdated Hide outdated src/java/org/pantsbuild/tools/ivy/DependencyUpdateChecker.java
boolean isTransitiveDependency =
latest.getDependencyDescriptor(latest.getRoot()) == null;
if ((!isTransitiveDependency) || (isTransitiveDependency && showTransitive)) {
StringBuilder sb = new StringBuilder();

This comment has been minimized.

@baroquebobcat

baroquebobcat Mar 28, 2017

Contributor

Since you aren't iterating to construct the StringBuilder here, you could just use a string literal, ie INDENTATION + originalDependency.getResolvedId().getOrganisation() + "#" + ..., and the compiler will construct the StringBuilder for you.

@baroquebobcat

baroquebobcat Mar 28, 2017

Contributor

Since you aren't iterating to construct the StringBuilder here, you could just use a string literal, ie INDENTATION + originalDependency.getResolvedId().getOrganisation() + "#" + ..., and the compiler will construct the StringBuilder for you.

for (Object ivyNode2 : latestReport.getDependencies()) {
IvyNode latest = (IvyNode) ivyNode2;
if (originalDependency.getModuleId().equals(latest.getModuleId())) {
dependencyFound = true;

This comment has been minimized.

@baroquebobcat

baroquebobcat Mar 28, 2017

Contributor

You could add a break here rather than continuing the loop after the dependency was found.

@baroquebobcat

baroquebobcat Mar 28, 2017

Contributor

You could add a break here rather than continuing the loop after the dependency was found.

This comment has been minimized.

@cheister

cheister Mar 29, 2017

Contributor

This is pretty much the original code the from the ant task. I'm not sure if there was any difference to match the first or last dependency in the list. Mainly I assumed what they were using in the ant task worked so I kept that behavior

@cheister

cheister Mar 29, 2017

Contributor

This is pretty much the original code the from the ant task. I'm not sure if there was any difference to match the first or last dependency in the list. Mainly I assumed what they were using in the ant task worked so I kept that behavior

for (Object ivyNode2 : originalReport.getDependencies()) {
IvyNode originalDependency = (IvyNode) ivyNode2;
if (originalDependency.getModuleId().equals(latest.getModuleId())) {
dependencyFound = true;

This comment has been minimized.

@baroquebobcat

baroquebobcat Mar 28, 2017

Contributor

You could add a break here. Also, I think it'd make sense to extract this and the loop above in displayMissingDependencyOnLatest into a private method.

@baroquebobcat

baroquebobcat Mar 28, 2017

Contributor

You could add a break here. Also, I think it'd make sense to extract this and the loop above in displayMissingDependencyOnLatest into a private method.

Show outdated Hide outdated tests/python/pants_test/backend/jvm/tasks/test_ivy_updates_integration.py
)
"""))
pants_run = self.run_pants(['updates.ivy',
'--updates-ivy-exclude-coordinates=commons-io:commons-io',

This comment has been minimized.

@baroquebobcat

baroquebobcat Mar 28, 2017

Contributor

I'd like to see some tests that pass more general regexes.

@baroquebobcat

baroquebobcat Mar 28, 2017

Contributor

I'd like to see some tests that pass more general regexes.

ResolveReport resolvedReport =
getIvyInstance().resolve(ivyFile.toURI().toURL(), resolveOptions);
if (resolvedReport.hasError()) {
System.err.println("Resolved report has an error");

This comment has been minimized.

@baroquebobcat

baroquebobcat Mar 28, 2017

Contributor

Will running the resolve here provide any more info, or is this the only output?

@baroquebobcat

baroquebobcat Mar 28, 2017

Contributor

Will running the resolve here provide any more info, or is this the only output?

This comment has been minimized.

@cheister

cheister Mar 29, 2017

Contributor

Most of the useful error messaging has been sent to stdout at this point. I did think it was oadd that the ResolveReport has a hasError() method but no method for getting the errors.

@cheister

cheister Mar 29, 2017

Contributor

Most of the useful error messaging has been sent to stdout at this point. I did think it was oadd that the ResolveReport has a hasError() method but no method for getting the errors.

This comment has been minimized.

@baroquebobcat

baroquebobcat Mar 29, 2017

Contributor

Cool. That makes sense.

@baroquebobcat

baroquebobcat Mar 29, 2017

Contributor

Cool. That makes sense.

@@ -45,6 +45,6 @@ public InvalidCmdLineArgumentException(
String message) {
super(String.format("Invalid option value '%s' for option '%s': %s",
optionName, optionValue, message));
optionValue, optionName, message));

This comment has been minimized.

@baroquebobcat

baroquebobcat Mar 28, 2017

Contributor

Good catch! Could you add a regression test?

@baroquebobcat

baroquebobcat Mar 28, 2017

Contributor

Good catch! Could you add a regression test?

Show outdated Hide outdated src/python/pants/backend/jvm/register.py
@@ -157,6 +158,7 @@ def register_goals():
task(name='ivy', action=IvyResolve).install('resolve', first=True)
task(name='ivy-imports', action=IvyImports).install('imports')
task(name='unpack-jars', action=UnpackJars).install()
task(name='ivy', action=IvyUpdates).install('updates')

This comment has been minimized.

@baroquebobcat

baroquebobcat Mar 28, 2017

Contributor

I feel like there could be a better name than updates. Bundler from the ruby ecosystem uses outdated for their command. I think that could work.

I could imagine the use case for this would be something like:

./pants outdated 3rdparty::
[updates]
  [python]
    Updates available! There are N outdated artifacts
    abcd:1.2.3 -> abcd:1.2.4
    ...
  [ivy]
     Updates available! There are N outdated artifacts
     xyz:1.2.3 -> xyz:1.2.4
     ...
  SUCCESS
@baroquebobcat

baroquebobcat Mar 28, 2017

Contributor

I feel like there could be a better name than updates. Bundler from the ruby ecosystem uses outdated for their command. I think that could work.

I could imagine the use case for this would be something like:

./pants outdated 3rdparty::
[updates]
  [python]
    Updates available! There are N outdated artifacts
    abcd:1.2.3 -> abcd:1.2.4
    ...
  [ivy]
     Updates available! There are N outdated artifacts
     xyz:1.2.3 -> xyz:1.2.4
     ...
  SUCCESS
Show outdated Hide outdated src/python/pants/backend/jvm/tasks/ivy_updates.py
# rev='0.0.1'),
# First run ./pants binary src/java/org/pantsbuild/tools/ivy:ivy-dependency-update-checker
JarDependency(org = 'org.pantsbuild',
name = 'ivy-depdendency-update-checker',

This comment has been minimized.

@baroquebobcat

baroquebobcat Mar 28, 2017

Contributor

nit: s/depdendency/dependency/

@baroquebobcat

baroquebobcat Mar 28, 2017

Contributor

nit: s/depdendency/dependency/

@cheister

This comment has been minimized.

Show comment
Hide comment
@cheister

cheister Mar 29, 2017

Contributor

Regarding outdated vs updates. Maven and Gradle both go with dependency-updates but doing ivy-dependency-updates seemed a bit long to me. Since there was already the pattern of resolve.ivy, resolve.node, resolve.go, resolve.scala-js-compile for resolve goals I figured we could use something similar for updates starting with updates.ivy

If you really prefer outdated are you thinking of ivy.outdated ? Or just outdated ?

Contributor

cheister commented Mar 29, 2017

Regarding outdated vs updates. Maven and Gradle both go with dependency-updates but doing ivy-dependency-updates seemed a bit long to me. Since there was already the pattern of resolve.ivy, resolve.node, resolve.go, resolve.scala-js-compile for resolve goals I figured we could use something similar for updates starting with updates.ivy

If you really prefer outdated are you thinking of ivy.outdated ? Or just outdated ?

@baroquebobcat

This comment has been minimized.

Show comment
Hide comment
@baroquebobcat

baroquebobcat Mar 29, 2017

Contributor

I was thinking it would make sense to have the goal be outdated, then other 3rdparty resolution tasks that check for outdated versions could also be registered below it.

Once there's more facilities for modifying BUILD files with pants, we could have an update-ext or something goal that would bump the versions. It would be good for it to not have to overload the meaning of update(s)

Also, I just find outdated to be more specific than updates without being too much longer.

So, the usage if you wanted to use scoped options for the task in this patch would be

./pants outdated.ivy --exclude-patterns=".*" 3rdparty/jvm::

That seems clearer to me than the current name:

./pants updates.ivy --exclude-patterns=".*" 3rdparty/jvm::

That looks like it could be either printing updated versions, or actually updating them.

Contributor

baroquebobcat commented Mar 29, 2017

I was thinking it would make sense to have the goal be outdated, then other 3rdparty resolution tasks that check for outdated versions could also be registered below it.

Once there's more facilities for modifying BUILD files with pants, we could have an update-ext or something goal that would bump the versions. It would be good for it to not have to overload the meaning of update(s)

Also, I just find outdated to be more specific than updates without being too much longer.

So, the usage if you wanted to use scoped options for the task in this patch would be

./pants outdated.ivy --exclude-patterns=".*" 3rdparty/jvm::

That seems clearer to me than the current name:

./pants updates.ivy --exclude-patterns=".*" 3rdparty/jvm::

That looks like it could be either printing updated versions, or actually updating them.

@cheister

This comment has been minimized.

Show comment
Hide comment
@cheister

cheister Mar 29, 2017

Contributor

The one thing I don't like about outdated is it implies that the version you using is out-dated if you are not on the absolute latest version. This might not be true if there are dev releases and stable releases. e.g. should we consider Pants 1.2.1 outdated?

What about dependency-updates ? So we would have dependency-updates.ivy etc.

Contributor

cheister commented Mar 29, 2017

The one thing I don't like about outdated is it implies that the version you using is out-dated if you are not on the absolute latest version. This might not be true if there are dev releases and stable releases. e.g. should we consider Pants 1.2.1 outdated?

What about dependency-updates ? So we would have dependency-updates.ivy etc.

@baroquebobcat

This comment has been minimized.

Show comment
Hide comment
@baroquebobcat

baroquebobcat Mar 29, 2017

Contributor

That's fair. I don't like the idea of using dependency-updates because dependency is already really overloaded in pants due to it being the word for edges in the build graph.

How about check-update? Yum uses it for this purpose. With that name the resulting full scope would be check-update.ivy.

I still like outdated better, because it's a relatively unused word within pants' dictionary of words we could use, and it is short. But, we could move towards something like that later.

Contributor

baroquebobcat commented Mar 29, 2017

That's fair. I don't like the idea of using dependency-updates because dependency is already really overloaded in pants due to it being the word for edges in the build graph.

How about check-update? Yum uses it for this purpose. With that name the resulting full scope would be check-update.ivy.

I still like outdated better, because it's a relatively unused word within pants' dictionary of words we could use, and it is short. But, we could move towards something like that later.

@cheister

This comment has been minimized.

Show comment
Hide comment
@cheister

cheister Mar 30, 2017

Contributor

Here is what it looks like with outdated which seems better than check-updates.

Contributor

cheister commented Mar 30, 2017

Here is what it looks like with outdated which seems better than check-updates.

@baroquebobcat

🚢

@cheister cheister merged commit 2f189f1 into pantsbuild:master Mar 30, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@cheister cheister deleted the cheister:add-dependency-update-checker branch Mar 30, 2017

lenucksi added a commit to lenucksi/pants that referenced this pull request Apr 25, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment