Skip to content
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 @maven//:outdated to check for updated versions of artifacts #465

Merged
merged 1 commit into from
Nov 14, 2020

Conversation

cheister
Copy link
Collaborator

This adds an outdated action to a maven_install repository so you can run

bazel run @maven//:outdated

to check for updated versions of artifacts. e.g.

Checking for updates of 23 artifacts against the following repositories:
	https://repo1.maven.org/maven2
	https://digitalassetsdk.bintray.com/DigitalAssetSDK
	https://maven.google.com
	https://packages.confluent.io/maven/

org.pantsbuild:jarjar [1.6.6 -> 1.7.2]
junit:junit [4.12 -> 4.13.1]
org.jetbrains.kotlin:kotlin-test [1.3.21 -> 1.4.10]
com.digitalasset:damlc [100.12.1 -> 100.13.45]
com.squareup:javapoet [1.11.1 -> 1.13.0]


set -euo pipefail

if [ -f "private/tools/prebuilt/outdated_deploy.jar" ]; then
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if there is a better way to check if the action is running inside the rules_jvm_external repo or outside of it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's an open issue that coursier should run with the java runtime configured for the build (#445) I'd suggest relying on that, and then you could use $(location) to expand the location of the deploy jar in a rule or macro that writes this file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell #445 is still an open issue because JAVABASE is not exposed to repository rules. If we fix JAVABASE for the java that coursier uses we should fix it here also but I don't currently see a straightforward fix otherwise.

releaseVersion = getReleaseVersion(repository, groupId, artifactId);
if (releaseVersion != null) {
verboseLog(String.format("Found version [%s] for %s:%s in %s", releaseVersion, groupId, artifactId, repository));
// Should we search all repositories in the list for latest version instead of just the first
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the first one found has been working for me but may not find the truly latest version of an artifact in the list of repositories.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is reasonable, but to reduce potential confusion, I think the repository where this lookup succeeded should be printed regardless of RJE_VERBOSE.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mind if we leave this only in verbose mode? Mainly because it keeps the output clean and I'm not sure I know of any other outdated tool that tells you where it found the update by default.

private/outdated.sh Outdated Show resolved Hide resolved
Copy link
Collaborator

@shs96c shs96c left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic in Outdated.java looks good to me.


set -euo pipefail

if [ -f "private/tools/prebuilt/outdated_deploy.jar" ]; then
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's an open issue that coursier should run with the java runtime configured for the build (#445) I'd suggest relying on that, and then you could use $(location) to expand the location of the deploy jar in a rule or macro that writes this file.

List<String> artifacts = Files.readAllLines(Paths.get(artifactsFilePath), StandardCharsets.UTF_8);
List<String> repositories = Files.readAllLines(Paths.get(repositoriesFilePath), StandardCharsets.UTF_8);

System.out.println(String.format("Checking for updates of %d artifacts against the following repositories:", artifacts.size()));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: System.out.printf

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mainly kept it with String.format since it matches all of the other logging in the file that uses verboseLog instaed of System.out.println

artifacts = []
for a in repository_ctx.attr.artifacts:
artifacts.append(json_parse(a))
for artifact in repository_ctx.attr.artifacts:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the future, please separate refactoring changes from feature changes for a smaller diff :)

releaseVersion = getReleaseVersion(repository, groupId, artifactId);
if (releaseVersion != null) {
verboseLog(String.format("Found version [%s] for %s:%s in %s", releaseVersion, groupId, artifactId, repository));
// Should we search all repositories in the list for latest version instead of just the first
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is reasonable, but to reduce potential confusion, I think the repository where this lookup succeeded should be printed regardless of RJE_VERBOSE.

@cheister cheister merged commit 1c52e73 into bazelbuild:master Nov 14, 2020
@cheister cheister deleted the add_outdated_action branch November 14, 2020 21:21
@jin jin mentioned this pull request Jan 6, 2021
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants