Centralize installation logic & reduce admin reqs #11
Conversation
Prior to this patch, the project's C++ code could only be built if Microsoft Visual Studio was run with the privileges of a system administrator. Encapsulate installation steps in the MakeVoice.exe binary, and extend that binary to require administrative privileges (allowing the code to be built without them). This change also supports forthcoming work to eliminate duplicated installation logic between the project's Node.js code and C++ code.
| <PostBuildEvent> | ||
| <Message>Performing Registration</Message> | ||
| <Command>regsvr32 /s /c "$(TargetPath)"</Command> | ||
| </PostBuildEvent> |
There was a problem hiding this comment.
@mzgoddard I'm still only guessing about why gh-9 removes this registration, but I hesitated to accept the removal in that context because it's important for the C++ development workflow.
Since this patch relocates the registration to MakeVoice.exe, and since that doesn't be appear to be used in gh-9, I think this will address both of our needs. And (as noted in the commit message) it gets us closer to a solution where npm install just shells out to MakeVoice.exe. Although I wonder if that will have a negative impact on what you're trying to achieve in gh-9.
There was a problem hiding this comment.
I went back through the Actions logs for the runs testing the testing pr https://github.com/bocoup/aria-at-automation-driver/pull/9. Run 9 and 10 exited that registration building ttseng with code 3. Immediate searches on microsoft docs and regsvr32 didn't really help with what code 3 means. One commit in the process of debugging that issue made that removal. The next was able to build ttseng and Vocalizer and complete the other action steps.
Run 9: https://github.com/bocoup/aria-at-automation-driver/runs/4511094728?check_suite_focus=true#step:4:6386
Run 10: https://github.com/bocoup/aria-at-automation-driver/runs/4511137068?check_suite_focus=true#step:4:6386
Run 11: https://github.com/bocoup/aria-at-automation-driver/runs/4511238540?check_suite_focus=true#step:4:6444
regsvr32 doc
There was a problem hiding this comment.
I recall from the difficulty getting the dll working on an image I created that the logging functionality I added to the dll locally that it'd initialize the dll scoped logging class instance when the dll is loaded. Not that logging functionality but some other change made by regsvr32 called while building ttseng encountered an error in that context.
A result from a quick google search seems to confirm that:
If something goes wrong, the regsvr32 program reports the error details to the user, and the exit code summarizes which step it got stuck at.
...
What do the various regsvr32 exit codes mean?
Exit code Meaning 3 LoadLibrary failed
There was a problem hiding this comment.
For some added clarity, ttseng is registered in a later step by npm ci.
https://github.com/bocoup/aria-at-automation-driver/runs/4511238540?check_suite_focus=true#step:7:28
| CloseHandle(process_info.hProcess); | ||
| CloseHandle(process_info.hThread); | ||
|
|
||
| return exitCode == 0 ? S_OK : E_FAIL; |
There was a problem hiding this comment.
Should we log the exit code in some way if it non-zero?
There was a problem hiding this comment.
Sounds good to me! Done in a "fixup" commit.
Prior to this patch, the project's C++ code could only be built if
Microsoft Visual Studio was run with the privileges of a system
administrator.
Encapsulate installation steps in the MakeVoice.exe binary, and extend
that binary to require administrative privileges (allowing the code to
be built without them).
This change also supports forthcoming work to eliminate duplicated
installation logic between the project's Node.js code and C++ code.