feat: Building playbackengine #622
Conversation
|
There was a problem hiding this comment.
I'm very uneasy about the approach of finding msbuild and recompiling that project. I know I got convinced on our last call but I believe we need the trade offs (and risks) written down somewhere and involve other folks before we go ahead. I worry about side effects beyond building this one project (e.g: What if I have 2 unity projects open?)
|
|
||
| namespace Sentry.Unity.Editor | ||
| { | ||
| internal static class EditorFileIO |
There was a problem hiding this comment.
Is this only operations that can run on the editor? The type lives in the Sentry.Unity.Editor so that's covered so what about dropping Editor from the name?
It seems we have some wrappers over Unity's FileUtil with some logging and dealing with directories being created. Do we need to use that so Unity can track things or we could just use File and Directory classes from the BCL?
What if we just call this SentryFileUtil or simply FileUtil:
| internal static class EditorFileIO | |
| internal static class FileUtil |
bitsandfoxes
force-pushed
the
feat/build-playbackengine
branch
from
8ec2d83
to
f09bbf1
Compare
| { | ||
| if (!File.Exists(editorOptions.MSBuildPath)) | ||
| { | ||
| logger.LogDebug("Failed to find 'MSBuild' at '{0}'. Trying to locate.", editorOptions.MSBuildPath); |
There was a problem hiding this comment.
| logger.LogDebug("Failed to find 'MSBuild' at '{0}'. Trying to locate.", editorOptions.MSBuildPath); | |
| logger.LogDebug("'MSBuild' not known. '{0}'. Trying to locate.", editorOptions.MSBuildPath); |
| { | ||
| if (!File.Exists(editorOptions.VSWherePath)) | ||
| { | ||
| logger?.LogDebug("Failed to find 'VSWhere' at '{0}'. Trying to locate.", editorOptions.VSWherePath); |
There was a problem hiding this comment.
| logger?.LogDebug("Failed to find 'VSWhere' at '{0}'. Trying to locate.", editorOptions.VSWherePath); | |
| logger?.LogDebug("Path to 'VSWhere' not known. Have '{0}'. Trying to locate.", editorOptions.VSWherePath); |
| StartInfo = new ProcessStartInfo | ||
| { | ||
| FileName = editorOptions.VSWherePath, | ||
| Arguments = "-latest -requires Microsoft.Component.MSBuild -find MSBuild\\**\\Bin\\MSBuild.exe", |
There was a problem hiding this comment.
Can we check if the file we found is signed?
Does Unity also run this same command? Finding a file like this and executing it?
|
|
||
| logger?.LogDebug("Using 'VSWhere' at '{0}' to locate MSBuild.", editorOptions.VSWherePath); | ||
|
|
||
| var vsWhereOutput = ""; |
There was a problem hiding this comment.
| var vsWhereOutput = ""; | |
| var vsWhereOutput = new StringBuilder(); |
|
|
||
| logger?.LogDebug("Using 'VSWhere' at '{0}' to locate MSBuild.", editorOptions.VSWherePath); | ||
|
|
||
| var vsWhereOutput = ""; |
There was a problem hiding this comment.
| var vsWhereOutput = ""; | |
| // The program only returns one line | |
| var vsWhereOutput = ""; |
|
|
||
| if (!File.Exists(vsWhereOutput)) | ||
| { | ||
| throw new FileNotFoundException($"Failed to locate 'MSBuild'."); |
There was a problem hiding this comment.
Where do we handle this? Do we downgrade to initalizing the SDK at runtime?
| var packageListRequest = Client.List(true); | ||
| while (!packageListRequest.IsCompleted) | ||
| { | ||
| // TODO: timeout - can't use Task.Run because it has to be on the main thread |
There was a problem hiding this comment.
Basic timeout could be something this:
| // TODO: timeout - can't use Task.Run because it has to be on the main thread | |
| Thread.Sleep(100); | |
| if (counter++ * 10 > timeoutMillisecond) |
| process.WaitForExit(); | ||
|
|
||
| var logFile = Path.Combine(_projectPath, "build.log"); | ||
| File.WriteAllText(logFile, output.ToString()); |
There was a problem hiding this comment.
Instead of allocating the whole string and writing it with WriteAllText it's better to use a different API and stream data through.
Instead of a string builder, get an OutputStream and just write the data straight into it.
I think the Process class already has streams you can hook into and write into a FileStream.
| EditorFileIO.CopyDirectory(projectSource, projectTarget, logger); | ||
|
|
||
| // TODO: Does the .props file have to look like that? | ||
| // The 'UnityCommon.props' is missing from the provided source code |
There was a problem hiding this comment.
What if this is fixed on future versions? Maybe check if it's already there before doing this?
| EditorGUI.DrawRect(EditorGUILayout.GetControlRect(false, 1), Color.gray); | ||
| EditorGUILayout.Space(); | ||
|
|
||
| EditorGUILayout.Toggle("Add Sentry to Windows Player", cliOptions.AddSentryToWindowsPlayer); |
There was a problem hiding this comment.
what about using: Initialize and "native crash" in the name somehow?
|
This issue has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you remove the label "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
To note:
AddSentryToWindowsPlayeron the editor options for now#skip-changelog