Uses common lib for Cloud SDK detection. #335

joaoandremartins · 2016-07-08T20:41:23Z

googlebot · 2016-07-08T20:41:24Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

chanseokoh · 2016-07-08T20:43:56Z

pom.xml

@@ -79,7 +79,7 @@
      <!-- bundle: com.google.cloud.tools.app.lib -->
      <groupId>com.google.cloud.tools</groupId>
      <artifactId>appengine-plugins-core</artifactId>
-      <version>0.1.1-SNAPSHOT</version>
+      <version>0.1.2-SNAPSHOT</version>


#332 was just merged.

elharo · 2016-07-11T13:25:50Z

Please resolve conflicts and repush.

googlebot · 2016-07-11T15:48:05Z

CLAs look good, thanks!

joaoandremartins · 2016-07-11T15:51:45Z

@elharo I think the conflicts are resolved. Let me know if you want me to walk you through what I've done so far. Might be faster that way!

elharo · 2016-07-11T15:53:21Z

...c/com/google/cloud/tools/eclipse/appengine/localserver/ui/CloudSdkRuntimeWizardFragment.java

@@ -146,7 +146,7 @@ public void widgetSelected(SelectionEvent event) {
      }
    });

-    File location = new CloudSdkProvider(null).getCloudSdkLocation();
+    File location = new CloudSdkProvider().getCloudSdk().getJavaAppEngineSdkPath().toFile();


Why do we need toFile here? If getJavaAppEngineSdkPath returns a Path we can just declare location to be that instead, I expect.

Shouldn't this be the CLOUD_SDK instance?

Because there is already a Path class imported, this was my shortcut to having less readable java.nio.file.Path. I'll change to Path, though.

@briandealwis because we're only using this once in this class, I didn't factor this to a static variable. Let me know if there is anything I'm missing.

briandealwis · 2016-07-11T17:42:54Z

@joaoandremartins my thought was that we should ideally make the Cloud SDK discovery more pluggable, so this preference-based code can be just another resolver that complements the existing PATH and fixed-location resolvers. Perhaps configured with the JDK's IServiceLocator.

I don't think we should treat the resolution as being a cheap. Certainly doing lots of filesystem checks in the PATH isn't that cheap for non-SSD, and many corporate developers are still on underpowered hardware.

briandealwis · 2016-07-11T17:44:09Z

...cets/src/com/google/cloud/tools/eclipse/appengine/facets/AppEngineSdkClasspathContainer.java

-  // TODO should be changed once app-tools-lib can provide the directory
-  // https://github.com/GoogleCloudPlatform/app-tools-lib-for-java/issues/134
-  private static final String SDK_JAR = "platform/google_appengine/google/appengine/tools/java/lib/appengine-tools-api.jar";
+  private static final CloudSdk CLOUD_SDK = new CloudSdkProvider().getCloudSdk();


This shouldn't be a static. Actually, AppEngineSdkClasspathContainer should accept being provided with a CloudSdk instance.

Removed static.

I suspect you want to pass in a CloudSdk instance so we always get the freshest Cloud SDK we have. In that case, could it make sense to just call CloudSdkProvider every time we want it? Or are you worried this will incur in unnecessary FS checks?

elharo · 2016-07-12T11:56:47Z

Please put this on hold for the moment. It's starting to look like we may need to back out some of the changes this depends on first, for reasons unrelated to this PR.

elharo · 2016-07-13T12:20:11Z

This can move forward again. We've resolved the issues that were blocking it. Though I'm afraid you have to merge again. :-(

briandealwis · 2016-07-13T16:34:48Z

....google.cloud.tools.eclipse.sdk/src/com/google/cloud/tools/eclipse/sdk/CloudSdkProvider.java

+    // Let's use the configured path, if there is one. Otherwise, the lib will try to discover the
+    // path.
+    if (configuredPath != null && !configuredPath.isEmpty()) {
+    	// TODO(joaomartins): What happens when the provided path is invalid? Tools lib isn't


We allow it in case the SDK distribution has changed so the validate() code is now out-of-date. We show a warning in the preference page.

briandealwis · 2016-07-13T16:47:06Z

CloudSdkProvider was only needed because the Cloud SDK resolving mechanism is not sufficiently flexible. We could happily get rid of CloudSdkProvider entirely if we could instead configure the CloudSdk.Builder with an alternative PathResolver-like instance that knew to first look at the relevant Eclipse preference. We could do this via java.util.ServiceLoader, for example.

I don't think we can completely get rid of CloudSdkPrompter: I don't think it's safe to push UI-prompting into the path resolving code since we don't really know the UI situation.

chanseokoh · 2016-07-13T19:03:33Z

...ols.eclipse.sdk/src/com/google/cloud/tools/eclipse/sdk/internal/CloudSdkContextFunction.java

@@ -61,11 +61,9 @@ public Object compute(IEclipseContext context, String contextKey) {

    Object path = context.get(PreferenceConstants.CLOUDSDK_PATH);
    File location = toFile(path);


location is no longer used. Remove it?

I don't think location should be unused, I think there's value in being able to specify an alternative path this way — it certainly doesn't hurt.

The background here is that context functions are only re-evaluated (which we'll want to do if the preference changed) when some input used by the function is changed. We trigger the re-evaluation here by retrieving the SDK Path from the referencing contexts, and then setting the SDK Path on those contexts when the preference changes (see further above to the referencedContexts, which is used by the Activator's IPreferenceChangeListener and CloudSdkContextFunction#sdkPathChanged()).

That said, there's a logic error here in that a path value explicitly set in the context will be overwritten when the preference is next changed, and compute() should be:

Object path = context.get(PreferenceConstants.CLOUDSDK_PATH); if(path == null) { // record this context as using the preference value referencedContexts.add(context); }

But if you strongly believe there should only be a single SDK Path, then we should use a different context key name than PreferenceConstants.CLOUDSDK_PATH, like "sdkPathChanged".

CloudSdkProvider now takes an additional location that can come from the context. If there is no context location, it uses the one in the preference store. If none there either, it then delegates SDK discovery to the library. If the library can't find it, then null is returned and later on the user is prompted to set a valid one.

elharo · 2016-07-14T16:33:16Z

...cets/src/com/google/cloud/tools/eclipse/appengine/facets/AppEngineSdkClasspathContainer.java

-      if (jarFile.exists()) {
-        String appEngineToolsApiJar = jarFile.getPath();
+    CloudSdk cloudSdk = new CloudSdkProvider().getCloudSdk();
+    if (cloudSdk.getJavaAppEngineSdkPath() != null) {


This is weird. You're checking to see if getJavaAppEngineSdkPath is null but then calling a different method.

Yes, I fixed this locally but didn't push yet.

briandealwis · 2016-07-15T14:24:19Z

...ols.eclipse.sdk.ui/src/com/google/cloud/tools/eclipse/sdk/ui/preferences/messages.properties

@@ -6,5 +6,5 @@ CloudSdkPreferencePage_4=Cannot launch a browser
 CloudSdkPreferencePage_5=&SDK location:
 CloudSdkPreferencePage_6=No SDK found: {0}
 CloudSdkPrompter_0=Google Cloud SDK Not Configured
-CloudSdkPrompter_1=The Google Cloud SDK cannot be found\!  Would you like to configure it now?
+CloudSdkPrompter_1=Invalid or no Google Cloud SDK specified. Do you want to configure Google Cloud SDK now?


I don't think this is an improvement: the first part sounds awkward.

I wanted a message that enforces the user must either specify a valid Cloud SDK or the operation will fail, which the first message didn't convey very well IMO.
I agree the first part doesn't sound great, feel free to provide an alternative.

How about:

The Google Cloud SDK is required but cannot be found! Would you like to configure it now?

briandealwis · 2016-07-16T03:13:07Z

LGTM

joaoandremartins · 2016-07-18T14:24:08Z

Waiting for changes from GoogleCloudPlatform/appengine-plugins#164 to be pushed to Maven central so tests can succeed.

joaoandremartins · 2016-07-18T20:44:18Z

...t/src/com/google/cloud/tools/eclipse/appengine/localserver/ServletClasspathProviderTest.java

+import com.google.cloud.tools.appengine.cloudsdk.CloudSdk;
+import com.google.cloud.tools.eclipse.sdk.CloudSdkProvider;
+
+@RunWith(MockitoJUnitRunner.class)


@elharo @briandealwis PTAL. I made a few more changes so the unit/integration tests pass. After these changes, I think this is ready for submission.

fixes.

elharo · 2016-07-19T13:07:15Z

...e.deploy/src/com/google/cloud/tools/eclipse/appengine/deploy/standard/StandardDeployJob.java

@@ -28,7 +27,8 @@
 *  <li>stage project for deploy</li>
 *  <li>deploy staged project</li>
 * </ol>
- * It uses a work directory where it will create separate directory for the exploded WAR and the staging results.
+ * It uses a work directory where it will create separate directory for the exploded WAR and the


separate directories

elharo · 2016-07-19T13:08:45Z

LGTM

googlebot added the cla: no label Jul 8, 2016

chanseokoh reviewed Jul 8, 2016
View reviewed changes

elharo self-assigned this Jul 11, 2016

joaoandremartins force-pushed the get-tools-jar branch from 8a2b1ac to 466e38e Compare July 11, 2016 15:48

googlebot added cla: yes and removed cla: no labels Jul 11, 2016

elharo reviewed Jul 11, 2016
View reviewed changes

briandealwis reviewed Jul 11, 2016
View reviewed changes

joaoandremartins force-pushed the get-tools-jar branch from 466e38e to ffac7bd Compare July 11, 2016 20:39

elharo added the blocked label Jul 12, 2016

elharo removed the blocked label Jul 12, 2016

briandealwis reviewed Jul 13, 2016
View reviewed changes

chanseokoh reviewed Jul 13, 2016
View reviewed changes

elharo reviewed Jul 14, 2016
View reviewed changes

briandealwis mentioned this pull request Jul 15, 2016

Remove CloudSdkProvider in favour of new CloudSdkResolver strategy #362

Merged

briandealwis reviewed Jul 15, 2016
View reviewed changes

joaoandremartins self-assigned this Jul 15, 2016

joaoandremartins force-pushed the get-tools-jar branch from 1ae8769 to 54f934a Compare July 15, 2016 21:56

briandealwis mentioned this pull request Jul 16, 2016

Allow configuring other strategies for finding the Google Cloud SDK. GoogleCloudPlatform/appengine-plugins#168

Merged

joaoandremartins reviewed Jul 18, 2016
View reviewed changes

joaoandremartins added 10 commits July 18, 2016 17:08

Uses common lib for Cloud SDK detection.

c90334c

Some style changes and fixed an issue with SDK path.

2f1265c

Uses SDK location from the context, in addition to preference store.

2706aeb

Fixes a small bug with empty configuredPath.

e649bba

Several small fixes.

8a02ca8

Makes the user specify a valid Cloud SDK before proceeding. Other small

b601915

fixes.

Makes CloudSdkProvider not inherit and fixes getCloudSdk() Javadoc.

15d58b6

Tweaks Cloud SDK not found message.

1884c5b

Fixed unit tests.

59fbac1

Removed comment.

29c191c

joaoandremartins force-pushed the get-tools-jar branch from 9ab01c3 to 29c191c Compare July 18, 2016 21:10

Fixes errors from later commit.

4859a12

elharo reviewed Jul 19, 2016
View reviewed changes

Fix Javadoc nit.

fd354ef

joaoandremartins merged commit b2bff4a into master Jul 19, 2016

joaoandremartins deleted the get-tools-jar branch July 19, 2016 13:39

briandealwis mentioned this pull request Jul 22, 2016

Path -> File --> Path Cleanup #381

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uses common lib for Cloud SDK detection. #335

Uses common lib for Cloud SDK detection. #335

joaoandremartins commented Jul 8, 2016

googlebot commented Jul 8, 2016

chanseokoh Jul 8, 2016

elharo commented Jul 11, 2016

googlebot commented Jul 11, 2016

joaoandremartins commented Jul 11, 2016

elharo Jul 11, 2016

briandealwis Jul 11, 2016

joaoandremartins Jul 11, 2016

briandealwis commented Jul 11, 2016

briandealwis Jul 11, 2016

joaoandremartins Jul 11, 2016

elharo commented Jul 12, 2016 •

edited

Loading

elharo commented Jul 13, 2016

briandealwis Jul 13, 2016

briandealwis commented Jul 13, 2016

chanseokoh Jul 13, 2016

briandealwis Jul 13, 2016

joaoandremartins Jul 13, 2016

elharo Jul 14, 2016

joaoandremartins Jul 14, 2016

briandealwis Jul 15, 2016

joaoandremartins Jul 15, 2016

akerekes Jul 15, 2016

briandealwis commented Jul 16, 2016

joaoandremartins commented Jul 18, 2016

joaoandremartins Jul 18, 2016

elharo Jul 19, 2016

elharo commented Jul 19, 2016

		@@ -61,11 +61,9 @@ public Object compute(IEclipseContext context, String contextKey) {

		Object path = context.get(PreferenceConstants.CLOUDSDK_PATH);
		File location = toFile(path);

Uses common lib for Cloud SDK detection. #335

Uses common lib for Cloud SDK detection. #335

Conversation

joaoandremartins commented Jul 8, 2016

googlebot commented Jul 8, 2016

Choose a reason for hiding this comment

elharo commented Jul 11, 2016

googlebot commented Jul 11, 2016

joaoandremartins commented Jul 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

briandealwis commented Jul 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elharo commented Jul 12, 2016 • edited Loading

elharo commented Jul 13, 2016

Choose a reason for hiding this comment

briandealwis commented Jul 13, 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

briandealwis commented Jul 16, 2016

joaoandremartins commented Jul 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elharo commented Jul 19, 2016

elharo commented Jul 12, 2016 •

edited

Loading