-
Notifications
You must be signed in to change notification settings - Fork 113
Clean up scripts and improve documentation to prepare for a release #375
Clean up scripts and improve documentation to prepare for a release #375
Conversation
@@ -4,15 +4,15 @@ | |||
<ImportGroup Label="PropertySheets" /> | |||
<PropertyGroup Label="UserMacros"> | |||
<!-- From: https://www.blackmagicdesign.com/support --> | |||
<DeckLink_inc>$(SolutionDir)\..\..\external\Blackmagic DeckLink SDK 10.9.11\Win\include</DeckLink_inc> | |||
<DeckLink_inc>C:\Blackmagic DeckLink SDK 10.9.11\Win\include</DeckLink_inc> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<DeckLink_inc>C:\Blackmagic DeckLink SDK 10.9.11\Win\include</DeckLink_inc> [](start = 4, length = 75)
These changes look unintentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are intentional. We no longer require these SDKs to be relocated to the project for scripted building. Now we just default to the best expected location for these sdks.
Previously, we had hardcoded dll look up locations in build scripts. Now we use whatever a developer specifies here to resolve the location of these dependencies.
$SolutionDir = "$PSScriptRoot\..\..\..\src\SpectatorView.Native\" | ||
|
||
Write-Host "`nIncluded Compositor Components:" | ||
Write-Host " Blackmagic Decklink: " (Test-Path $DependenciesPropsContent.Project.PropertyGroup.DeckLink_inc.replace($SolutionDirVariable, $SolutionDir)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Test-Path $DependenciesPropsContent.Project.PropertyGroup.DeckLink_inc.replace($SolutionDirVariable, $SolutionDir)) [](start = 52, length = 116)
If the $(SolutionDir) is no longer in Dependencies.props, do you still need to do the replace here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This step doesn't introduce any additional errors if
New-Item -ItemType Directory -Force -Path $fileName | ||
Copy-Item -Path "$fileContent\*" -Destination $fileName -Recurse | ||
|
||
Set-Location $currDir |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this work for both file and directory symlinks? If it's intended for both, maybe a comment + naming the variables here $fileOrDirectory would make that clearer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question, we have not had any file symlinks to test with. This naming was based on the previous piped arguments higher up, so i will probably keep this as it is for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This review contains the following:
TODO:
Before we can decalre an official release, two issues were discovered: