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

Refine corerun instructions. Improve argument parsing in src/tests/build. #81733

Merged
merged 3 commits into from
Feb 8, 2023
Merged

Refine corerun instructions. Improve argument parsing in src/tests/build. #81733

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ee3c24e
Refine corerun instructions. Improve arg parsing in src/tests/build.
jeffhandley Feb 7, 2023
9b0efe5
Revert the libraries configuration default. Refine usage and docs.
jeffhandley Feb 7, 2023
e4f796c
Exit argument processing at first unrecognized arg per existing pipel…
jeffhandley Feb 8, 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
2 changes: 1 addition & 1 deletion docs/workflow/testing/coreclr/running-aspnet-benchmarks-with-crossgen2.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member Author

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

--Os --composite -o /path/to/results/composite/TotalComposite.dll /path/to/results/application/*.dll
```

Expand Down
10 changes: 5 additions & 5 deletions docs/workflow/testing/using-corerun-and-coreroot.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Copy link
Member Author

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).


* First we will add the `Core_Root` folder to the `PATH` environment variable for ease of use. Note that you can always skip this step and fully qualify the name instead.
* First we will add `corerun`'s folder to the `PATH` environment variable for ease of use. Note that you can always skip this step and fully qualify the name instead.
* This example assumes you built on the _Debug_ configuration for the _x64_ architecture. Make sure you adjust the path accordingly to your kind of build.
* Then, we also need the libraries. Since we only built the runtime, we will tell `corerun` to use the ones shipped with .NET's default installation on your machine.
* This example assumes your default .NET installation's version is called "_7.0.0_". Same deal as with your runtime build path, adjust to the version you have installed on your machine.
Expand All @@ -78,7 +78,7 @@ export CORE_LIBRARIES="/usr/local/share/dotnet/shared/Microsoft.NETCore.App/7.0.
corerun HelloWorld.dll
```

On Powershell:
On PowerShell:

```powershell
# Note the '+=' since we're appending to the already existing PATH variable.
Expand Down Expand Up @@ -117,13 +117,13 @@ export PATH="$PATH:<repo_root>/artifacts/tests/coreclr/linux.x64.Debug/Tests/Cor
corerun HelloWorld.dll
```

On Powershell:
On PowerShell:

```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
Copy link
Member Author

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.

```

The advantage of generating the Core_Root, instead of sticking to the _corerun_ from the _clr_ build, is that you can also test and debug libraries at the same time.
Expand Down
137 changes: 95 additions & 42 deletions src/tests/build.cmd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member Author

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.

set "__args= %*"
set processedArgs=
set __UnprocessedBuildArgs=
Expand Down Expand Up @@ -69,14 +70,10 @@ set __BuildNeedTargetArg=
:Arg_Loop
if "%1" == "" goto ArgsDone

if /i "%1" == "/?" goto Usage
if /i "%1" == "-?" goto Usage
if /i "%1" == "/h" goto Usage
if /i "%1" == "-h" goto Usage
if /i "%1" == "/help" goto Usage
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)
Copy link
Member Author

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.


@REM The following arguments do not support `/`, `-`, or `--` prefixes
if /i "%1" == "x64" (set __BuildArch=x64&set processedArgs=!processedArgs! %1&shift&goto Arg_Loop)
if /i "%1" == "x86" (set __BuildArch=x86&set processedArgs=!processedArgs! %1&shift&goto Arg_Loop)
if /i "%1" == "arm" (set __BuildArch=arm&set processedArgs=!processedArgs! %1&shift&goto Arg_Loop)
Expand All @@ -88,38 +85,61 @@ if /i "%1" == "checked" (set __BuildType=Checked&set processedArgs

if /i "%1" == "ci" (set __ArcadeScriptArgs="-ci"&set __ErrMsgPrefix=##vso[task.logissue type=error]&set processedArgs=!processedArgs! %1&shift&goto Arg_Loop)

if /i "%1" == "skiprestorepackages" (set __SkipRestorePackages=1&set processedArgs=!processedArgs! %1&shift&goto Arg_Loop)
if /i "%1" == "skipmanaged" (set __SkipManaged=1&set processedArgs=!processedArgs! %1&shift&goto Arg_Loop)
if /i "%1" == "skipnative" (set __SkipNative=1&set __CopyNativeProjectsAfterCombinedTestBuild=false&set processedArgs=!processedArgs! %1&shift&goto Arg_Loop)
if /i "%1" == "skiptestwrappers" (set __SkipTestWrappers=1&set processedArgs=!processedArgs! %1&shift&goto Arg_Loop)
if /i "%1" == "skipgeneratelayout" (set __SkipGenerateLayout=1&set processedArgs=!processedArgs! %1&shift&goto Arg_Loop)

if /i "%1" == "rebuild" (set __RebuildTests=1&set processedArgs=!processedArgs! %1&shift&goto Arg_Loop)

if /i "%1" == "test" (set __BuildTestProject=!__BuildTestProject!%2%%3B&set processedArgs=!processedArgs! %1 %2&shift&shift&goto Arg_Loop)
if /i "%1" == "dir" (set __BuildTestDir=!__BuildTestDir!%2%%3B&set processedArgs=!processedArgs! %1 %2&shift&shift&goto Arg_Loop)
if /i "%1" == "tree" (set __BuildTestTree=!__BuildTestTree!%2%%3B&set processedArgs=!processedArgs! %1 %2&shift&shift&goto Arg_Loop)

if /i "%1" == "log" (set __BuildLogRootName=%2&set processedArgs=!processedArgs! %1 %2&shift&shift&goto Arg_Loop)

if /i "%1" == "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 "%1" == "generatelayoutonly" (set __SkipManaged=1&set __SkipNative=1&set __CopyNativeProjectsAfterCombinedTestBuild=false&set processedArgs=!processedArgs! %1&shift&goto Arg_Loop)
if /i "%1" == "buildtestwrappersonly" (set __SkipNative=1&set __SkipManaged=1&set __BuildTestWrappersOnly=1&set __SkipGenerateLayout=1&set processedArgs=!processedArgs! %1&shift&goto Arg_Loop)
if /i "%1" == "-cmakeargs" (set __CMakeArgs="%2=%3" %__CMakeArgs%&set "processedArgs=!processedArgs! %1 %2=%3"&shift&shift&goto Arg_Loop)
if /i "%1" == "-msbuild" (set __Ninja=0&set processedArgs=!processedArgs! %1&shift&goto Arg_Loop)
if /i "%1" == "buildagainstpackages" (echo error: Remove /BuildAgainstPackages switch&&exit /b1)
if /i "%1" == "crossgen2" (set __TestBuildMode=crossgen2&set processedArgs=!processedArgs! %1&shift&goto Arg_Loop)
if /i "%1" == "composite" (set __CompositeBuildMode=1&set __TestBuildMode=crossgen2&set processedArgs=!processedArgs! %1&shift&goto Arg_Loop)
if /i "%1" == "pdb" (set __CreatePdb=1&set processedArgs=!processedArgs! %1&shift&goto Arg_Loop)
if /i "%1" == "nativeaot" (set __TestBuildMode=nativeaot&set processedArgs=!processedArgs! %1&shift&goto Arg_Loop)
if /i "%1" == "perfmap" (set __CreatePerfmap=1&set processedArgs=!processedArgs! %1&shift&goto Arg_Loop)
if /i "%1" == "Exclude" (set __Exclude=%2&set processedArgs=!processedArgs! %1 %2&shift&shift&goto Arg_Loop)
if /i "%1" == "-priority" (set __Priority=%2&shift&set processedArgs=!processedArgs! %1=%2&shift&goto Arg_Loop)
if /i "%1" == "allTargets" (set "__BuildNeedTargetArg=/p:CLRTestBuildAllTargets=%1"&set processedArgs=!processedArgs! %1&shift&goto Arg_Loop)
if /i "%1" == "-excludemonofailures" (set __Mono=1&set processedArgs=!processedArgs! %1&shift&goto Arg_Loop)
if /i "%1" == "-mono" (set __Mono=1&set processedArgs=!processedArgs! %1&shift&goto Arg_Loop)
if /i "%1" == "mono" (set __Mono=1&set processedArgs=!processedArgs! %1&shift&goto Arg_Loop)
if /i "%1" == "--" (set processedArgs=!processedArgs! %1&shift&goto Arg_Loop)
@REM For the arguments below, we support `/`, `-`, and `--` prefixes.
@REM But we also recognize them without prefixes at all. To achieve that,
@REM we remove the `/` and `-` characters from the string for comparison.

Copy link
Member Author

Choose a reason for hiding this comment

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

All of the changes for recognizing /, -, and -- prefixes were based on trying to follow the instructions for -generatelayoutonly. A few things were discovered:

  1. The build.sh script handles arguments with and without - prefixes, but the build.cmd was not
  2. The build.cmd script was inconsistent with which arguments needed - and which didn't
  3. The help argument even recognized the -- prefix

The logic below makes each of the arguments work with or without a prefix, supporting /, -, or -- prefixes.

set arg=%~1
set arg=%arg:/=%
set arg=%arg:-=%

if /i "%arg%" == "?" goto Usage
if /i "%arg%" == "h" goto Usage
if /i "%arg%" == "help" goto Usage

@REM Specify this argument to test the argument parsing logic of this script without executing the build
if /i "%arg%" == "TestArgParsing" (set __TestArgParsing=1&set processedArgs=!processedArgs! %1&shift&goto Arg_Loop)

@REM The following arguments are switches that do not consume any subsequent arguments
if /i "%arg%" == "rebuild" (set __RebuildTests=1&set processedArgs=!processedArgs! %1&shift&goto Arg_Loop)
if /i "%arg%" == "skiprestorepackages" (set __SkipRestorePackages=1&set processedArgs=!processedArgs! %1&shift&goto Arg_Loop)
if /i "%arg%" == "skipmanaged" (set __SkipManaged=1&set processedArgs=!processedArgs! %1&shift&goto Arg_Loop)
if /i "%arg%" == "skipnative" (set __SkipNative=1&set __CopyNativeProjectsAfterCombinedTestBuild=false&set processedArgs=!processedArgs! %1&shift&goto Arg_Loop)
if /i "%arg%" == "skiptestwrappers" (set __SkipTestWrappers=1&set processedArgs=!processedArgs! %1&shift&goto Arg_Loop)
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)
Copy link
Member Author

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%" == "buildtestwrappersonly" (set __SkipNative=1&set __SkipManaged=1&set __BuildTestWrappersOnly=1&set __SkipGenerateLayout=1&set processedArgs=!processedArgs! %1&shift&goto Arg_Loop)
if /i "%arg%" == "msbuild" (set __Ninja=0&set processedArgs=!processedArgs! %1&shift&goto Arg_Loop)
if /i "%arg%" == "crossgen2" (set __TestBuildMode=crossgen2&set processedArgs=!processedArgs! %1&shift&goto Arg_Loop)
if /i "%arg%" == "composite" (set __CompositeBuildMode=1&set __TestBuildMode=crossgen2&set processedArgs=!processedArgs! %1&shift&goto Arg_Loop)
if /i "%arg%" == "pdb" (set __CreatePdb=1&set processedArgs=!processedArgs! %1&shift&goto Arg_Loop)
if /i "%arg%" == "nativeaot" (set __TestBuildMode=nativeaot&set processedArgs=!processedArgs! %1&shift&goto Arg_Loop)
if /i "%arg%" == "perfmap" (set __CreatePerfmap=1&set processedArgs=!processedArgs! %1&shift&goto Arg_Loop)
if /i "%arg%" == "allTargets" (set "__BuildNeedTargetArg=/p:CLRTestBuildAllTargets=%1"&set processedArgs=!processedArgs! %1&shift&goto Arg_Loop)
if /i "%arg%" == "excludemonofailures" (set __Mono=1&set processedArgs=!processedArgs! %1&shift&goto Arg_Loop)
if /i "%arg%" == "mono" (set __Mono=1&set processedArgs=!processedArgs! %1&shift&goto Arg_Loop)

@REM The following arguments also consume one subsequent argument
if /i "%arg%" == "test" (set __BuildTestProject=!__BuildTestProject!%2%%3B&set processedArgs=!processedArgs! %1 %2&shift&shift&goto Arg_Loop)
if /i "%arg%" == "dir" (set __BuildTestDir=!__BuildTestDir!%2%%3B&set processedArgs=!processedArgs! %1 %2&shift&shift&goto Arg_Loop)
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)
Copy link
Member Author

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.


@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)
Copy link
Member Author

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.


@REM Obsolete arguments that now produce errors
if /i "%arg%" == "buildagainstpackages" (echo error: Remove /BuildAgainstPackages switch&&exit /b 1)

@REM Any unrecognized argument will be unprocessed and passed to msbuild
shift
goto Arg_Loop

:ArgsDone

if [!processedArgs!]==[] (
set __UnprocessedBuildArgs=%__args%
Expand All @@ -130,7 +150,40 @@ if [!processedArgs!]==[] (
)
)
Copy link
Member Author

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).

Copy link
Member Author

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.


:ArgsDone
if defined __TestArgParsing (
@ECHO ----------------
@ECHO PROCESSED ARGS
@ECHO "%processedArgs%"
@ECHO ----------------
@ECHO UNPROCESSED ARGS
@ECHO "%__UnprocessedBuildArgs%"
@ECHO ----------------
@ECHO __BuildArch=%__BuildArch%
@ECHO __BuildType=%__BuildType%
@ECHO __Exclude=%__Exclude%
@ECHO __RebuildTests=%__RebuildTests%
@ECHO __BuildTestProject=%__BuildTestProject%
@ECHO __BuildTestDir=%__BuildTestDir%
@ECHO __BuildTestTree=%__BuildTestTree%
@ECHO __BuildLogRootName=%__BuildLogRootName%
@ECHO __SkipRestorePackages=%__SkipRestorePackages%
@ECHO __SkipManaged=%__SkipManaged%
@ECHO __SkipTestWrappers=%__SkipTestWrappers%
@ECHO __BuildTestWrappersOnly=%__BuildTestWrappersOnly%
@ECHO __SkipNative=%__SkipNative%
@ECHO __CompositeBuildMode=%__CompositeBuildMode%
@ECHO __TestBuildMode=%__TestBuildMode%
@ECHO __CreatePdb=%__CreatePdb%
@ECHO __CreatePerfmap=%__CreatePerfmap%
@ECHO __CopyNativeTestBinaries=%__CopyNativeTestBinaries%
@ECHO __CopyNativeProjectsAfterCombinedTestBuild=%__CopyNativeProjectsAfterCombinedTestBuild%
@ECHO __SkipGenerateLayout=%__SkipGenerateLayout%
@ECHO __GenerateLayoutOnly=%__GenerateLayoutOnly%
@ECHO __Ninja=%__Ninja%
@ECHO __CMakeArgs=%__CMakeArgs%

EXIT /b 1
)

@if defined _echo @echo on

Expand Down Expand Up @@ -174,7 +227,7 @@ REM Set up the directory for MSBuild debug logs.
set MSBUILDDEBUGPATH=%__MsbuildDebugLogsDir%

set __CommonMSBuildArgs="/p:TargetOS=%__TargetOS%"
set __CommonMSBuildArgs=%__CommonMSBuildArgs% "/p:Configuration=%__BuildType%"
set __CommonMSBuildArgs=%__CommonMSBuildArgs% "/p:Configuration=%__BuildType%" "/p:LibrariesConfiguration=%__BuildType%"
jeffhandley marked this conversation as resolved.
Show resolved Hide resolved
set __CommonMSBuildArgs=%__CommonMSBuildArgs% "/p:TargetArchitecture=%__BuildArch%"

if "%__Mono%"=="1" (
Expand Down Expand Up @@ -315,7 +368,7 @@ echo.
echo Build the CoreCLR tests.
echo.
echo Usage:
echo %0 [option1] [option2] ...
echo %__commandName% [option1] [option2] ...
echo All arguments are optional. Options are case-insensitive. The options are:
echo.
echo.-? -h -help --help: view this message.
Expand Down