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

Outdated: handle more maven-metadata.xml files #508

Merged
merged 2 commits into from
Jan 12, 2021

Conversation

dhalperi
Copy link
Contributor

@dhalperi dhalperi commented Jan 6, 2021

Fix #507

The current maven-metadata.xml for javax.inject:javax.inject is missing a
release or latest tag.

    <metadata>
      <groupId>javax.inject</groupId>
      <artifactId>javax.inject</artifactId>
      <version>1</version>
      <versioning>
        <versions>
          <version>1</version>
        </versions>
        <lastUpdated>20100720032040</lastUpdated>
      </versioning>
    </metadata>

In this case, grab the first version in the list.

With the new code paths, the control flow was getting hard to track,
so I switched to returning at decision time.


This change is Reviewable

@dhalperi
Copy link
Contributor Author

dhalperi commented Jan 6, 2021

Some notes:

  • I regenerated the deploy jar, but I guess a maintainer will want to (re)-do this for security?
  • The stack trace in bazel run @maven//:outdated crashes with NPE (4.0, java11) #507 does not line up with the existing source code (L56/L58). So I'm assuming the deploy jar was out of date.
  • I didn't see any unit tests to update, please point me there if I should.

@jin
Copy link
Collaborator

jin commented Jan 6, 2021

@cheister shall we switch Outdated.jar to be built from source when it's requested? I'm also thinking of doing the same for the hasher.

@dhalperi
Copy link
Contributor Author

dhalperi commented Jan 6, 2021

Would that require users to have to add outdated's deps in some way to our own WORKSPACE files?

@jin
Copy link
Collaborator

jin commented Jan 7, 2021

Yep. That's already needed today if you want to use java_export to Google Cloud Storage, with the deps.bzl setup macros.

@cheister
Copy link
Collaborator

cheister commented Jan 9, 2021

Thanks for the PR. I'll look into adding better tests for this class

@cheister cheister mentioned this pull request Jan 9, 2021
@cheister
Copy link
Collaborator

Do you mind rebasing this PR to get the tests from #511 ?

Fix bazel-contrib#507

The current `maven-metadata.xml` for `javax.inject:javax.inject` is missing a
release or latest tag.

    <metadata>
     <groupId>javax.inject</groupId>
     <artifactId>javax.inject</artifactId>
     <version>1</version>
     <versioning>
      <versions>
       <version>1</version>
      </versions>
      <lastUpdated>20100720032040</lastUpdated>
     </versioning>
    </metadata>

In this case, grab the first version in the list.

With the new code paths, the control flow was getting hard to track,
so I switched to returning at decision time.
@dhalperi
Copy link
Contributor Author

Do you mind rebasing this PR to get the tests from #511 ?

Done. I also rewrote the (commented-out) test to use classpath resources so it doesn't need Internet.

Copy link
Collaborator

@cheister cheister left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!

@dhalperi
Copy link
Contributor Author

Great! Just checking, what are next steps? Obviously, I won't be able to (squash&)merge the PR.

@jin jin merged commit 576cc9d into bazel-contrib:master Jan 12, 2021
@dhalperi dhalperi deleted the fix-crash branch January 12, 2021 17:32
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.

bazel run @maven//:outdated crashes with NPE (4.0, java11)
3 participants