Changing dev workflow scripts to use the Run Command Tool #10231
Conversation
|
@dotnet-bot test ci please |
|
@maririos We can do some testing of this by running the generated jobs from @dotnet-bot test ci please. |
|
@dotnet-bot test ci please |
| @@ -0,0 +1,2 @@ | |||
| @call %~dp0run.cmd build-managed -nodoReuse -binclashWindows %* | |||
There was a problem hiding this comment.
Do we have documentation for the build-managed command? The docs only refer to build to my knowledge at this point.
There was a problem hiding this comment.
build-native and build-managed are a detail of implementation in this repo.
For the common dev workflow Build is the task that encapsulates this work and that is why Build is calling them.
If all of our .NET Core repos need this behavior, then we can update the dev workflow, but I don't think this is true.
There was a problem hiding this comment.
Maybe I'm misunderstanding how this is used. I saw the call to run build-managed and assumed it was now a top level supported command. I completely agree it's an implementation detail that shouldn't be exposed.
But that also makes me not understand this sample. If it's an implementation detail why is it going through run.cmd at all? Why not skip the middle man and go directly to a build-managed command. What value is run.cmd providing here?
There was a problem hiding this comment.
Each command needs one tool to run. Build-managed and Build native used different tools so we divided them into different commands.
The developers are not going to call run build-native but will call build.cm/sh which is what we want.
If they want to be more specific and do special things for only one type of build, they have the possibility.
|
@mmitche just explained to me how to test it, so I'm doing it right now. I'll update the PR later with more information about the tests |
|
@dotnet-bot test ci please |
| echo /s - Deletes the untracked files under src directory (git clean src -xdf). | ||
| echo /e - Kills and/or stops the processes that are still running, for example VBCSCompiler.exe | ||
| echo /all - Combines all of the above. | ||
| echo -b - Deletes the binary output directory. It uses the Run Command Tool to do it. |
There was a problem hiding this comment.
Can the usage come from run? Then only "all" needs to be handled by this script. The usage I think should just say what it is doing and there isn't any need for mentioning the run command tool.
There was a problem hiding this comment.
Unless I'm missing something it doesn't look like the usage is coming from run, it is still in the script.
There was a problem hiding this comment.
I was thinking on how to show the -all option to the user in a way that will fit with the format the run tool uses for help, but the only way that I see is to leave it in the script, but not print it. So no ones is calling the Usage function anymore.
|
@dotnet-bot test ci please |
| fi | ||
|
|
||
| lowerI="$(echo $1 | awk '{print tolower($0)}')" | ||
| case $lowerI in |
There was a problem hiding this comment.
Is there a reason this cannot be in dir.props and not in the sh script? I would hope all we would need in the sh script is a placeholder for the OS to pass to msbuild.
There was a problem hiding this comment.
If we do it in dir.props then these properties would be overridable, so if we decide to do it there, then we should just make sure that the values are correct.
There was a problem hiding this comment.
I also want it to be in dir.props. There is a section where we are already defining TestNugetRuntimeId but as @joperezr pointed out, when calling this variables from the scripts they overwrite all the places were we have them. Just putting them in dir.props is not the same behavior.
I haven't test it to see if it would be a breaking change. But for a next iteration we could do it.
|
This is looking good. Lets make sure we don't break the builds and get it merged. |
|
@dotnet-bot test ci please |
|
After testing the changes in the CI, only one job is failing: linuxarmemulator_cross. |
|
@sjsujin Take a look at this, Please. |
|
@dotnet-bot test Innerloop Linux ARM Emulator Release Cross Build |
Is it correct? Until now, I think that I have to use "@dotnet-bot test ... please" as follows. |
Yes. If you go to the details of that test you'll see the error and the log I've sent you on email. |
|
@dotnet-bot test ci please |
|
I updated the documentation of CoreFx on how to build and how to use the dev workflow scripts with PR #10231 |
|
@dotnet-bot test Innerloop Linux ARM Emulator Release Cross Build |
|
/CC @prajwal-aithal |
| ROOTFS_DIR="$__ARMRootfsMountPath" CPLUS_INCLUDE_PATH=$LINUX_ARM_INCPATH CXXFLAGS=$LINUX_ARM_CXXFLAGS ./build.sh $__buildArch clean cross $__verboseFlag $__buildConfig skiptests | ||
| ROOTFS_DIR="$__ARMRootfsMountPath" CPLUS_INCLUDE_PATH=$LINUX_ARM_INCPATH CXXFLAGS=$LINUX_ARM_CXXFLAGS | ||
| BUILD_NATIVE="$ROOTFS_DIR" ./build-native.sh -buildArch=$__buildArch -$__buildConfig -- cross $__verboseFlag | ||
| BUILD_MANAGED="$ROOTFS_DIR" ./build-managed.sh -buildArch=$__buildArch -$__buildConfig -skipTests | ||
|
|
There was a problem hiding this comment.
In this case, ROOTFS_DIR is empty in 'build-native.sh' and 'build-managed.sh'
So I suggest below each lines. From ROOTFS_DIR to verboseFlag have to the one line.
ROOTFS_DIR="$__ARMRootfsMountPath" CPLUS_INCLUDE_PATH=$LINUX_ARM_INCPATH CXXFLAGS=$LINUX_ARM_CXXFLAGS BUILD_NATIVE="$ROOTFS_DIR" ./build-native.sh -buildArch=$__buildArch -$__buildConfig -- cross clean $__verboseFlag
ROOTFS_DIR="$__ARMRootfsMountPath" CPLUS_INCLUDE_PATH=$LINUX_ARM_INCPATH CXXFLAGS=$LINUX_ARM_CXXFLAGS BUILD_MANAGED="$ROOTFS_DIR" ./build-managed.sh -buildArch=$__buildArch -$__buildConfig -skipTests
And I wonder where 'BUILD_NATIVE' and 'BUILD_MANAGED' are used in. I cannot find the place using those values.
This PR updates the dev workflow scripts that we have, to use the Run Command Tool.
As PR dotnet/coreclr#6400 states, "This is a first step to move towards consuming the run tool to synchronize the dev workflow across the dotnet repos."
For more information about the dev workflow please read here.
For information about the Run Command Tool, please read here.
We are working on updating the documentation of CoreFx too.
cc: @Priya91 @weshaggard @jaredpar @markwilkie @Chrisboh @joperezr @sokket @TyOverby
FYI: Because the changes involve updating the
netci.groovyscript, the CI builds are expected to fail. Once we merge the changes, we can validate the change. I did tests locally in a Windows and an Ubuntu machine.