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

Fix conditions to include suceeded on send to helix inner #45432

Merged
merged 2 commits into from
Dec 2, 2020
Merged

Fix conditions to include suceeded on send to helix inner #45432

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
93a1d60
Fix conditions to include suceeded on send to helix inner
safern Dec 1, 2020
d8fb961
Fix yaml syntax
safern Dec 1, 2020
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
8 changes: 4 additions & 4 deletions eng/pipelines/common/templates/runtimes/send-to-helix-inner-step.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,26 +12,26 @@ steps:
# TODO: Remove and consolidate this when we move to arcade via init-tools.cmd.
Copy link
Member

Choose a reason for hiding this comment

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

unrelated, what is this about?

Copy link
Member

Choose a reason for hiding this comment

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

Not sure if this is the original intent of the comment but send-to-helix-step.yml exists in three versions: I created this particular file by extracting the Helix triggering logic from the CoreCLR version of the script in order to unify the Windows and Unix version; two other versions come from Arcade, they reside at:

https://github.com/dotnet/runtime/blob/master/eng/common/templates/steps/perf-send-to-helix.yml
https://github.com/dotnet/runtime/blob/master/eng/common/templates/steps/send-to-helix.yml

There's not much difference among them and they could be easily unified but I believe that avoiding the code duplication between Windows and Unix requires using a subordinate script (like I did in CoreCLR) and my proposal to take the change to Arcade was not met with too much enthusiasm, see here:

dotnet/arcade#6227

Copy link
Member Author

Choose a reason for hiding this comment

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

To me it seems like we can unify them already? We could use the script extension that we set in xplat-setup and just use single hyphen arguments which are supported, right?

Copy link
Member

Choose a reason for hiding this comment

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

I think you're right that we should be able to unify the Windows and Unix variants in send-to-helix-inner-step.yml. Sadly we cannot use the existing extension switch in xplat-setup though as the Windows variant uses a PowerShell, not a batch script (ps1 instead of cmd) but that's not rocket science to add to xplat-setup as another variant extension. There are also a few additional subtle differences (-ci vs. --ci) hitting the traditional annoyance that there's no easy way to write a conditional expression in YAML scripts and the "step" template doesn't support variable definitions.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe we can just add support for single hyphen arguments in the msbuild.sh script as @ViktorHofer did a while ago for the build.sh script in arcade. This was the exact reason why we added that support, to be able to have commands that work on both Unix and Windows scripts:

runtime/eng/common/build.sh

Line 84 in 8bc9834

opt="$(echo "${1/#--/-}" | awk '{print tolower($0)}')"

- powershell: $(Build.SourcesDirectory)\eng\common\build.ps1 -ci ${{ parameters.restoreParams }}
displayName: Restore blob feed tasks (Windows)
condition: ${{ and(ne(parameters.condition, false), ne(parameters.restoreParams, '')) }}
condition: ${{ and(succeeded(), ne(parameters.condition, false), ne(parameters.restoreParams, '')) }}

- powershell: $(Build.SourcesDirectory)\eng\common\msbuild.ps1 -ci ${{ parameters.sendParams }}
displayName: ${{ parameters.displayName }} (Windows)
condition: ${{ and(ne(parameters.condition, false), ne(parameters.sendParams, '')) }}
condition: ${{ and(succeeded(), ne(parameters.condition, false), ne(parameters.sendParams, '')) }}
env: ${{ parameters.environment }}
continueOnError: ${{ parameters.shouldContinueOnError }}

- ${{ if ne(parameters.osGroup, 'windows') }}:
# TODO: Remove and consolidate this when we move to arcade via init-tools.sh.
- script: $(Build.SourcesDirectory)/eng/common/build.sh --ci ${{ parameters.restoreParams }}
displayName: Restore blob feed tasks (Unix)
condition: ${{ and(ne(parameters.condition, false), ne(parameters.restoreParams, '')) }}
condition: ${{ and(succeeded(), ne(parameters.condition, false), ne(parameters.restoreParams, '')) }}
${{ if eq(parameters.osGroup, 'FreeBSD') }}:
env:
# Arcade uses this SDK instead of trying to restore one.
DotNetCoreSdkDir: /usr/local/dotnet

- script: $(Build.SourcesDirectory)/eng/common/msbuild.sh --ci ${{ parameters.sendParams }}
displayName: ${{ parameters.displayName }} (Unix)
condition: ${{ and(ne(parameters.condition, false), ne(parameters.sendParams, '')) }}
condition: ${{ and(succeeded(), ne(parameters.condition, false), ne(parameters.sendParams, '')) }}
env: ${{ parameters.environment }}
continueOnError: ${{ parameters.shouldContinueOnError }}