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

Include CodeStyle targets files even when not enforcing code style in build #38968

Merged
merged 1 commit into from
Mar 4, 2024

Conversation

mavasani
Copy link
Contributor

Please see the PR description in dotnet/roslyn#72238 (comment), which gives a background on this fix. This change along with that Roslyn PR is needed for fixing the regression in dotnet/roslyn#72094

… build

Please see the PR description in dotnet/roslyn#72238 (comment), which gives a background on this fix. This change along with that Roslyn PR is needed for fixing the regression in dotnet/roslyn#72094
@arunchndr
Copy link
Member

@marcpopMSFT this one blocks another change we might need to present to servicing - dotnet/roslyn#72238. Could we get an approval from your side and merge?

@baronfel baronfel requested a review from a team February 26, 2024 21:30
@baronfel
Copy link
Member

@dsplaisted / @joeloff any problems with merging this one?

@joeloff
Copy link
Member

joeloff commented Feb 26, 2024

After reading the issue/comments that were linked, this seems to be something we need. Is this only applicable to newer versions or will these need to be ported to older releases of the SDK as part of servicing?

@mavasani
Copy link
Contributor Author

After reading the issue/comments that were linked, this seems to be something we need. Is this only applicable to newer versions or will these need to be ported to older releases of the SDK as part of servicing?

Basically any backport of the Roslyn change will be a no-op in VS unless the underlying SDK installed has this change. So, if we are backporting the Roslyn change to VS 17.10, we want to at least backport this SDK change into whichever .NET SDK version is installed as part of 17.10

@cremor
Copy link

cremor commented Feb 27, 2024

So, if we are backporting the Roslyn change to VS 17.10, we want to at least backport this SDK change into whichever .NET SDK version is installed as part of 17.10

I'd have hoped that this is released as a VS 17.9.x bugfix?

@mavasani
Copy link
Contributor Author

So, if we are backporting the Roslyn change to VS 17.10, we want to at least backport this SDK change into whichever .NET SDK version is installed as part of 17.10

I'd have hoped that this is released as a VS 17.9.x bugfix?

Tagging @arkalyanms. I believe this will depend on how many users are hitting this, but I am fine if we feel this should be backported to 17.9. dotnet/roslyn#52991 (comment) has suggestions on how to clean up your editorconfig to avoid hitting this case

@mavasani
Copy link
Contributor Author

Can someone please help me with the unrelated build failure on this PR?

@baronfel
Copy link
Member

This test has been failing consistently across a number of branches - I believe we have a team member trying to root cause it currently.

@mavasani
Copy link
Contributor Author

This test has been failing consistently across a number of branches - I believe we have a team member trying to root cause it currently.

@baronfel Can this PR be merged overriding the failing test leg?

@dsplaisted
Copy link
Member

@baronfel Can this PR be merged overriding the failing test leg?

I am investigating the failing test, but if we can consider disabling the test if we need to get this PR merged quickly. That would be preferrable to having the test consistently failing.

@mavasani
Copy link
Contributor Author

@baronfel Can this PR be merged overriding the failing test leg?

I am investigating the failing test, but if we can consider disabling the test if we need to get this PR merged quickly. That would be preferrable to having the test consistently failing.

I think that might help as we are considering backporting this fix. I'll let @arkalyanms make a call a here.

@dsplaisted
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mavasani mavasani merged commit 4f79fda into main Mar 4, 2024
16 checks passed
@mavasani mavasani deleted the mavasani-patch-1 branch March 4, 2024 20:36
@cremor
Copy link

cremor commented Mar 6, 2024

Tagging @arkalyanms. I believe this will depend on how many users are hitting this, but I am fine if we feel this should be backported to 17.9

@arkalyanms Could you please comment on if this (and dotnet/roslyn#72238) will be backported to 17.9?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Infrastructure untriaged Request triage from a team member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants