Skip to content

Allows to run test on Windows#403

Merged
binji merged 1 commit intoWebAssembly:masterfrom
Cellule:windows_test
Dec 20, 2016
Merged

Allows to run test on Windows#403
binji merged 1 commit intoWebAssembly:masterfrom
Cellule:windows_test

Conversation

@Cellule
Copy link
Contributor

@Cellule Cellule commented Dec 16, 2016

  • Use wasm.exe instead of ./wasm to run tests on Windows
  • wasm.exe doesn't recognize paths with \\, which is the default separator on Windows
  • Windows doesn't recognize arguments wrapped using 'argument', it needs to use "argument"

class RunTests(unittest.TestCase):
def _runCommand(self, command, logPath, expectedExitCode = 0):
if isWindows:
command = command.replace('\'', '"').replace('\\', '/')
Copy link
Member

Choose a reason for hiding this comment

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

I'm surprised this is needed, shouldn't os.path.join take care of this?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, sorry, now I get it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah the underlying problem is that the interpreter doesn't recognize \ in paths, but I wanted to make a quick fix for the time being, I might look into this later on.

Copy link
Member

Choose a reason for hiding this comment

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

I think the problem was merely insufficient quotation in main.ml. I just pushed a quick fix:

6aa5c27

Can you rebase and see whether that makes this line unnecessary? Then the isWindows flag might be unnecessary as well.


parser = argparse.ArgumentParser()
parser.add_argument('--wasm', metavar='<wasm-command>', default='./wasm')
parser.add_argument('--wasm', metavar='<wasm-command>', default=('./wasm' if not isWindows else 'wasm.exe'))
Copy link
Member

Choose a reason for hiding this comment

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

Would os.path.join(".", "wasm") work?

Copy link
Contributor

Choose a reason for hiding this comment

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

One has .exe extension, the other doesn't.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, but that's not needed when used as a command, is it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

os.path.join('.', 'wasm') results in ./wasm even on Windows which is not valid.
However, I found that os.path.join(os.getcwd(), 'wasm') is a valid command

@rossberg
Copy link
Member

lgtm

@Cellule
Copy link
Contributor Author

Cellule commented Dec 17, 2016

I was able to remove the .replace("\\", "/") with the latest fix.
However, the single quote was a problem in that script only. On Windows the command wasm 'my path' passes the argument 'my path' to the program instead of my path (no quotes)

@Cellule
Copy link
Contributor Author

Cellule commented Dec 20, 2016

I don't have merge rights on this repo, can someone merge this for me please?

@binji binji merged commit b055d01 into WebAssembly:master Dec 20, 2016
@Cellule
Copy link
Contributor Author

Cellule commented Dec 20, 2016

Thanks

@Cellule Cellule deleted the windows_test branch December 20, 2016 22:23
dhil pushed a commit to dhil/webassembly-spec that referenced this pull request Oct 3, 2023
Make type setting of i8.pack and i16.pack less jarring
raoxiaojia pushed a commit to WasmCert/spec that referenced this pull request Apr 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants