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

Update to commons-compress' changed BSN #3546

Merged
merged 1 commit into from
Nov 19, 2019
Merged

Update to commons-compress' changed BSN #3546

merged 1 commit into from
Nov 19, 2019

Conversation

briandealwis
Copy link
Member

Update our 3rd-party feature with commons-compress' changed OSGi symbolic name. Fortunately the package names haven't changed and so appengine-plugins-core/com.google.cloud.tools.appengine is unaffected.

Filed COMPRESS-498 about the name change, which seems to have been a mistake.

@codecov
Copy link

codecov bot commented Nov 19, 2019

Codecov Report

Merging #3546 into master will decrease coverage by 0.08%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #3546      +/-   ##
============================================
- Coverage     70.13%   70.04%   -0.09%     
  Complexity     2989     2989              
============================================
  Files           376      376              
  Lines         13586    13586              
  Branches       1600     1600              
============================================
- Hits           9528     9516      -12     
- Misses         3422     3434      +12     
  Partials        636      636
Impacted Files Coverage Δ Complexity Δ
...ibraries/LibraryClasspathContainerResolverJob.java 78.57% <0%> (-21.43%) 5% <0%> (-1%)
...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%> (ø)
.../tools/eclipse/test/util/project/ProjectUtils.java 69.1% <0%> (+1.68%) 39% <0%> (+1%) ⬆️

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 01866e4...bbbe9da. Read the comment docs.

Copy link
Contributor

@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.

I'm surprised the build didn't catch this. How did you notice this? Is there a verification we're missing somewhere?

@briandealwis briandealwis merged commit a871869 into master Nov 19, 2019
@briandealwis
Copy link
Member Author

The build was pulling org.apache.commons.compress 1.18 from Eclipse Orbit. But I'm not quite sure why the build/verify-feature-completeness didn't fail since appengine-plugins-core does say it wants org.apache.commons.compress.* packages with version ≥ 1.19.

@chanseokoh
Copy link
Contributor

Could it be that it uses the old name on Photon but a new name on recent Eclipse?

@briandealwis
Copy link
Member Author

Ah, figured out the verify-feature-completeness:

  1. verify-feature-completeness attempts to install our com.google.cloud.tools.eclipse.suite.feature feature using our build artifacts in gcp-repo/ and the eclipse/ide-target-platform as a proxy for the main Eclipse repositories.
  2. (Using eclipse/ide-target-platform results in significant speedups.)
  3. The ide-target-platform is assembled from the target platform and our specific Maven dependencies with OSGi metadata (from the root pom). So it includes the org.apache.commons.commons-compress bundle.
  4. The only actual consumer of commons-compress is appengine-plugins-core, which imports its requisite org.apache.commons.compress.* as packages.
  5. The installation succeeds since the org.apache.commons.commons-compress bundle is present in the ide-target-platform.

If I change the test to instead reference download.eclipse.org/releases/xxx directly then it fails as the only bundle providing org.apache.commons.compress.* packages is Orbit's org.apache.commons.compress version 1.18.

@briandealwis
Copy link
Member Author

briandealwis commented Nov 19, 2019

Our com.google.cloud.tools.eclipse.3rdparty.feature feature does include bundles that we require that aren't commonly available or that differ from Orbit, but we need to include them by bundle name rather than package imports. So we specified org.apache.commons.compress which worked until commons-compress changed its bundle name in 1.18. But Orbit's variant still has name org.apache.commons.compress and so that was considered satisfactory.

We could modify com.google.cloud.tools.eclipse.3rdparty.feature to specify required version numbers (1.19) which might have caught this situation. It wouldn't differentiate if Orbit provided 1.19, but maybe that doesn't actually matter since we just care about 1.19 regardless of its source.

@chanseokoh chanseokoh deleted the ccbsn branch December 6, 2019 20:45
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.

4 participants