-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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.
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
Refine corerun instructions. Improve argument parsing in src/tests/build. #81733
Refine corerun instructions. Improve argument parsing in src/tests/build. #81733
Conversation
Tagging subscribers to this area: @hoyosjs Issue DetailsWhile working through the corerun instructions, I got tripped up in a few ways.
This PR updates the instructions and also improves the argument parsing in
|
@@ -192,7 +192,7 @@ CoreRun.exe \runtime\artifacts\bin\coreclr\windows.x64.Release\crossgen2\crossge | |||
On Linux: | |||
|
|||
```bash | |||
./corerun /runtime/artifacts/bin/coreclr/Linux.x64.Release/crossgen2/crossgen2.dll | |||
corerun /runtime/artifacts/bin/coreclr/Linux.x64.Release/crossgen2/crossgen2.dll |
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.
corerun
is executed from $PATH, not from the current folder
@@ -53,9 +53,9 @@ dotnet new console | |||
dotnet build | |||
``` | |||
|
|||
Now, instead of running our app the usual way, we will use our newly built `corerun` to execute it using our build of the runtime. For this, we will follow the steps denoted below: | |||
Now, instead of running our app the usual way, we will use `corerun` to execute it using our build of the runtime. The `corerun` executable is created as part of building the `clr` subset, and it will exist in the `<repo root>/artifacts/bin/coreclr/<OS>.<Arch>.<Configuration>` folder. For this, we will follow the steps denoted below: |
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.
"Our newly built" was confusing to me. I removed that and aimed to add clarity that it should have been built already (and where it exists so that can be confirmed).
|
||
```powershell | ||
# Note the '+=' since we're appending to the already existing PATH variable. | ||
# Also, replace the ';' with ':' if on Linux or macOS. | ||
$Env:PATH += ';<repo_root>\artifacts\tests\coreclr\windows.x64.Debug\Tests\Core_Root' | ||
.\corerun HelloWorld.dll | ||
corerun HelloWorld.dll |
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.
corerun
is found via path, not from the current folder.
@@ -30,6 +30,7 @@ set "__MsbuildDebugLogsDir=%__LogsDir%\MsbuildDebugLogs" | |||
set __Exclude=%__RepoRootDir%\src\tests\issues.targets | |||
|
|||
REM __UnprocessedBuildArgs are args that we pass to msbuild (e.g. /p:TargetArchitecture=x64) | |||
set __commandName=%~nx0 |
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.
Down in Usage
, we need to print the command name, but we've already shifted the args by then. This will grab the file name and extension before we do any shifting.
src/tests/build.cmd
Outdated
if /i "%1" == "-help" goto Usage | ||
if /i "%1" == "--help" goto Usage | ||
@REM All arguments following this tag will be passed directly to msbuild (as unprocessed arguments) | ||
if /i "%1" == "--" (set processedArgs=!processedArgs! %1&shift&goto ArgsDone) |
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.
Previously, handling of --
seemed broken. It was just continuing the loop, which could then recognize and process arguments that were expected to be passed through. Chances are, such args were going unrecognized and by happenstance, they were still getting passed through. But this changes the behavior to match the Usage notes.
src/tests/build.cmd
Outdated
if /i "%arg%" == "skipgeneratelayout" (set __SkipGenerateLayout=1&set processedArgs=!processedArgs! %1&shift&goto Arg_Loop) | ||
|
||
if /i "%arg%" == "copynativeonly" (set __CopyNativeTestBinaries=1&set __SkipNative=1&set __CopyNativeProjectsAfterCombinedTestBuild=false&set __SkipGenerateLayout=1&set __SkipTestWrappers=1&set processedArgs=!processedArgs! %1&shift&goto Arg_Loop) | ||
if /i "%arg%" == "generatelayoutonly" (set __GenerateLayoutOnly=1&set __SkipManaged=1&set __SkipNative=1&set __CopyNativeProjectsAfterCombinedTestBuild=false&set processedArgs=!processedArgs! %1&shift&goto Arg_Loop) |
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.
Previously, the generatelayoutonly
argument didn't set __GenerateLayoutOnly=1
, which made it do more work than intended when specified.
if /i "%arg%" == "tree" (set __BuildTestTree=!__BuildTestTree!%2%%3B&set processedArgs=!processedArgs! %1 %2&shift&shift&goto Arg_Loop) | ||
if /i "%arg%" == "log" (set __BuildLogRootName=%2&set processedArgs=!processedArgs! %1 %2&shift&shift&goto Arg_Loop) | ||
if /i "%arg%" == "Exclude" (set __Exclude=%2&set processedArgs=!processedArgs! %1 %2&shift&shift&goto Arg_Loop) | ||
if /i "%arg%" == "priority" (set __Priority=%2&set processedArgs=!processedArgs! %1=%2&shift&shift&goto Arg_Loop) |
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.
Despite a comment about priority
above that it needs special handling; it was still incorrect. It was shifting at the wrong place, and it was resulting in the processed/unprocessed args not being correct.
src/tests/build.cmd
Outdated
if /i "%arg%" == "priority" (set __Priority=%2&set processedArgs=!processedArgs! %1=%2&shift&shift&goto Arg_Loop) | ||
|
||
@REM The following arguments also consume two subsequent arguments | ||
if /i "%arg%" == "cmakeargs" (set __CMakeArgs="%2=%3" %__CMakeArgs%&set "processedArgs=!processedArgs! %1 %2 %3"&shift&shift&shift&goto Arg_Loop) |
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.
The cmakeargs
argument was also not correctly shifting the arguments, so extra/incorrect arguments would end up flowing through to msbuild.
@@ -130,7 +150,40 @@ if [!processedArgs!]==[] ( | |||
) | |||
) |
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 logic was unexpectedly above the :ArgsDone
label and was being hit whenever there was an unrecognized argument (vs. when we were done processing arguments).
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 was working by coincidence that argument parsing aborts upon the first unrecognized argument. There are existing pipelines relying on that behavior.
Nonetheless, the logic is moved below the label now, even though we still exit processing upon the first unrecognized argument.
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.
Very nice, thanks Jeff for all the fixes, improvements and documentation updates!
While working through the corerun instructions, I got tripped up in a few ways.
src/tests/build.cmd
was not behaving the same assrc/tests/build.sh
This PR updates the instructions and also improves the argument parsing in
src/tests/build
based on these findings. For the argument parsing, I also introduced a mechanism for testing the results of the argument parsing for better maintainability going forward.