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.

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 bootstrapped builds in VS test insertion YAML #69130

Merged
merged 2 commits into from
Aug 1, 2023

Conversation

jcouv
Copy link
Member

@jcouv jcouv commented Jul 20, 2023

In order to get speedometer results on an experimental compiler codegen change, we need to temporarily enable bootstrapping.

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Infrastructure untriaged Issues and PRs which have not yet been triaged by a lead labels Jul 20, 2023
@jcouv jcouv self-assigned this Jul 20, 2023
@jcouv jcouv requested a review from jaredpar July 20, 2023 19:05
@jcouv jcouv marked this pull request as ready for review July 20, 2023 19:05
@jcouv jcouv requested a review from a team as a code owner July 20, 2023 19:05
Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

Talked with Julien offline about this, and I think I'm going to need a complete explanation of why this needs to be in a separate PR that will slow down all roslyn CI for several days.

@jcouv
Copy link
Member Author

jcouv commented Jul 21, 2023

Sounds like we'll have to wait til @jaredpar is back, as I'm not able to provide sufficient details.

@jcouv jcouv marked this pull request as draft July 21, 2023 22:16
@jaredpar
Copy link
Member

... why this needs to be in a separate PR that will slow down all roslyn CI for several days.

How will this slow down roslyn CI? This is the PR official pipeline file. To my understanding that is only used in the following pipeline which is run "on demand" only.

https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_build?definitionId=8972&_a=summary

@333fred
Copy link
Member

333fred commented Jul 31, 2023

The file is named azure-pipelines-pr-validation.yml. If it's not actually for PR validation, maybe the file needs to be renamed.

@333fred
Copy link
Member

333fred commented Jul 31, 2023

I also am still unsure why this needs to be checked in. Why does the compiler itself need to be built with the flag?

@jcouv jcouv marked this pull request as ready for review July 31, 2023 16:53
@jcouv
Copy link
Member Author

jcouv commented Jul 31, 2023

Why does the compiler itself need to be built with the flag?

We want a speedometer result of VS using roslyn bits using the new codegen.
Bootstrapping is what will cause the roslyn bits to use the new codegen. Without bootstrapping, the roslyn bits would still use the old async codegen.

Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

We want a speedometer result of VS using roslyn bits using the new codegen.
Bootstrapping is what will cause the roslyn bits to use the new codegen. Without bootstrapping, the roslyn bits would still use the old async codegen.

Thanks. This, and that the file was not standard ci, were the pieces that I was missing.

@jcouv
Copy link
Member Author

jcouv commented Jul 31, 2023

@jaredpar for review. Thanks

@jaredpar
Copy link
Member

The file is named azure-pipelines-pr-validation.yml. If it's not actually for PR validation, maybe the file needs to be renamed.

I agree the name is confusing and I wish there we had a better one. I'm fine with renaming if @dibarbet agrees.

@dibarbet
Copy link
Member

The file is named azure-pipelines-pr-validation.yml. If it's not actually for PR validation, maybe the file needs to be renamed.

I agree the name is confusing and I wish there we had a better one. I'm fine with renaming if @dibarbet agrees.

I'm fine with renaming, but the pipeline is specifically for validating PRs (against VS). Maybe something like azure-pipelines-test-insertion or something? I'll trust you all to come up with something good :)

@jcouv
Copy link
Member Author

jcouv commented Jul 31, 2023

I'm not planning to rename in this PR (not the intent, I'm just trying to unblock validation of a codegen change), that can be a follow-up.
@jaredpar for a second look. Thanks

@jcouv jcouv changed the title Enable bootstrapped builds Enable bootstrapped builds in VS test insertion YAML Jul 31, 2023
@jaredpar
Copy link
Member

Yeah I don't want to change the name here. Just saying it's fine for another PR.

@jcouv jcouv merged commit 7784555 into dotnet:main Aug 1, 2023
24 checks passed
@jcouv jcouv deleted the bootstrap-build branch August 1, 2023 06:48
@ghost ghost added this to the Next milestone Aug 1, 2023
@jcouv jcouv removed the untriaged Issues and PRs which have not yet been triaged by a lead label Aug 1, 2023
@dibarbet dibarbet removed this from the Next milestone Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants