Skip to content

updates to support Windows 10 using linux subsystem #700

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

updates to support Windows 10 using linux subsystem #700

wants to merge 1 commit into from

Conversation

rjackson64840
Copy link

@rjackson64840 rjackson64840 commented Apr 2, 2018 •
edited by sync-by-unito bot
Loading

The following issues are corrected:

  • windows paths are injected into the bash scripts (fixed by converting to linux paths)
  • commands passed into bash on windows must be enclosed in quotes
  • Process.waitFor() not working as expected on Windows (with embedded Linux)
  • reading the output streams from bash running in the java process problematic, leading to the following adjustments:
    • process.waitFor() changed to process.waitFor(timeout) with suitable timeouts selected based on empirical data
    • workarounds for all the stream reads put in place

Also added additional helpful logging/output which will assist Windows users in troubleshooting.

Tested on Windows 10 using the Linux Subsystem for Windows. All of the localstack tests pass except for the SQS messaging tests.

┆Issue is synchronized with this Jira Bug by Unito

@rjackson64840
updates to support Windows; all localstack tests are passing in Windo…
2bacf38 
…ws except the SQS tests; tested using the Linux Subsystem for Windows on Windows 10.
whummer
whummer reviewed Apr 7, 2018
View reviewed changes
localstack/ext/java/src/main/java/cloud/localstack/LocalstackTestRunner.java
} catch (IOException e) {
throw new RuntimeException(e);
if (IS_WINDOWS) {
exec(false, "cd '" + getTmpInstallDir() + "/localstack/infra'; cat > " + INSTALLATION_FINISHED_MARKER);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious about this change - installationDoneMarker.createNewFile() does not work under Windows?

localstack/ext/java/src/main/java/cloud/localstack/LocalstackTestRunner.java
@@ -192,11 +237,18 @@ public static boolean isEnvConfigSet(String configName) {
}

private static String getEndpoint(String service) {
LOG.info(String.format("fetching endpoint for '%s'...", service));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we can make this a debug message instead of info? (same for line 242)

localstack/ext/java/src/main/java/cloud/localstack/LocalstackTestRunner.java
@@ -241,21 +301,84 @@ public void run() {
}
}

/**
* for Windows there are a several key differnces:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: differnces

localstack/ext/java/src/main/java/cloud/localstack/LocalstackTestRunner.java
if (cmd.length == 1 && !new File(cmd[0]).exists()) {
String commandArg = "\"" + cmd[0] + "; exit\"";
timeout = getProcessTimeout(commandArg);
LOG.info(String.format("executing (timeout = %ds) : bash -c %s", timeout, commandArg));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should also reduce the log level from info to debug here.

localstack/ext/java/src/main/java/cloud/localstack/LocalstackTestRunner.java
});
}
} else {
/* make sure we destroy the process on JVM shutdown */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: duplication with lines 291-296 - could be moved to a separate util method.

localstack/ext/java/src/main/java/cloud/localstack/LocalstackTestRunner.java
Process proc;
try {
proc = exec(false, cmd);
LOG.info("starting...");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps a more "verbose"/detailed message like:

LOG.info("Starting LocalStack infrastructure services from installation directory: " + getTmpInstallDir());

Then we can also remove line 381.

localstack/ext/java/src/main/java/cloud/localstack/LocalstackTestRunner.java
@@ -270,12 +393,39 @@ private void setupInfrastructure() {
throw new RuntimeException("Unable to start local infrastructure. Debug output: " + output);
}
INFRA_STARTED.set(proc);
LOG.info("started...");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps a more "verbose"/detailed message like:

LOG.info("Successfully started LocalStack infrastructure services.");

localstack/ext/java/src/main/java/cloud/localstack/ServiceName.java
}

static {
serviceMap.put(API_GATEWAY, "http://localhost:4567");
Copy link
Member

@whummer whummer Apr 7, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to avoid hardcoding the service endpoints here. Is there any chance we can make getEndpointOnWindows implement a logic similar to getEndpointOnLinux? Can you elaborate a bit why the existing getEndpoint(...) method doesn't work under Windows? Thanks

If the same approach (calling a python process that reads the configuration) does not work under Windows, another option to get the default service ports/endpoints would be to read the localstack_client config file from the installation directory - typically $TMPDIR/localstack_install_dir/.venv/lib/python*/site-packages/localstack_client/config.py (where python* is a placeholder for the concrete python version, for example python2.7 or python3.6).

The problem with using the default ports, though, is that this could lead to conflicts if users provide configuration settings that specify custom ports per service. Therefore, the dynamic port lookup (by calling the python process) would be the preferred option here.

@whummer
Copy link
Member

whummer commented Apr 7, 2018 •
edited
Loading

Thanks for this comprehensive PR @rjackson64840 - great changes! I left a few comments - most of them are minor issues. One thing we should try to figure out is how to not hardcode the ports/endpoints in the code. Thanks

@whummer
Copy link
Member

whummer commented May 20, 2018

Any update on this PR @rjackson64840 ? I think we're almost there, would be great to get these changes merged... thanks

@whummer
Copy link
Member

whummer commented Sep 2, 2018

Trying one more time. @rjackson64840 any chance we can rebase this PR onto latest master, and address the review comments above? Thanks

@whummer whummer force-pushed the master branch 2 times, most recently from 5200867 to 21a1db4 Compare April 6, 2021 15:35
@alexrashed
Copy link
Member

This PR is stale for quite a long time, a lot has changed since then. Please feel free to file a new PR or create an issue.

@alexrashed alexrashed closed this Jun 23, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@whummer whummer whummer left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rjackson64840 @whummer @alexrashed @silv-io