add a BOM to our generated pom.xml files #2956

elharo · 2018-03-20T21:01:52Z

@chanseokoh fix #2923 for Maven projects

briandealwis · 2018-03-21T13:50:00Z

plugins/com.google.cloud.tools.eclipse.util/templates/appengine/pom.xml.flex.ftl

+      <dependency>
+        <groupId>com.google.cloud</groupId>
+        <artifactId>google-cloud</artifactId>
+        <version>${googleCloudJavaVersion}</version>


googleCloudJavaBomVersion?

briandealwis · 2018-03-21T13:50:06Z

...wproject.test/src/com/google/cloud/tools/eclipse/appengine/newproject/CodeTemplatesTest.java

+                  scopePresent = true;
+                }
+              }
+              Assert.assertTrue(scopePresent);


This would be simplified using XPath as in PomXmlValidator?

briandealwis · 2018-03-21T13:52:00Z

...engine.newproject/src/com/google/cloud/tools/eclipse/appengine/newproject/CodeTemplates.java

@@ -208,7 +214,7 @@ private static void createPomXml(IProject project, AppEngineProjectConfig config
    String sdkVersion = getCurrentVersion(
        "com.google.appengine", //$NON-NLS-1$
        "appengine-api-1.0-sdk", //$NON-NLS-1$
-        "1.9.62"); //$NON-NLS-1$
+        "1.9.63"); //$NON-NLS-1$


I think it's worth pulling these artifact constants (including the BOM artifact too) out into explicit constants rather than have them scattered in the code.

Makes sense. Can you file an issue for that?

codecov · 2018-03-21T14:25:33Z

Codecov Report

Merging #2956 into master will decrease coverage by 0.01%.
The diff coverage is 74.71%.

@@             Coverage Diff              @@
##             master    #2956      +/-   ##
============================================
- Coverage      69.2%   69.18%   -0.02%     
- Complexity     2648     2657       +9     
============================================
  Files           356      358       +2     
  Lines         12428    12482      +54     
  Branches       1474     1483       +9     
============================================
+ Hits           8601     8636      +35     
- Misses         3220     3233      +13     
- Partials        607      613       +6

Impacted Files	Coverage Δ	Complexity Δ
...oogle/cloud/tools/eclipse/util/CloudToolsInfo.java	`85.71% <ø> (ø)`	`7 <0> (ø)`	⬇️
...d/tools/eclipse/appengine/libraries/BuildPath.java	`71.79% <0%> (ø)`	`22 <0> (ø)`	⬇️
...ls/eclipse/appengine/newproject/CodeTemplates.java	`93.26% <100%> (+0.13%)`	`21 <1> (ø)`	⬇️
...se/appengine/libraries/Maven4NamespaceContext.java	`50% <50%> (ø)`	`2 <2> (?)`
...e/cloud/tools/eclipse/appengine/libraries/Bom.java	`70% <70%> (ø)`	`6 <6> (?)`
...e/cloud/tools/eclipse/appengine/libraries/Pom.java	`86% <79.59%> (-4.16%)`	`35 <15> (+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 86bd279...ac9cb01. Read the comment docs.

briandealwis · 2018-03-21T14:48:13Z

...ect.test/src/com/google/cloud/tools/eclipse/appengine/newproject/Maven4NamespaceContext.java

+ * Maps all prefixes to http://maven.apache.org/POM/4.0.0 and
+ * all namespace URIs to "m". Not suitable for general use.
+ */
+class Maven4NamespaceContext implements NamespaceContext {


You can reuse com.google.cloud.tools.eclipse.appengine.validation.MavenContext

I'd forgotten about that class. Looking at it now, it has the same issues this class does. That is, it works only for very limited code and is actively broken for all other uses. Given that, I don't feels safe exposing it as part of the public API. We could look for or build a more correct namespace context that fully satisfies the interface's contract.

chanseokoh · 2018-03-21T14:51:07Z

Trying to understand the direction of this BOM stuff. So, with a BOM, I believe eventually we will get rid of the <version> element of Cloud libraries we add to pom.xml through the library page, right? Just checking, as this isn't happening in this PR.

elharo · 2018-03-21T15:29:33Z

With the BOM we can remove explicit versions from the google-cloud-java dependencies. However this may require splitting our library addition code into two: one for google-cloud-java dependencies and one for everything else.

elharo · 2018-03-22T00:28:18Z

There's a bug where our code is expecting there to be only one dependencies element but gets confused when there's more than one in the document:

  <dependencyManagement>
    <dependencies>
      <dependency>
        <groupId>com.google.cloud</groupId>
        <artifactId>google-cloud</artifactId>
        <version>0.40.0-alpha</version>
        <type>pom</type>
        <scope>import</scope>
      </dependency>
    <dependency>
<groupId>com.google.endpoints</groupId>
<artifactId>endpoints-framework</artifactId>
<version>2.0.13</version>
</dependency>
<dependency>
<groupId>com.googlecode.objectify</groupId>
<artifactId>objectify</artifactId>
<version>5.1.22</version>
</dependency>
</dependencies>
  </dependencyManagement>

I'm surprised tests didn't catch this.

elharo

FYI, this PR now fixes bugs that occur when adding libraries to any existing pom.xml files that have a dependencyManagement element.
It's not only about adding bom support.

elharo

Ping. This is now ready for review.

chanseokoh · 2018-03-23T15:14:25Z

...appengine.libraries.test/src/com/google/cloud/tools/eclipse/appengine/libraries/BomTest.java

@@ -0,0 +1,89 @@
+/*
+ * Copyright 2018 Google LLC.


No period after "LLC".

chanseokoh · 2018-03-23T15:24:41Z

...appengine.libraries.test/src/com/google/cloud/tools/eclipse/appengine/libraries/PomTest.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2017 Google Inc.
+ * Copyright 2017 Google LLC. All Rights Reserved.


Should end with "LLC" with no period.

chanseokoh · 2018-03-23T15:29:55Z

...appengine.libraries.test/src/com/google/cloud/tools/eclipse/appengine/libraries/BomTest.java

+
+    Node rootElement = doc.getDocumentElement();
+
+    dependencyManager =


Is this dependencyManager or dependencyManagement? (I'm confused.)

management, done

chanseokoh · 2018-03-23T15:30:09Z

plugins/com.google.cloud.tools.eclipse.appengine.libraries.test/testdata/testpom.xml

@@ -13,6 +13,18 @@
  <artifactId>com.google.cloud.tools.eclipse.appengine.libraries.test</artifactId>
  <version>0.1.0-SNAPSHOT</version>
  <packaging>eclipse-test-plugin</packaging>
+
+  <dependencyManagement>


Is this dependencyManager or dependencyManagement? (I'm confused.)

dependencyManagement

chanseokoh · 2018-03-23T15:32:31Z

....eclipse.appengine.libraries/src/com/google/cloud/tools/eclipse/appengine/libraries/Bom.java

+
+/**
+ * 
+ */


chanseokoh · 2018-03-23T15:35:12Z

....eclipse.appengine.libraries/src/com/google/cloud/tools/eclipse/appengine/libraries/Bom.java

+
+  private final static XPathFactory factory = XPathFactory.newInstance();
+
+  private static Set<String> artifacts; 


How about artifactIds?

chanseokoh · 2018-03-23T15:40:45Z

....eclipse.appengine.libraries/src/com/google/cloud/tools/eclipse/appengine/libraries/Pom.java

+            "http://maven.apache.org/POM/4.0.0", "dependencies");
+      }
+    } catch (XPathExpressionException ex) {
+      IStatus status = StatusUtil.error(Pom.class, ex.getMessage(), ex);


This doesn't seem it will happen. RuntimeException?

since we can return a status here, that's safer. But your'e right, it shouldn't happen.

chanseokoh · 2018-03-23T16:01:50Z

....eclipse.appengine.libraries/src/com/google/cloud/tools/eclipse/appengine/libraries/Bom.java

+      String bomArtifactId = (String) xpath.evaluate(
+          "string(./m:dependencies/m:dependency/m:artifactId)",
+          dependencyManager,
+          XPathConstants.STRING);


Let's say users include another 3rd-party BOM in addition to ours. I guess this code may not work, since it could return the group ID and artifact ID of the other 3rd-party POM?

Good point. Let me think about that.
FYI, right now in prod the code is broken with any dependencyManagement section including ours.

chanseokoh · 2018-03-23T16:05:46Z

....eclipse.appengine.libraries/src/com/google/cloud/tools/eclipse/appengine/libraries/Bom.java

+  static {
+    try {
+      List<CloudLibrary> cloudLibraries =
+          com.google.cloud.tools.libraries.CloudLibraries.getCloudLibraries();


To confirm: so we simply assume here our Cloud BOM includes all the libraries that appengine-plugins-core returns, and a future work is to read the actual content of the BOM?

briandealwis · 2018-03-23T15:43:36Z

...appengine.libraries.test/src/com/google/cloud/tools/eclipse/appengine/libraries/PomTest.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2017 Google Inc.
+ * Copyright 2017 Google LLC. All Rights Reserved.


briandealwis · 2018-03-23T15:46:25Z

...appengine.libraries.test/src/com/google/cloud/tools/eclipse/appengine/libraries/PomTest.java

-      NodeList dependencies = actual.getElementsByTagName("dependencies");
-      NodeList children = ((Element) dependencies.item(0)).getElementsByTagName("dependency");
-
-      Assert.assertEquals(2, children.getLength());


I think it's worth keeping asserting length == 2

briandealwis · 2018-03-23T15:47:07Z

...appengine.libraries.test/src/com/google/cloud/tools/eclipse/appengine/libraries/PomTest.java

@@ -286,7 +297,7 @@ public void testResolveLibraries() throws CoreException {
    Library library2 = newLibrary("id2", file1, file2);

    pom.updateDependencies(Arrays.asList(library1), Arrays.asList(library1, library2));
-
+    


Remove the introduced space

Google style is officially not to worry about trailing whitespace

briandealwis · 2018-03-23T15:53:03Z

...appengine.libraries.test/src/com/google/cloud/tools/eclipse/appengine/libraries/PomTest.java

-      Element dependencies = (Element) dependenciesList.item(0);
+      Assert.assertEquals(2, dependenciesList.getLength());
+
+      Element dependencies = (Element) dependenciesList.item(1);


Do you think it's worth a comment here saying item 0 is from the <dependencyManagement> section? (Or would it be clearer to rewrite this section using XPath too to ensure the depMgmt section is never seen?)

added comment

briandealwis · 2018-03-23T15:54:17Z

....eclipse.appengine.libraries/src/com/google/cloud/tools/eclipse/appengine/libraries/Bom.java

+import org.w3c.dom.Element;
+
+/**
+ * 


Class comment? This class might be better named to indicate that it's specific to the Google Cloud BOM. (Need to remember that users may use other BOMs, like the Spring BOM, and the bom project defines BOMs for all sorts of projects.)

I expect it's going to become more generic. For now it's package private.

briandealwis · 2018-03-23T15:54:46Z

....eclipse.appengine.libraries/src/com/google/cloud/tools/eclipse/appengine/libraries/Bom.java

+  static {
+    try {
+      List<CloudLibrary> cloudLibraries =
+          com.google.cloud.tools.libraries.CloudLibraries.getCloudLibraries();


Is there a reason for the FQCN?

We have a same-name class, so I think it's good to signify that this is from appengine-plugins-core.

briandealwis · 2018-03-23T16:06:34Z

....eclipse.appengine.libraries/src/com/google/cloud/tools/eclipse/appengine/libraries/Bom.java

+
+    try {
+      String bomGroupId = (String) xpath.evaluate(
+          "string(./m:dependencies/m:dependency/m:groupId)",


Wouldn't you want to get with type=pom and scope=import?

not necessary

briandealwis · 2018-03-23T16:36:32Z

...wproject.test/src/com/google/cloud/tools/eclipse/appengine/newproject/CodeTemplatesTest.java

+        XPathConstants.NODESET);
+    Assert.assertEquals(1, dependencyManagementNodes.getLength());
+
+    String bomGroupId = (String) xpath.evaluate(


Worth checking that there is a single <dependencies> element?

No, we're not. We're getting the first top-level dependencies element.

chanseokoh · 2018-03-23T18:49:51Z

Things seem to work fine. Only one minor problem:

"Duplicating managed version 4.12 for junit" in pom.xml. BOM seems to pull in the JUnit version too. Removing the <version> element clears the warning. Not sure why the BOM exposes a test dependency version. Will there be a way to exclude it?

elharo · 2018-03-23T20:59:32Z

The ultimate fix to the JUnit warning is to expand the dependency checking code to actually query the BOM. For now, though, it's just a low priority warning with no actual effect on the user's code.

patflynn · 2018-03-26T14:08:53Z

I don't see junit in the https://github.com/GoogleCloudPlatform/google-cloud-java/blob/master/google-cloud-bom/pom.xml (and wouldn't expect it there)

…

On Fri, Mar 23, 2018 at 6:53 PM Elliotte Rusty Harold < ***@***.***> wrote: Merged #2956 <#2956>. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#2956 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHf5HfYDXItT0d7qn2Wm87IraCiSVoR0ks5thXzvgaJpZM4SypUT> .

elharo · 2018-03-26T14:40:38Z

The BOM pulls in transitive dependencies, not just the immediate declarations. It's a lot broader and deeper than it looks.

chanseokoh · 2018-03-26T14:41:42Z

Yeah, it pulls in guava too. The <version> element is not needed when importing the BOM.

patflynn · 2018-03-26T14:52:04Z

Did you guys trace which dep is pulling in junit? We should file a bug.

…

On Mon, Mar 26, 2018 at 10:41 AM Chanseok Oh ***@***.***> wrote: Yeah, it pulls in guava too. The <version> element is not needed when importing the BOM. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#2956 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHf5Hdb6856jS5uLgNLmRQHelrgocZE4ks5tiP4mgaJpZM4SypUT> .

briandealwis · 2018-03-26T18:34:15Z

...appengine.libraries.test/src/com/google/cloud/tools/eclipse/appengine/libraries/BomTest.java

+    Element springDependency =
+        configureBom(doc, "org.springframework.boot", "spring-boot-dependencies", "2.0.0.RELEASE");
+    dependencies.appendChild(springDependency);
+    Element cloudDependency = configureBom(doc, "com.google.cloud", "google-cloud", "0.40.0-alpha");


Aha. This should be google-cloud-bom.

Did you guys trace which dep is pulling in junit? We should file a bug.

@patflynn turns out we were not pulling in a BOM.

Elliotte Harold added 2 commits March 20, 2018 17:00

add a pom to our generated pom.xml files

ba21bd9

add a pom to our generated pom.xml files

fa8fcdd

googlebot added the cla: yes label Mar 20, 2018

elharo changed the title ~~add a pom to our generated pom.xml files~~ add a BOM to our generated pom.xml files Mar 20, 2018

briandealwis reviewed Mar 21, 2018

View reviewed changes

Elliotte Harold added 2 commits March 21, 2018 09:53

fix up deps

c790e19

best version

4b9fef5

Elliotte Harold added 4 commits March 21, 2018 10:35

use xpath

2e9c983

use xpath

23dfbc8

javadoc

29347c5

googleCloudJavaBomVersion

9786405

briandealwis reviewed Mar 21, 2018

View reviewed changes

briandealwis mentioned this pull request Mar 21, 2018

Pull out artifact IDs into constants #2958

Open

Elliotte Harold added 2 commits March 21, 2018 12:37

Merge branch 'master' into bom

02eb29f

consider BOM when deciding whether to add version to dependency

a9393fd

pick correct dependencies element

31a4275

elharo commented Mar 22, 2018

View reviewed changes

elharo mentioned this pull request Mar 22, 2018

getElementsByTagNameNS and createElementNS #2960

Closed

Elliotte Harold added 2 commits March 22, 2018 16:34

use metadata

c681b6e

fix dependencyManaged

43dc5b3

elharo commented Mar 23, 2018

View reviewed changes

chanseokoh reviewed Mar 23, 2018

View reviewed changes

briandealwis reviewed Mar 23, 2018

View reviewed changes

Elliotte Harold added 2 commits March 23, 2018 13:06

Merge branch 'master' into bom

234a4a6

code review nits

144552e

remove FQCN

3122b9d

chanseokoh approved these changes Mar 23, 2018

View reviewed changes

Elliotte Harold added 2 commits March 23, 2018 17:22

handle multiple boms

eaa4315

better todo

ac9cb01

elharo merged commit 0ce011e into master Mar 23, 2018

elharo deleted the bom branch March 23, 2018 22:53

briandealwis reviewed Mar 26, 2018

View reviewed changes


		Node rootElement = doc.getDocumentElement();

		dependencyManager =


		private final static XPathFactory factory = XPathFactory.newInstance();

		private static Set<String> artifacts;

		@@ -286,7 +297,7 @@ public void testResolveLibraries() throws CoreException {
		Library library2 = newLibrary("id2", file1, file2);

		pom.updateDependencies(Arrays.asList(library1), Arrays.asList(library1, library2));

add a BOM to our generated pom.xml files #2956

add a BOM to our generated pom.xml files #2956

Conversation

elharo commented Mar 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 21, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chanseokoh commented Mar 21, 2018

elharo commented Mar 21, 2018 • edited Loading

elharo commented Mar 22, 2018

elharo left a comment • edited Loading

Choose a reason for hiding this comment

elharo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chanseokoh commented Mar 23, 2018

elharo commented Mar 23, 2018

patflynn commented Mar 26, 2018 via email

elharo commented Mar 26, 2018

chanseokoh commented Mar 26, 2018

patflynn commented Mar 26, 2018 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elharo commented Mar 20, 2018 •

edited

Loading

codecov bot commented Mar 21, 2018 •

edited

Loading

elharo commented Mar 21, 2018 •

edited

Loading

elharo left a comment •

edited

Loading