-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Enable Mono build in CI #1934
Enable Mono build in CI #1934
Conversation
…to enable-mono-ci
# Conflicts: # eng/pipelines/libraries/base-job.yml
# alpine coreclr cmake errors on newer builds | ||
${{ if eq(parameters.runtimeFlavor, 'mono') }}: | ||
image: alpine-3.9-WithNode-0fc54a3-20200121145958 | ||
${{ if ne(parameters.runtimeFlavor, 'mono') }}: |
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.
Can we open an issue for this and investigate? We should try to be consistent with the docker images we build with.
Also, instead of using != mono
I would change this to == coreclr
so that it is easier to read.
${{ if ne(parameters.runtimeFlavor, 'mono') }}: | |
${{ if eq(parameters.runtimeFlavor, 'coreclr') }}: |
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.
Yes I'll file an issue for that as it's highly concerning. Just hadn't had time yet.
@@ -50,6 +50,13 @@ jobs: | |||
- name: ROOTFS_DIR | |||
value: ${{ parameters.jobParameters.crossrootfsDir }} | |||
|
|||
- ${{ if ne(parameters.jobParameters.runtimeFlavor, '') }}: |
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.
runtimeFlavor should always be passed down as a jobParameter
since platform-matrix is passing it down always, so no need to have this empty check any more.
@@ -80,7 +81,7 @@ jobs: | |||
|
|||
- ${{ if eq(parameters.osGroup, 'OSX') }}: | |||
- script: | | |||
brew install pkgconfig icu4c openssl | |||
brew install pkgconfig icu4c openssl autoconf automake libtool pkg-config python3 |
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.
why do we need these? This is building libraries, not mono.
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.
cc @akoeplinger
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.
We probably don't. I was just adding dependencies where it seemed fit and didn't notice this is only used for libraries build.
- template: xplat-pipeline-job.yml | ||
parameters: | ||
buildConfig: ${{ parameters.buildConfig }} | ||
_BuildConfig: ${{ parameters.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 parameter is not used in mono/xplat-pipeline-job.yml, not even defined as a parameter in the template.
osSubgroup: ${{ parameters.osSubgroup }} | ||
helixType: 'build/product/' | ||
enableMicrobuild: true | ||
stagedBuild: ${{ parameters.stagedBuild }} |
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.
stagedBuild
is not necessary at all, right?
container: ${{ parameters.container }} | ||
condition: ${{ parameters.condition }} | ||
dependsOn: | ||
- ${{ if ne(parameters.stagedBuild, 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.
I don't think this will ever be 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.
Right, this is a copy from coreclr. I assumed it will always be true but didn't change it as this would need to be cleaned-up in coreclr as well and I wanted to minimize risk.
@@ -0,0 +1,111 @@ | |||
parameters: |
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 seems very similar to coreclr's xplat-job template, can we share them? Maybe move coreclr's to common.
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.
See answer below.
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.
@@ -0,0 +1,67 @@ | |||
parameters: |
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.
Instead of introducing another template, can we use coreclr's and move it to common?
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 tried to share them but there were too many differences in paths which weren't easy to fix. We should file an issue to consolidate runtime (coreclr & mono) infrastructure files where possible (not just yml).
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.
Sounds good. Let's please open that issue as it seems overkill to have this duplicate templates (maintaining multiple yml files is painful).
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.
- eng/pipelines/libraries/* | ||
- subset: mono | ||
include: | ||
- src/libraries/System.Private.CoreLib/* |
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.
Does mono depend on the native libraries? src/libraries/Native
?
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.
for building mono? I don't think so. cc @akoeplinger
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.
The Mono build doesn't depend on src/libraries/Native
.
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.
but what about the tests?
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.
We'll enable Mono runtime tests at a later point so that's not an issue for now.
|
||
<!-- Set default Configuration and Platform --> | ||
<PropertyGroup> | ||
<OSGroup Condition="'$(OSGroup)' == '' and '$([MSBuild]::IsOSPlatform(Windows))' == 'true'">Windows_NT</OSGroup> |
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.
Can this instead be shared by libraries, coreclr and mono?
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.
not until we have merged @Anipik's configuration system PR.
|
||
<PropertyGroup> | ||
<LangVersion>8.0</LangVersion> | ||
<UseSharedCompilation>true</UseSharedCompilation> |
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.
Aren't this properties defined in the root, Directory.Build.props
? Should we instead put them there?
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 already removed bunch of duplicate properties, I don't plan to remove more as part of this PR. I have an open PR for the managed side of coreclr in which I will include changes for mono as well.
testScope: innerloop | ||
liveRuntimeBuildConfig: release | ||
dependsOnTestBuildConfiguration: ${{ variables.debugOnPrReleaseOnRolling }} | ||
dependsOnTestArchitecture: 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.
Should this be conditioned to when mono or libraries is changed?
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 don't think we want to run this tests when only coreclr is touched or when only installer is touched. That said, I believe the build of mono release should also have the same condition as this step.
Follow-up to dotnet#1934 Also disables one test that seems to be flaky on Mono.
Follow-up to #1934 Also disables one test that seems to be flaky on Mono.
Fixes #1932