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
Add new official build for runtimelab #2563
Conversation
FYI for msft employees, internal run ID is 2438214 |
Succeeded, but it did not publish the packages (e.g. https://dev.azure.com/dnceng/public/_artifacts/feed/dotnet-experimental/NuGet/runtime.browser-wasm.Microsoft.DotNet.ILCompiler.LLVM/) |
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.
Some high level questions.
What is it exactly that is required of the "official" .yml
(that wasn't previously)?
Is it required to have a separate .yml
instead of using runtimelab.yml
with isOfficialBuild: true
, or is it a code organization choice? Can/should we delete isOfficialBuild == true
paths from runtimelab.yml
now (downstream and upstream)?
extends: | ||
template: /eng/pipelines/common/templates/pipeline-with-resources.yml@self | ||
parameters: | ||
isOfficialBuild: true |
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.
Below in the file, there are places that check isOfficialBuild
instead of hardcoding it. What is the intent behind that?
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.
It is likely that some are auto-generated from the conversion script, and some were just written by me by hand. Thus you have me hardcoding things to true (because this is the official build) and the script not knowing what to do and pulling the variable.
templatePath: 'templates-official' | ||
timeoutInMinutes: 300 | ||
isOfficialBuild: ${{ variables.isOfficialBuild }} | ||
testGroup: innerloop |
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.
testGroup: innerloop
Where is this parameter defined? I don't see it in https://github.com/dotnet/runtimelab/blob/feature/NativeAOT-LLVM/eng/pipelines/common/global-build-job.yml.
${{ if eq(variables.isOfficialBuild, true) }}: | ||
buildArgs: -s clr.aot+libs+nativeaot.packages -c $(_BuildConfig) |
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 is an odd placement for buildArgs
. I wonder if the indentation works out correctly.
jobTemplate: /eng/pipelines/common/global-build-job.yml | ||
buildConfig: Release | ||
platforms: | ||
- windows_arm64 |
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.
windows_x64
?
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.
I will add that.
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.
Note that we don't really need windows_arm64
here (and probably don't want it, since it doesn't get tested on PR CI).
Yup, expected -- it's branch gated so we won't get any new packages until this is merged. |
In short, we need to import an internal template that sets policy.
It's possible to share, but hard. The internal template requires certain tasks to be used, and those tasks aren't available in the public template. The current guidance is to split to internal and external.
Yes, although that's not strictly necessary. |
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.
Thank you for the explanation, it makes sense to me.
I have added a couple suggestions that hardcode the official build parameter and make the code a bit simpler.
Modulo that, LGTM. We can see to the runtimelab.yml
changes separately.
I am assuming a version of this will go upstream. Is that correct?
jobTemplate: /eng/pipelines/common/global-build-job.yml | ||
buildConfig: Release | ||
platforms: | ||
- windows_arm64 |
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.
Note that we don't really need windows_arm64
here (and probably don't want it, since it doesn't get tested on PR CI).
Co-authored-by: SingleAccretion <62474226+SingleAccretion@users.noreply.github.com>
Co-authored-by: SingleAccretion <62474226+SingleAccretion@users.noreply.github.com>
Co-authored-by: SingleAccretion <62474226+SingleAccretion@users.noreply.github.com>
Co-authored-by: SingleAccretion <62474226+SingleAccretion@users.noreply.github.com>
Co-authored-by: SingleAccretion <62474226+SingleAccretion@users.noreply.github.com>
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.
LGTM, thanks
I do not think we should be doing that. It makes official build flakier and the failures in official build are impossible for non-Microsoft people to see and debug. |
The current config is running tests, so I can instead just skip testing entirely. |
Yes, that would be better. |
Sorry, my mistake. |
np, simple change. Looks like the internal build passed. |
|
This brings in the same 1ES official build templates that are in runtime and are now required for all internal builds.