Skip to content

Check Android repository too#3605

Merged
elharo merged 7 commits into
masterfrom
android
Feb 14, 2020
Merged

Check Android repository too#3605
elharo merged 7 commits into
masterfrom
android

Conversation

@elharo

@elharo elharo commented Feb 13, 2020

Copy link
Copy Markdown
Contributor

@elharo elharo left a comment

Copy link
Copy Markdown
Contributor Author

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

codecov Bot commented Feb 13, 2020

Copy link
Copy Markdown

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
Copy Markdown
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

elharo commented Feb 14, 2020

Copy link
Copy Markdown
Contributor Author

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.

@elharo elharo left a comment

Copy link
Copy Markdown
Contributor Author

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

elharo commented Feb 14, 2020

Copy link
Copy Markdown
Contributor Author

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

@elharo elharo left a comment

Copy link
Copy Markdown
Contributor Author

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.

@briandealwis briandealwis left a comment

Copy link
Copy Markdown
Member

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