Make getSdkPath and getJavaToolsJar public. #164

joaoandremartins · 2016-07-13T21:40:47Z

getSdkPath should only be used for testing in GoogleCloudPlatform/google-cloud-eclipse#335

elharo · 2016-07-14T01:01:25Z

src/main/java/com/google/cloud/tools/appengine/cloudsdk/CloudSdk.java

@@ -174,7 +175,7 @@ private void logCommand(List<String> command) {
    logger.info("submitting command: " + WHITESPACE_JOINER.join(command));
  }

-  private Path getSdkPath() {
+  @VisibleForTesting public Path getSdkPath() {


why @VisibleForTesting? It seems useful and safe enough.

joaoandremartins · 2016-07-14T14:00:31Z

@elharo PTAL

elharo · 2016-07-14T14:27:55Z

Tests appear to be failing due to:

[INFO] --- maven-checkstyle-plugin:2.17:check (default) @ appengine-plugins-core ---
[WARNING] src/main/java/com/google/cloud/tools/appengine/cloudsdk/CloudSdk.java:354 JavadocParagraph: Empty line should be followed by

tag on the next line.

This shouldn't cuase a failure though.

briandealwis · 2016-07-14T14:28:21Z

src/main/java/com/google/cloud/tools/appengine/cloudsdk/CloudSdk.java

@@ -195,7 +195,7 @@ public Path getJavaAppEngineSdkPath() {
    return getSdkPath().resolve(JAVA_APPENGINE_SDK_PATH);
  }

-  private Path getJavaToolsJar() {
+  public Path getJavaToolsJar() {


I wonder if these type of methods always return a Collection<Path> or Path[] for future-proofing (what happens if AppEngine's tools is split into multiple jars)?

I think there are two approaches:

The caller should know where the particular jars are situated within the Cloud SDK, in which case we just need a resolve() type method. We're doing this in the Eclipse classpath providers, where we're pulling out runtime jars (servlet-api.jar and asp-api.jar).

The locations are masked by this library, and the caller instead requests them by purpose, like this method. Means we need to continue to modify and update this class as new libraries become included in the Cloud SDK.

Maybe the naming is wrong, but it's just a reference to the location of appcfg.

I think the actual expectation is that no one uses these. They really should be internal. Do you guys want to use this for something more than just testing?

I think it could be fair that the client has some knowledge of the SDK structure. The alternative would be to have a method to return each and every one of the SDK JARs, which might be a bit unsightly.
What do you guys think?

I would like to worry about the case where the SDK is split into different directories later, when we have to.

@loosebazooka it's being used for a few things in the Eclipse plugin.
Find shared/servlet-api.jar and shared/jsp-api.jar, display the current location on the text box, set it for internal use, check that the path exists (though this should really be being done at the library), etc.

My above response was actually for getJavaAppEngineSdkPath(). Eclipse is only using getJavaToolsJar() to add its location to the classpath IIUC.

joaoandremartins · 2016-07-15T15:55:25Z

@elharo @briandealwis PTAL. I added JAR location resolution and some unit tests.

elharo · 2016-07-15T16:14:41Z

src/main/java/com/google/cloud/tools/appengine/cloudsdk/CloudSdk.java

@@ -93,6 +95,12 @@ private CloudSdk(Path sdkPath, String appCommandMetricsEnvironment,
    this.processRunner = new DefaultProcessRunner(async, stdOutLineListeners, stdErrLineListeners,
        exitListeners, startListeners);

+    // Populate jar locations.


something I just discovered: not all google cloud SDK installs have this. The app-engine-java component needs to be explicitly installed.

Not for this PR, but we need to think about what happens if these paths aren't present.

And that'll be true for GKE support too.

Added a TODO. We should probably just detect if the Java SDK is installed and ask the user to install it if it isn't.

@joaoandremartins that's a good idea. Can you put that down as an issue on the repository?

I wonder if we can piggyback on #165? I'll link the TODO I added there, and maybe even assign it to myself.

In the library we can't assume there's a user. Any asking of user needs to be done from the UI.

elharo · 2016-07-15T16:14:51Z

LGTM

elharo · 2016-07-15T16:21:11Z

src/main/java/com/google/cloud/tools/appengine/cloudsdk/CloudSdk.java

      throw new AppEngineException(
-          "Validation Error: Java Tools jar path '" + getJavaToolsJar() + "' is not valid");
+          "Validation Error: Java Tools jar path '"
+                  + JAR_LOCATIONS.get(JAVA_TOOLS_JAR) + "' is not valid");


we should have a more specific message explaining why the location is invalid

Files.isDirectory()/isRegularFile().

elharo · 2016-07-15T18:48:10Z

src/main/java/com/google/cloud/tools/appengine/cloudsdk/CloudSdk.java

      throw new AppEngineException(
-          "Validation Error: SDK directory '" + sdkPath + "' is not valid");
+          "Validation Error: SDK location '" + sdkPath + "' is a directory.");


is not a directory

loosebazooka · 2016-07-15T19:02:33Z

src/main/java/com/google/cloud/tools/appengine/cloudsdk/CloudSdk.java

+    // Populate jar locations.
+    // TODO(joaomartins): Consider case where SDK doesn't contain these jars. Only App Engine
+    // SDK does.
+    JAR_LOCATIONS.put("servlet-api.jar",


I know this has been submitted, but there is no guarantee these jars will exist in the future in the cloud SDK. What is the need for these?

This CL seems to be modifying more than is intended in it's title.

These jars are needed to setup projects in Eclipse, and to run local servers.

And on top of that, why provide this as a map?

It's implemented as a map, but provided as API methods. Do you have any alternative suggestion?

It's dangerous to have this dependency, api tools jar was purely for running staging and nothing else. The appengine sdk dependency may or may not exist in the future, we expect a standalone staging command to replace going into the java SDK here. I would generally avoid depending on anything in the java SDK included in the cloud sdk.

Would it be difficult to get these jars from somewhere else?

The tools jar is used in https://github.com/GoogleCloudPlatform/gcloud-eclipse-tools/blob/master/plugins/com.google.cloud.tools.eclipse.appengine.facets/src/com/google/cloud/tools/eclipse/appengine/facets/AppEngineSdkClasspathContainer.java#L40. I just assumed it was needed and bundled the change due to the same requirement in https://github.com/GoogleCloudPlatform/gcloud-eclipse-tools/blob/master/plugins/com.google.cloud.tools.eclipse.appengine.localserver/src/com/google/cloud/tools/eclipse/appengine/localserver/ServletClasspathProvider.java#L39.

If the jars are in fact needed, then I think they should come from the SDK, hence the library should give them.

Although I agree that any functionality should probably be provided through an API, not JAR imports, that is a separate conversation to this PR.

joaoandremartins · 2016-07-15T19:54:45Z

Paths were already used over Files on the proof-of-concept implementation, at first, as per Patrick's suggestion and best practices.
I seem to recall having commented on the usage of File on your big PR, but maybe that comment fell through the cracks.

I couldn't make out any other changes to the existing API that caused disruption.

elharo · 2016-07-15T20:45:23Z

If we're going to run a local Java server, and if that is a function of gcloud (a critical function I would argue) then I don't see how it can be without servlet.jar and these other jars somewhere. If gcloud moves these, then this API gives us enough indirection to reflect the new location without changing clients.

joaoandremartins · 2016-07-15T20:56:20Z

The API let's you run a local devserver. https://github.com/GoogleCloudPlatform/app-tools-lib-for-java/blob/master/src/main/java/com/google/cloud/tools/appengine/cloudsdk/CloudSdkAppEngineDevServer.java#L53

loosebazooka · 2016-07-15T21:22:00Z

Is there somewhere you can point me to where you are using these jars?

elharo · 2016-07-16T12:36:15Z

We add these to the classpath when creating a new App Engine project in Eclipse, or adding the App Engine facet to an existing non-maven project.

They're also implicitly used when running a local development server.

joaoandremartins · 2016-07-18T14:21:52Z

@loosebazooka @meltsufin do we have to wait for a new release to upload new changes to Sonatype? GoogleCloudPlatform/google-cloud-eclipse#335 depends on this change and I can't submit it until these changes are published.

* Make getSdkPath public for testing. * Fixes an issue with resolving paths in CloudSdk and adds unit tests. * Replaced toFile() with Files.isDirectory()/isRegularFile().

… configuration based on dev appserver 1 or 2-alpha. (#165) * WIP - Fixes #158 - dev appserver 1 stuff * javadoc and simpler code whether checking <services> parameter has been configured by the user * remove default value from appYamls, revert file->path change * unit tests for RunMojo * update appengine-plugins-core dependency to 0.3.0 * try to fix kokoro test * use custom ports to test run in integration test to avoid port clash on Kokoro * fix async run test as well for Kokoro * Fixes #158 - dev appserver 1 stuff * additional tests * add datastorePath to RunMojo * add javadoc to datastorePath * increase timeout to see if it fixes the kokoro-ubuntu build * reformat pom.xml dependency for junitparams, rename SupportedVersion to SupportedDevServerVersion

* preserve datastore-indexes-auto across non-clean rebuilds

googlebot added the cla: yes label Jul 13, 2016

joaoandremartins mentioned this pull request Jul 13, 2016

Uses common lib for Cloud SDK detection. GoogleCloudPlatform/google-cloud-eclipse#335

Merged

elharo reviewed Jul 14, 2016
View reviewed changes

joaoandremartins added 2 commits July 14, 2016 10:03

Make getSdkPath public for testing.

e56181d

Removes VisibleForTesting and moves comment.

0fff81e

joaoandremartins force-pushed the tools-jar branch from b1ef731 to 0fff81e Compare July 14, 2016 14:04

briandealwis reviewed Jul 14, 2016
View reviewed changes

joaoandremartins added 2 commits July 14, 2016 11:17

Fixes several minor issues.

6aba444

Fixes an issue with resolving paths in CloudSdk and adds unit tests.

3b4754d

elharo reviewed Jul 15, 2016
View reviewed changes

Changed error messages. Replaced toFile() with

2e3db75

Files.isDirectory()/isRegularFile().

elharo reviewed Jul 15, 2016
View reviewed changes

Fixes message typos.

f2f298e

joaoandremartins merged commit 20c18cc into master Jul 15, 2016

joaoandremartins deleted the tools-jar branch July 15, 2016 18:58

loosebazooka reviewed Jul 15, 2016
View reviewed changes

JoeWang1127 pushed a commit that referenced this pull request Nov 29, 2023

preserve datastore-indexes-auto across non-clean rebuilds (#164)

f006d98

* preserve datastore-indexes-auto across non-clean rebuilds

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make getSdkPath and getJavaToolsJar public. #164

Make getSdkPath and getJavaToolsJar public. #164

joaoandremartins commented Jul 13, 2016 •

edited

Loading

elharo Jul 14, 2016

joaoandremartins commented Jul 14, 2016

elharo commented Jul 14, 2016

briandealwis Jul 14, 2016

loosebazooka Jul 14, 2016

joaoandremartins Jul 14, 2016

joaoandremartins Jul 14, 2016

joaoandremartins Jul 14, 2016

joaoandremartins Jul 14, 2016

joaoandremartins commented Jul 15, 2016

elharo Jul 15, 2016

briandealwis Jul 15, 2016

joaoandremartins Jul 15, 2016

loosebazooka Jul 15, 2016

joaoandremartins Jul 15, 2016

elharo Jul 15, 2016

elharo commented Jul 15, 2016

elharo Jul 15, 2016

elharo Jul 15, 2016

loosebazooka Jul 15, 2016

elharo Jul 15, 2016

loosebazooka Jul 15, 2016

joaoandremartins Jul 15, 2016

loosebazooka Jul 15, 2016

loosebazooka Jul 15, 2016

joaoandremartins Jul 15, 2016 •

edited

Loading

joaoandremartins commented Jul 15, 2016

elharo commented Jul 15, 2016

joaoandremartins commented Jul 15, 2016

loosebazooka commented Jul 15, 2016

elharo commented Jul 16, 2016

joaoandremartins commented Jul 18, 2016

Make getSdkPath and getJavaToolsJar public. #164

Make getSdkPath and getJavaToolsJar public. #164

Conversation

joaoandremartins commented Jul 13, 2016 • edited Loading

Choose a reason for hiding this comment

joaoandremartins commented Jul 14, 2016

elharo commented Jul 14, 2016

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

joaoandremartins commented Jul 15, 2016

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

elharo commented Jul 15, 2016

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

joaoandremartins Jul 15, 2016 • edited Loading

Choose a reason for hiding this comment

joaoandremartins commented Jul 15, 2016

elharo commented Jul 15, 2016

joaoandremartins commented Jul 15, 2016

loosebazooka commented Jul 15, 2016

elharo commented Jul 16, 2016

joaoandremartins commented Jul 18, 2016

joaoandremartins commented Jul 13, 2016 •

edited

Loading

joaoandremartins Jul 15, 2016 •

edited

Loading