-
Notifications
You must be signed in to change notification settings - Fork 26
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
Make getSdkPath and getJavaToolsJar public. #164
Changes from 5 commits
e56181d
0fff81e
6aba444
3b4754d
2e3db75
f2f298e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,9 +29,11 @@ | |
import com.google.common.collect.Maps; | ||
|
||
import java.io.File; | ||
import java.nio.file.Files; | ||
import java.nio.file.Path; | ||
import java.nio.file.Paths; | ||
import java.util.ArrayList; | ||
import java.util.HashMap; | ||
import java.util.List; | ||
import java.util.Map; | ||
import java.util.logging.Logger; | ||
|
@@ -52,6 +54,7 @@ public class CloudSdk { | |
"platform/google_appengine/google/appengine/tools/java/lib"; | ||
private static final String JAVA_TOOLS_JAR = "appengine-tools-api.jar"; | ||
private static final String WINDOWS_BUNDLED_PYTHON = "platform/bundledpython/python.exe"; | ||
private static final Map<String, Path> JAR_LOCATIONS = new HashMap<>(); | ||
|
||
private final Path sdkPath; | ||
private final ProcessRunner processRunner; | ||
|
@@ -93,6 +96,14 @@ private CloudSdk(Path sdkPath, String appCommandMetricsEnvironment, | |
this.processRunner = new DefaultProcessRunner(async, stdOutLineListeners, stdErrLineListeners, | ||
exitListeners, startListeners); | ||
|
||
// 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", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These jars are needed to setup projects in Eclipse, and to run local servers. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And on top of that, why provide this as a map? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's implemented as a map, but provided as API methods. Do you have any alternative suggestion? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it be difficult to get these jars from somewhere else? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
getJavaAppEngineSdkPath().resolve("shared/servlet-api.jar")); | ||
JAR_LOCATIONS.put("jsp-api.jar", getJavaAppEngineSdkPath().resolve("shared/jsp-api.jar")); | ||
JAR_LOCATIONS.put(JAVA_TOOLS_JAR, | ||
sdkPath.resolve(JAVA_APPENGINE_SDK_PATH).resolve(JAVA_TOOLS_JAR)); | ||
} | ||
|
||
/** | ||
|
@@ -162,7 +173,7 @@ public void runAppCfgCommand(List<String> args) throws ProcessRunnerException { | |
command.add( | ||
Paths.get(System.getProperty("java.home")).resolve("bin/java").toAbsolutePath().toString()); | ||
command.add("-cp"); | ||
command.add(getJavaToolsJar().toAbsolutePath().toString()); | ||
command.add(JAR_LOCATIONS.get(JAVA_TOOLS_JAR).toString()); | ||
command.add("com.google.appengine.tools.admin.AppCfg"); | ||
command.addAll(args); | ||
|
||
|
@@ -175,7 +186,7 @@ private void logCommand(List<String> command) { | |
logger.info("submitting command: " + WHITESPACE_JOINER.join(command)); | ||
} | ||
|
||
private Path getSdkPath() { | ||
public Path getSdkPath() { | ||
return sdkPath; | ||
} | ||
|
||
|
@@ -195,14 +206,20 @@ public Path getJavaAppEngineSdkPath() { | |
return getSdkPath().resolve(JAVA_APPENGINE_SDK_PATH); | ||
} | ||
|
||
private Path getJavaToolsJar() { | ||
return getJavaAppEngineSdkPath().resolve(JAVA_TOOLS_JAR); | ||
} | ||
|
||
private Path getWindowsPythonPath() { | ||
return getSdkPath().resolve(WINDOWS_BUNDLED_PYTHON); | ||
} | ||
|
||
/** | ||
* Gets the file system location for an SDK jar. | ||
* | ||
* @param jarName the jar file name. For example, "servlet-api.jar" | ||
* @return the path in the file system | ||
*/ | ||
public Path getJarPath(String jarName) { | ||
return JAR_LOCATIONS.get(jarName); | ||
} | ||
|
||
/** | ||
* Checks whether the configured Cloud SDK Path is valid. | ||
* | ||
|
@@ -212,26 +229,28 @@ public void validate() throws AppEngineException { | |
if (sdkPath == null) { | ||
throw new AppEngineException("Validation Error: SDK path is null"); | ||
} | ||
if (!sdkPath.toFile().isDirectory()) { | ||
if (!Files.isDirectory(sdkPath)) { | ||
throw new AppEngineException( | ||
"Validation Error: SDK directory '" + sdkPath + "' is not valid"); | ||
"Validation Error: SDK location '" + sdkPath + "' is a directory."); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is not a directory |
||
} | ||
if (!getGCloudPath().toFile().isFile()) { | ||
if (!Files.isRegularFile(getGCloudPath())) { | ||
throw new AppEngineException( | ||
"Validation Error: gcloud path '" + getGCloudPath() + "' is not valid"); | ||
"Validation Error: gcloud location '" + getGCloudPath() + "' is not a file."); | ||
} | ||
if (!getDevAppServerPath().toFile().isFile()) { | ||
if (!Files.isRegularFile(getDevAppServerPath())) { | ||
throw new AppEngineException( | ||
"Validation Error: dev_appserver.py path '" + getDevAppServerPath() + "' is not valid"); | ||
"Validation Error: dev_appserver.py location '" | ||
+ getDevAppServerPath() + "' is not a file."); | ||
} | ||
if (!getJavaAppEngineSdkPath().toFile().isDirectory()) { | ||
if (!Files.isDirectory(getJavaAppEngineSdkPath())) { | ||
throw new AppEngineException( | ||
"Validation Error: Java App Engine SDK path '" + getJavaAppEngineSdkPath() | ||
+ "' is not valid"); | ||
"Validation Error: Java App Engine SDK location '" | ||
+ getJavaAppEngineSdkPath() + "' is not a directory."); | ||
} | ||
if (!getJavaToolsJar().toFile().isFile()) { | ||
if (!Files.isRegularFile(JAR_LOCATIONS.get(JAVA_TOOLS_JAR))) { | ||
throw new AppEngineException( | ||
"Validation Error: Java Tools jar path '" + getJavaToolsJar() + "' is not valid"); | ||
"Validation Error: Java Tools jar location '" | ||
+ JAR_LOCATIONS.get(JAVA_TOOLS_JAR) + "' is not a file."); | ||
} | ||
} | ||
|
||
|
@@ -250,12 +269,13 @@ public static class Builder { | |
private int runDevAppServerWaitSeconds; | ||
|
||
/** | ||
* The home directory of Google Cloud SDK. If not set, will attempt to look for the SDK in known | ||
* install locations. | ||
* The home directory of Google Cloud SDK. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @param sdkPath the home ... |
||
* | ||
* @param sdkPath the root path for the Cloud SDK | ||
*/ | ||
public Builder sdkPath(File sdkPathFile) { | ||
if (sdkPathFile != null) { | ||
this.sdkPath = sdkPathFile.toPath(); | ||
public Builder sdkPath(Path sdkPath) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Api was made to expose only File, hence sdkPath(File...) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you suggesting any change is made here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Api to the library was accepting File instead of Path. Having this builder accept path affects that consistency. Having said that, it's not a really big deal. I would have liked this CL to be more constrained in it's purpose though. It really seems to have pushed a lot of things in that could have been discussed. |
||
if (sdkPath != null) { | ||
this.sdkPath = sdkPath; | ||
} | ||
return this; | ||
} | ||
|
@@ -352,6 +372,9 @@ public Builder runDevAppServerWait(int runDevAppServerWaitSeconds) { | |
|
||
/** | ||
* Create a new instance of {@link CloudSdk}. | ||
* | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. for the moment get rid of this blank line until we can configure checkstyle to ignore this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you need There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a tag at the new line beginning, let's see if it likes it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you run "mvn clean install" or "mvn checkstyle:check" locally, it will do the checkstyle checking. |
||
* <p>If {@code sdKPath} is not set, this method will look for the SDK in known install | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sdkPath |
||
* locations. | ||
*/ | ||
public CloudSdk build() { | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
package com.google.cloud.tools.appengine.cloudsdk; | ||
|
||
import static org.junit.Assert.assertEquals; | ||
|
||
import org.junit.Test; | ||
import org.junit.runner.RunWith; | ||
import org.mockito.runners.MockitoJUnitRunner; | ||
|
||
import java.nio.file.Path; | ||
import java.nio.file.Paths; | ||
|
||
/** | ||
* Unit tests for {@link CloudSdk} | ||
*/ | ||
@RunWith(MockitoJUnitRunner.class) | ||
public class CloudSdkTest { | ||
@Test | ||
public void testGetSdkPath() { | ||
Path location = Paths.get("/"); | ||
CloudSdk sdk = new CloudSdk.Builder().sdkPath(location).build(); | ||
assertEquals(location, sdk.getSdkPath()); | ||
} | ||
|
||
@Test | ||
public void testGetJavaAppEngineSdkPath() { | ||
Path location = Paths.get("/"); | ||
CloudSdk sdk = new CloudSdk.Builder().sdkPath(location).build(); | ||
assertEquals(location.resolve("platform/google_appengine/google/appengine/tools/java/lib"), | ||
sdk.getJavaAppEngineSdkPath()); | ||
} | ||
|
||
@Test | ||
public void testGetJarPathJavaTools() { | ||
Path location = Paths.get("/"); | ||
CloudSdk sdk = new CloudSdk.Builder().sdkPath(location).build(); | ||
assertEquals(Paths.get("/platform/google_appengine/google/appengine" | ||
+ "/tools/java/lib/appengine-tools-api.jar"), | ||
sdk.getJarPath("appengine-tools-api.jar")); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And that'll be true for GKE support too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joaoandremartins that's a good idea. Can you put that down as an issue on the repository?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we can piggyback on #165? I'll link the TODO I added there, and maybe even assign it to myself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the library we can't assume there's a user. Any asking of user needs to be done from the UI.