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

Check Android repository too #3605

Merged
merged 7 commits into from
Feb 14, 2020
Merged

Check Android repository too #3605

merged 7 commits into from
Feb 14, 2020

Conversation

elharo
Copy link
Contributor

@elharo elharo commented Feb 13, 2020

Copy link
Contributor Author

@elharo elharo left a comment

Choose a reason for hiding this comment

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

@briandealwis I think this PR hacks around the bug but it's not the right answer yet. It logs lots of weird maven exceptions it shouldn't. Please do review and comment though.

@codecov
Copy link

codecov bot commented Feb 13, 2020

Codecov Report

Merging #3605 into master will decrease coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #3605      +/-   ##
============================================
- Coverage     70.23%   70.17%   -0.06%     
- Complexity     2990     2991       +1     
============================================
  Files           377      377              
  Lines         13552    13554       +2     
  Branches       1597     1598       +1     
============================================
- Hits           9518     9512       -6     
- Misses         3404     3410       +6     
- Partials        630      632       +2
Impacted Files Coverage Δ Complexity Δ
...se/appengine/libraries/repository/MavenHelper.java 70.96% <100%> (+12.9%) 6 <0> (+2) ⬆️
...om/google/cloud/tools/eclipse/util/MavenUtils.java 57.69% <100%> (+1.92%) 16 <1> (+1) ⬆️
...se/appengine/libraries/model/MavenCoordinates.java 96.72% <100%> (+0.11%) 10 <0> (ø) ⬇️
...aflow/ui/handler/ModifyDataflowVersionHandler.java 0% <0%> (-20.84%) 0% <0%> (-2%)
...ipse/dataflow/core/project/DataflowMavenModel.java 54.16% <0%> (-9.73%) 9% <0%> (ø)
...ow/ui/preferences/RunOptionsDefaultsComponent.java 78.28% <0%> (-0.62%) 60% <0%> (-2%)
...calAppEngineServerLaunchConfigurationDelegate.java 64.5% <0%> (+0.3%) 39% <0%> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a6ee29a...74d9aba. Read the comment docs.

@elharo elharo changed the title Test Maven Resolution Check Android repository too Feb 14, 2020
@briandealwis
Copy link
Member

These errors look suspiciously like #3180. I suspect #3095 may play into it all too.

I looked briefly at how m2e's MavenPomEditor's Dependency Hierarchy page resolves items. It calls code somewhat like the following:

String classpath = Artifact.SCOPE_RUNTIME_PLUS_SYSTEM;
IMavenProjectFacade facade = MavenPlugin.getMavenProjectRegistry().getProject(pomFile.getProject());

      DependencyNode root = MavenPlugin.getMavenModelManager().readDependencyTree(facade, mavenProject, classpath)

@elharo
Copy link
Contributor Author

elharo commented Feb 14, 2020

I think #3180 and #3095 are symptoms, not causes. In all cases what we're seeing is a failure to find an artifact in Maven central. However there are different reasons that might be so. In this case the reason is simply that we're not checking the android repository.

I'm now inclined to go ahead with this PR since it does fix the problem. There might be a better way, but so far I don't see it.

Copy link
Contributor Author

@elharo elharo left a comment

Choose a reason for hiding this comment

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

PTAL. I came up with a less invasive hack that looks for androidx (and only androidx) artifacts in the Google repo.

@elharo
Copy link
Contributor Author

elharo commented Feb 14, 2020

That didn't work (though it did locally). I May have to back it out.

Copy link
Contributor Author

@elharo elharo left a comment

Choose a reason for hiding this comment

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

Build failures were jsut an unused import I have now removed. Please review.

Copy link
Member

@briandealwis briandealwis left a comment

Choose a reason for hiding this comment

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

At least this code is only intended for our libraries.

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.

3 participants