This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
Treat CLRTestExecutionArguments as an array in generated Bash scripts #22435
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@dotnet-bot help |
|
Welcome to the dotnet/coreclr Perf help The following is a list of valid commands on this PR. To invoke a command, comment the indicated phrase on the PR The following commands are valid for all PRs and repositories. Click to expand
The following jobs are launched by default for each PR against dotnet/coreclr:master. Click to expand
The following optional jobs are available in PRs against dotnet/coreclr:master. Click to expand
Have a nice day! |
|
Welcome to the dotnet/coreclr Repository The following is a list of valid commands on this PR. To invoke a command, comment the indicated phrase on the PR The following commands are valid for all PRs and repositories. Click to expand
The following jobs are launched by default for each PR against dotnet/coreclr:master. Click to expand
The following optional jobs are available in PRs against dotnet/coreclr:master. Click to expand
Have a nice day! |
|
Welcome to the dotnet/coreclr Repository The following is a list of valid commands on this PR. To invoke a command, comment the indicated phrase on the PR The following commands are valid for all PRs and repositories. Click to expand
The following jobs are launched by default for each PR against dotnet/coreclr:master. Click to expand
The following optional jobs are available in PRs against dotnet/coreclr:master. Click to expand
Have a nice day! |
|
@dotnet-bot test Ubuntu16.04 arm64 Cross Checked normal Build and Test |
|
@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test |
|
@dotnet/jit-contrib PTAL |
|
cc @janvorli |
janvorli
reviewed
Feb 6, 2019
| @@ -255,8 +255,8 @@ $(BashCLRTestLaunchCmds) | |||
| if [ ! -z ${RunCrossGen+x} ]%3B then | |||
| TakeLock | |||
| fi | |||
| echo $_DebuggerFullPath $(_CLRTestRunFile) $ExePath $CLRTestExecutionArguments | |||
| $_DebuggerFullPath $(_CLRTestRunFile) $ExePath $CLRTestExecutionArguments | |||
| echo $_DebuggerFullPath $(_CLRTestRunFile) $ExePath %24(printf "'%s' " "${CLRTestExecutionArguments[@]}") | |||
There was a problem hiding this comment.
I wonder what is the reason for using this obscure way of printing the arguments? Wouldn't just plain ${CLRTestExecutionArguments} work?
There was a problem hiding this comment.
I think it doesn't work for strings with spaces and empty string:
For example,
echesako@echesako4:~$ CLRTestExecutionArguments=(" ")
echesako@echesako4:~$ echo "CLRTestExecutionArguments=${CLRTestExecutionArguments}"
CLRTestExecutionArguments=
echesako@echesako4:~$ echo CLRTestExecutionArguments=$(printf "'%s' " "${CLRTestExecutionArguments}")
CLRTestExecutionArguments=' '
echesako@echesako4:~$ CLRTestExecutionArguments=("")
echesako@echesako4:~$ echo "CLRTestExecutionArguments=${CLRTestExecutionArguments}"
CLRTestExecutionArguments=
echesako@echesako4:~$ echo CLRTestExecutionArguments=$(printf "'%s' " "${CLRTestExecutionArguments}")
CLRTestExecutionArguments=''
There was a problem hiding this comment.
Maybe there is a simpler way of doing this?
There was a problem hiding this comment.
My original intention was to echo an exact command that can be copy-pasted to repro the test run
There was a problem hiding this comment.
For example, /opt/code/bin/tests/Linux.x64.Checked/Tests/Core_Root/corerun ThreadStartString.exe '' as opposed to /opt/code/bin/tests/Linux.x64.Checked/Tests/Core_Root/corerun ThreadStartString.exe
There was a problem hiding this comment.
How about just
echo "CLRTestExecutionArguments='${CLRTestExecutionArguments[@]}'"Seems to work for both cases you've mentioned
There was a problem hiding this comment.
Btw, your way doesn't seem to work for the case of three arguments:
export CLRTestExecutionArguments=(a b c)
echo CLRTestExecutionArguments=$(printf "'%s' " "${CLRTestExecutionArguments}")
Output:
CLRTestExecutionArguments='a'There was a problem hiding this comment.
Oh, nice! Thank you! I will simplify this part
There was a problem hiding this comment.
I made a typo in my example, the expression should be echo CLRTestExecutionArguments=$(printf "'%s' " "${CLRTestExecutionArguments[@]}"):
echesako@echesako4:~$ CLRTestExecutionArguments=(a b c)
echesako@echesako4:~$ echo CLRTestExecutionArguments=$(printf "'%s' " "${CLRTestExecutionArguments[@]}")
CLRTestExecutionArguments='a' 'b' 'c'
There was a problem hiding this comment.
Ah, so you wanted to print each of the arguments quoted. Then I don't know of different way from what you did.
|
Rebased to resolve conflicts |
janvorli
approved these changes
Feb 6, 2019
|
@dotnet-bot test Windows_NT arm64 Cross Checked Innerloop Build and Test @dotnet-bot test Ubuntu16.04 arm64 Cross Checked normal Build and Test |
|
@dotnet-bot test OSX10.12 x64 Checked Build and Test |
picenka21
pushed a commit
to picenka21/runtime
that referenced
this pull request
Feb 18, 2022
…stExecutionArgumentsAsAnArrayInBash Treat CLRTestExecutionArguments as an array in generated Bash scripts Commit migrated from dotnet/coreclr@f340a5f
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes the following issues with generated test runners (Bash scripts):
CLRTestKind="RunOnly"during the test run another script got called herecoreclr/tests/src/CLRTest.Execute.Bash.targets
Lines 266 to 274 in a3073f2
Since calling /bin/sh directly does not obey a shebang this produces the following error
since shopt is Bash builtin.
Instead we should call with /usr/bin/env bash.
It turns out that this is due to $(CLRTestExecutionArguments) being treated as a string inside the Bash script:
coreclr/tests/src/CLRTest.Execute.Bash.targets
Line 117 in a3073f2
And in this example, since $CLRTestExecutionArguments contains only one space character was simply ignored during the call:
coreclr/tests/src/CLRTest.Execute.Bash.targets
Lines 258 to 259 in a3073f2
On Windows the tests passes with the following output:
<CLRTestExecutionArguments>""</CLRTestExecutionArguments>was treated as a string containing two double-quote characters:while on Windows the test runs with an empty string as an argument:
I propose a solution to treat $(CLRTestExecutionArguments) as an array to fix both 2) and 3).
The only problem here that there is no elegant way to pass an array variable $CLRTestExecutionArguments between two scripts. Instead I decided to pass these values to the parent script via command line.
After the change:
ThreadStartString_1 passes
ThreadStartString_2 behaves as a Windows counterpart
Closes https://github.com/dotnet/coreclr/issues/3627