Skip to content
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

Add an option to put WinAppDriver output in a file #151

Merged
merged 3 commits into from
Jun 25, 2024
Merged

Add an option to put WinAppDriver output in a file #151

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ae76c1e
Add an option to put WinAppDriver output in a file
Steelskin Jun 24, 2024
411f9df
Use parent handles in child process
Steelskin Jun 25, 2024
5279f86
Revert unintended indent
Steelskin Jun 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 60 additions & 15 deletions Sources/WinAppDriver/Win32ProcessTree.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,33 @@
import struct Foundation.TimeInterval
import WinSDK

/// Options for launching a process.
internal struct ProcessLaunchOptions {
/// Spawn a new console for the process.
public var spawnNewConsole: Bool = true
/// Redirect the process's stdout to the given handle.
public var stdoutHandle: HANDLE? = nil
/// Redirect the process's stderr to the given handle.
public var stderrHandle: HANDLE? = nil
/// Redirect the process's stdin to the given handle.
public var stdinHandle: HANDLE? = nil
}

/// Starts and tracks the lifetime of a process tree using Win32 APIs.
internal class Win32ProcessTree {
internal let jobHandle: HANDLE
internal let handle: HANDLE

init(path: String, args: [String]) throws {
init(path: String, args: [String], options: ProcessLaunchOptions = ProcessLaunchOptions())
throws {
// Use a job object to ensure that the process tree doesn't outlive us.
jobHandle = try Self.createJobObject()

let commandLine = buildCommandLineArgsString(args: [path] + args)
do { handle = try Self.createProcessInJob(commandLine: commandLine, jobHandle: jobHandle) }
catch {
do {
handle = try Self.createProcessInJob(
commandLine: commandLine, jobHandle: jobHandle, options: options)
} catch {
CloseHandle(jobHandle)
throw error
}
Expand Down Expand Up @@ -64,23 +79,53 @@ internal class Win32ProcessTree {
return jobHandle
}

private static func createProcessInJob(commandLine: String, jobHandle: HANDLE) throws -> HANDLE {
private static func createProcessInJob(
commandLine: String,
jobHandle: HANDLE,
options: ProcessLaunchOptions = ProcessLaunchOptions()
Steelskin marked this conversation as resolved.
Show resolved Hide resolved
) throws -> HANDLE {
try commandLine.withCString(encodedAs: UTF16.self) { commandLine throws in
var startupInfo = STARTUPINFOW()
startupInfo.cb = DWORD(MemoryLayout<STARTUPINFOW>.size)
var redirectStdHandle = false

let creationFlags =
DWORD(CREATE_SUSPENDED) | DWORD(CREATE_NEW_PROCESS_GROUP)
| (options.spawnNewConsole ? DWORD(CREATE_NEW_CONSOLE) : 0)
if let stdoutHandle = options.stdoutHandle {
startupInfo.hStdOutput = stdoutHandle
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion for more compact

startupInfo.hStdOutput = options.stdoutHandle ?? INVALID_HANDLE_VALUE
startupInfo.hStdError = options.stderrHandle ?? INVALID_HANDLE_VALUE
startupInfo.hStdInput = options.stdinHandle ?? INVALID_HANDLE_VALUE
let redirectStdHandle = options.stdoutHandle != INVALID_HANDLE_VALUE || options.stderrHandle != INVALID_HANDLE_VALUE || options.stdinHandle != INVALID_HANDLE_VALUE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should not set these handles to INVALID_HANDLE_VALUE because INVALID_HANDLE_VALUE (= -1 = 0xfff...) is not actually invalid but is the current process handle. So here, we use the current process input/output handles as a fallback, which makes the redirectStdHandle logic more complex than it should be.

GetStdHandle() will return NULL if the standard input/output devices have been closed, which is also a valid value to set the stdoutHandle option to if you don't want the child process to write anything, for instance. IIUC NULL HANDLE (=0) is a different value from Swift nil, since the stdoutHandle in the ProcessLaunchOptions struct is an optional.

redirectStdHandle = true
} else {
startupInfo.hStdOutput = INVALID_HANDLE_VALUE
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this preserve the old behavior if we only set some of the handles? Should we force callers to either override all handles, or none?

jeffdav marked this conversation as resolved.
Show resolved Hide resolved
}
if let stderrHandle = options.stderrHandle {
startupInfo.hStdError = stderrHandle
redirectStdHandle = true
} else {
startupInfo.hStdError = INVALID_HANDLE_VALUE
}
if let stdinHandle = options.stdinHandle {
startupInfo.hStdInput = stdinHandle
redirectStdHandle = true
} else {
startupInfo.hStdInput = INVALID_HANDLE_VALUE
}
if redirectStdHandle {
startupInfo.dwFlags |= DWORD(STARTF_USESTDHANDLES)
}

var processInfo = PROCESS_INFORMATION()
guard CreateProcessW(
nil,
UnsafeMutablePointer<WCHAR>(mutating: commandLine),
nil,
nil,
false,
DWORD(CREATE_NEW_CONSOLE) | DWORD(CREATE_SUSPENDED) | DWORD(CREATE_NEW_PROCESS_GROUP),
nil,
nil,
&startupInfo,
&processInfo
nil,
Steelskin marked this conversation as resolved.
Show resolved Hide resolved
UnsafeMutablePointer<WCHAR>(mutating: commandLine),
nil,
nil,
redirectStdHandle, // Inherit handles is necessary for redirects.
creationFlags,
nil,
nil,
&startupInfo,
&processInfo
) else {
throw Win32Error.getLastError(apiName: "CreateProcessW")
}
Expand All @@ -100,4 +145,4 @@ internal class Win32ProcessTree {
return processInfo.hProcess
}
}
}
}
70 changes: 65 additions & 5 deletions Sources/WinAppDriver/WinAppDriver.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,16 @@ public class WinAppDriver: WebDriver {

private let httpWebDriver: HTTPWebDriver
private let processTree: Win32ProcessTree?
/// The write end of a pipe that is connected to the child process's stdin.
private let childStdinHandle: HANDLE?

private init(httpWebDriver: HTTPWebDriver, processTree: Win32ProcessTree? = nil) {
private init(
httpWebDriver: HTTPWebDriver,
processTree: Win32ProcessTree? = nil,
childStdinHandle: HANDLE? = nil) {
self.httpWebDriver = httpWebDriver
self.processTree = processTree
self.childStdinHandle = childStdinHandle
}

public static func attach(ip: String = defaultIp, port: Int = defaultPort) -> WinAppDriver {
Expand All @@ -34,12 +40,60 @@ public class WinAppDriver: WebDriver {
executablePath: String = defaultExecutablePath,
ip: String = defaultIp,
port: Int = defaultPort,
waitTime: TimeInterval? = defaultStartWaitTime) throws -> WinAppDriver {

waitTime: TimeInterval? = defaultStartWaitTime,
outputFile: String? = nil) throws -> WinAppDriver {
let processTree: Win32ProcessTree
var childStdinHandle: HANDLE? = nil
do {
processTree = try Win32ProcessTree(path: executablePath, args: [ ip, String(port) ])
var launchOptions = ProcessLaunchOptions()
if let outputFile = outputFile {
// Open the output file for writing to the child stdout.
var securityAttributes = SECURITY_ATTRIBUTES()
securityAttributes.nLength = DWORD(MemoryLayout<SECURITY_ATTRIBUTES>.size)
securityAttributes.bInheritHandle = true
launchOptions.stdoutHandle = try outputFile.withCString(encodedAs: UTF16.self) {
outputFile throws in
Steelskin marked this conversation as resolved.
Show resolved Hide resolved
CreateFileW(
UnsafeMutablePointer<WCHAR>(mutating: outputFile), DWORD(GENERIC_WRITE),
DWORD(FILE_SHARE_READ), &securityAttributes,
DWORD(OPEN_ALWAYS), DWORD(FILE_ATTRIBUTE_NORMAL), nil)
}
if launchOptions.stdoutHandle == INVALID_HANDLE_VALUE {
// Failed to open the output file for writing.
throw Win32Error.getLastError(apiName: "CreateFileW")
}

// Use the same handle for stderr.
launchOptions.stderrHandle = launchOptions.stdoutHandle

// WinAppDriver will close immediately if no stdin is provided so create a dummy
// pipe here to keep stdin open until the child process is closed.
var childReadInputHandle: HANDLE?
if !CreatePipe(&childReadInputHandle, &childStdinHandle, &securityAttributes, 0) {
CloseHandle(launchOptions.stdoutHandle)
Steelskin marked this conversation as resolved.
Show resolved Hide resolved
throw Win32Error.getLastError(apiName: "CreatePipe")
}
launchOptions.stdinHandle = childReadInputHandle

// Also use the parent console to stop spurious new consoles from spawning.
launchOptions.spawnNewConsole = false

}

// Close our handles when the process has launched. The child process keeps a copy.
defer {
Steelskin marked this conversation as resolved.
Show resolved Hide resolved
if let handle = launchOptions.stdoutHandle {
CloseHandle(handle)
}
if let handle = launchOptions.stdinHandle {
CloseHandle(handle)
}
}

processTree = try Win32ProcessTree(
path: executablePath, args: [ip, String(port)], options: launchOptions)
} catch let error as Win32Error {
CloseHandle(childStdinHandle)
throw StartError(message: "Call to Win32 \(error.apiName) failed with error code \(error.errorCode).")
}

Expand All @@ -55,7 +109,10 @@ public class WinAppDriver: WebDriver {
}
}

return WinAppDriver(httpWebDriver: httpWebDriver, processTree: processTree)
return WinAppDriver(
httpWebDriver: httpWebDriver,
processTree: processTree,
childStdinHandle: childStdinHandle)
}

deinit {
Expand All @@ -66,6 +123,9 @@ public class WinAppDriver: WebDriver {
assertionFailure("WinAppDriver did not terminate within the expected time: \(error).")
}
}
if let childStdinHandle {
CloseHandle(childStdinHandle)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to keep the stdin write handle until here if just to close it? And since we have it, we could send \n for nicer termination since WinAppDriver awaits one:

C:>"C:\Program Files (x86)\Windows Application Driver\WinAppDriver.exe"
Windows Application Driver listening for requests at: http://127.0.0.1:4723/
Press ENTER to exit.

Exiting...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we close the handle, this closes stdin for the WinAppDriver process, which causes the process to shut down. That's why we need to keep it open until the process shuts down. While we could use that as a proxy to shut down WinAppDriver in a cleaner manner, we only have childStdinHandle set up if we redirect the output to a file. I'm not opposed to changing the logic but then we have no way of redirecting the output to the spawned terminal to preserve API compatibility.

}
}

@discardableResult
Expand Down
34 changes: 34 additions & 0 deletions Tests/WinAppDriverTests/AppDriverOptionsTest.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
import TestsCommon
import WinSDK
import XCTest

@testable import WebDriver
@testable import WinAppDriver

class AppDriverOptionsTest: XCTestCase {
func tempFileName() -> String {
return FileManager.default.temporaryDirectory.appendingPathComponent(UUID().uuidString)
.appendingPathExtension("txt").path
}

/// Tests that redirecting stdout to a file works.
func testStdoutRedirectToFile() throws {
let outputFile = try {
Steelskin marked this conversation as resolved.
Show resolved Hide resolved
// Start a new instance of msinfo32 and write the output to a file.
let outputFile = tempFileName()
let _ = try MSInfo32App(
winAppDriver: WinAppDriver.start(
outputFile: outputFile
))
return outputFile
}()

// Read the output file.
let output = try String(contentsOfFile: outputFile, encoding: .utf16LittleEndian)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does WinAppDriver write as utf-16 when redirected to a file? That's unusual

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It writes UTF-16 either way. It's usually not noticeable because Windows Terminal handles either.

Copy link
Contributor

Choose a reason for hiding this comment

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

Terminals are supposed to decode stdout according to the GetConsoleOutputCP() of the console, so outputting UTF-16 would result in garbled contents. 🤔🤔🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably witchcraft involved:

PS > $output = & 'C:\Program Files (x86)\Windows Application Driver\WinAppDriver.exe'

PS > [system.Text.Encoding]::ASCII.GetBytes($output)
87
0
105
0
[...]

PS > $output = echo potato
PS > [system.Text.Encoding]::ASCII.GetBytes($output)
112
111
116
97
116
111

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know if that's a valid test but it is puzzling!


// Delete the file.
try FileManager.default.removeItem(atPath: outputFile)

XCTAssert(output.contains("msinfo32"))
}
}
Loading