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

Jump to bottom

Redscript Compilation Error-checking #23

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Redscript Compilation Error-checking #23

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
92faa92
correctly null terminate buffer, don't escape redscript_paths.txt, en…
jackhumbert May 9, 2023
9e128c0
adds some error checking for redscript compilation, exits on error
jackhumbert May 10, 2023
32f76f4
updates from review, move struct to sdk
jackhumbert May 10, 2023
faaa361
update sdk
jackhumbert May 11, 2023
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
50 changes: 41 additions & 9 deletions src/dll/Hooks/ExecuteProcess.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,30 +3,62 @@
#include "App.hpp"
#include "Hook.hpp"
#include "Systems/ScriptCompilationSystem.hpp"
#include <winbase.h>
jackhumbert marked this conversation as resolved.
Show resolved Hide resolved

namespace
{
bool isAttached = false;

bool _Global_ExecuteProcess(void* a1, RED4ext::CString& aCommand, FixedWString& aArgs,
RED4ext::CString& aCurrentDirectory, char a5);
bool _Global_ExecuteProcess(ScriptCompilation* a1, RED4ext::CString& aCommand, FixedWString& aArgs,
jackhumbert marked this conversation as resolved.
Show resolved Hide resolved
RED4ext::CString& aCurrentDirectory, ExecuteProcess_Flags aFlags);
Hook<decltype(&_Global_ExecuteProcess)> Global_ExecuteProcess(Addresses::Global_ExecuteProcess,
&_Global_ExecuteProcess);

bool _Global_ExecuteProcess(void* a1, RED4ext::CString& aCommand, FixedWString& aArgs,
RED4ext::CString& aCurrentDirectory, char a5)
bool _Global_ExecuteProcess(ScriptCompilation* a1, RED4ext::CString& aCommand, FixedWString& aArgs,
RED4ext::CString& aCurrentDirectory, ExecuteProcess_Flags aFlags)
{
if (strstr(aCommand.c_str(), "scc.exe") == nullptr)
{
return Global_ExecuteProcess(a1, aCommand, aArgs, aCurrentDirectory, a5);
return Global_ExecuteProcess(a1, aCommand, aArgs, aCurrentDirectory, aFlags);
}

auto scriptCompilationSystem = App::Get()->GetScriptCompilationSystem();
auto str = App::Get()->GetScriptCompilationSystem()->GetCompilationArgs(aArgs);

FixedWString newArgs;
newArgs.str = scriptCompilationSystem->GetCompilationArgs(aArgs).c_str();
newArgs.str = str.c_str();
newArgs.length = newArgs.maxLength = wcslen(newArgs.str);
return Global_ExecuteProcess(a1, aCommand, newArgs, aCurrentDirectory, a5);

spdlog::info(L"Final redscript compilation arg string: '{}'", newArgs.str);
auto result = Global_ExecuteProcess(a1, aCommand, newArgs, aCurrentDirectory, aFlags);

// 60000 is used in the game
auto waitResult = WaitForSingleObject(a1->handle, 60000);
jackhumbert marked this conversation as resolved.
Show resolved Hide resolved
WopsS marked this conversation as resolved.
Show resolved Hide resolved
switch (waitResult) {
case WAIT_TIMEOUT:
spdlog::error("Redscript compilation timed-out - exiting");
SHOW_LAST_ERROR_MESSAGE_AND_EXIT_FILE_LINE("Redscript compilation timed-out");
Copy link
Owner

Choose a reason for hiding this comment

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

Should we exit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's the debate I wanted to raise :) if we've added scripts, and it's the first try at compiling, I think we should. Otherwise, it could be a hot-reload, which should probably be handled differently/passed to another part of that plugin.

Copy link
Owner

Choose a reason for hiding this comment

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

I'm fine with it, @psiberx what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I applied the condition I described to the exit later on - we're still exiting for the cases here, though.

Copy link
Contributor

@psiberx psiberx May 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think we should only exit on timeout, but not if compilation fails.
Exiting only on game start is compatible with hot reload indeed. Although redscript shows the message "the game will start without scripts" when compilation fails, which can be a bit confusing.

break;
case WAIT_ABANDONED:
spdlog::error("Redscript compilation was abandoned - exiting");
SHOW_LAST_ERROR_MESSAGE_AND_EXIT_FILE_LINE("Redscript compilation was abandoned");
break;
case WAIT_FAILED:
spdlog::error("Redscript compilation failed - exiting");
SHOW_LAST_ERROR_MESSAGE_AND_EXIT_FILE_LINE("Redscript compilation failed");
break;
}
jackhumbert marked this conversation as resolved.
Show resolved Hide resolved

GetExitCodeProcess(a1->handle, &a1->errorCode);
jackhumbert marked this conversation as resolved.
Show resolved Hide resolved

if (a1->errorCode) {
spdlog::error(L"Redscript compilation was unsuccessful with error code: {} - exiting", a1->errorCode);
// redscript already showed the error, so we can just exit
TerminateProcess(GetCurrentProcess(), 1);
} else {
spdlog::info(L"Redscript compilation executed successfully");
}

return result;
}
} // namespace

Expand Down
5 changes: 2 additions & 3 deletions src/dll/Systems/ScriptCompilationSystem.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,10 +90,9 @@ std::wstring ScriptCompilationSystem::GetCompilationArgs(const FixedWString& aOr
for (const auto& [plugin, path] : m_scriptPaths)
{
spdlog::info(L"{}: '{}'", plugin->GetName(), path);
pathsFile << path << std::endl;
pathsFile << path.wstring() << std::endl;
}
spdlog::info(L"Paths written to: '{}'", pathsFilePath);
format_to(std::back_inserter(buffer), LR"( -compilePathsFile "{}"\0)", pathsFilePath);
spdlog::info(L"Final redscript compilation arg string: '{}'", buffer.data());
format_to(std::back_inserter(buffer), LR"( -compilePathsFile "{}"{})", pathsFilePath, '\0');
return buffer.data();
}
25 changes: 25 additions & 0 deletions src/dll/Systems/ScriptCompilationSystem.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,31 @@ struct FixedWString
const wchar_t* str;
};

// aFlags is initially 0
enum class ExecuteProcess_Flags : unsigned char
jackhumbert marked this conversation as resolved.
Show resolved Hide resolved
{
// unsets CREATE_NO_WINDOW
ShouldCreateWindow = 0x1,
// sets CREATE_BREAKAWAY_FROM_JOB | CREATE_SUSPENDED in Process Creation Flags
BreakawayAndSuspend = 0x2,
Unk3 = 0x3,
// unsets bInheritHandles
NoInheritHandles = 0x4,
};

DEFINE_ENUM_FLAG_OPERATORS(ExecuteProcess_Flags)

struct ScriptCompilation
{
wchar_t command[4096];
PHANDLE readPipe;
PHANDLE writePipe;
HANDLE handle;
HANDLE hThread;
uint64_t unk2;
DWORD errorCode;
};

class ScriptCompilationSystem : public ISystem
{
public:
Expand Down