Skip to content

Conversation

jnm2
Copy link
Contributor

@jnm2 jnm2 commented May 31, 2025

Fixes the following issues:

  1. MSBuildLocator isn't needed (MSBuildWorkspace methods can't be called in the same method body as MSBuildLocator.RegisterInstance roslyn#78478 (comment)) and it is failing on my machine where I have only VS Preview installed. I believe MSBuildLocator by default doesn't see preview installations.

  2. Add launch settings to the tools so that they are runnable immediately after cloning the repo by pressing F5 in Visual Studio. The launch settings contain the same arguments that are used for the workflow runs.

  3. Fix the broken example tester workflow. Update DocumentFormat.OpenXml and 6 other dependencies #1334 (review) caused it to start failing for the AnonymousFunctionsConv1 example because it pulled in Roslyn v4.14, which no longer reports a CS1661 diagnostic at the same location as a CS1678 error. (Discussed at https://github.com/dotnet/roslyn/pull/75400/files#r1866802220.) The workflow did not run for that PR, so this was not discovered until unrelated PRs started coming through.

  4. Add more paths to the example tester workflow trigger to catch issues like the above next time.

Also, this PR adds path filters for the other PR-triggered workflows so that they only run when files change which can currently affect them.

@BillWagner You had gitignored the launch profiles; what do you think about reversing this?

@jnm2 jnm2 marked this pull request as draft June 1, 2025 20:06
@jnm2
Copy link
Contributor Author

jnm2 commented Jun 1, 2025

It's possible that removing the MSBuildLocator call will cause the example tester task (not triggered here) to fail with one example changing its expected warnings. I'd like to add to the GitHub Actions filter for this PR to cause the tester to run when the tester code is changed.

@jnm2
Copy link
Contributor Author

jnm2 commented Jun 2, 2025

Sure enough, it's the removal of MSBuildLocator that causes the AnonymousFunctionsConv1 example to start producing only a single CS1661 error instead in place of two of the CS1661 errors that are being expected for that example.

I'll try to collect compiler versions before and after and see what's up.

@jnm2
Copy link
Contributor Author

jnm2 commented Jun 2, 2025

The compiler version is the same both before and after the change to stop using MSBuildLocator: 4.14.0-3.25262.10 (8edf7bcd)

It also turns out that this failure is unrelated to any of my PRs; the draft-v8 branch is currently broken for the test extractor run. The last PR that was merged showed this same failure: https://github.com/dotnet/csharpstandard/actions/runs/15323346177/job/43111801142

@jnm2
Copy link
Contributor Author

jnm2 commented Jun 2, 2025

I bisected and found the cause of the failure at #1334 (review). I'll resolve both issues in this PR: the lack of workflow trigger so that the workflow did not run for that PR, and the failure itself.

@jnm2 jnm2 changed the title Make tools runnable out of the box, matching .sh arguments Fix broken test extractor workflow on draft-v8, and make tools runnable out of the box Jun 2, 2025
@jnm2 jnm2 changed the title Fix broken test extractor workflow on draft-v8, and make tools runnable out of the box Fix broken example tester workflow on draft-v8, and make tools runnable out of the box Jun 2, 2025
@jnm2
Copy link
Contributor Author

jnm2 commented Jun 2, 2025

The failure was due to an intentional change in the Roslyn compiler v4.14, discussed at https://github.com/dotnet/roslyn/pull/75400/files#r1866802220. It doesn't make any difference for the purposes of the code example in the standard. I updated the expected diagnostics.

@jnm2 jnm2 marked this pull request as ready for review June 2, 2025 01:51
Copy link
Contributor

@jskeet jskeet left a comment

Choose a reason for hiding this comment

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

LGTM, but would be good to get @BillWagner to check as well.

Copy link
Member

@BillWagner BillWagner left a comment

Choose a reason for hiding this comment

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

This LGTM as well. Thanks a lot @jnm2 This is really helpful.

Let's :shipit:

@BillWagner BillWagner merged commit db36646 into draft-v8 Jun 5, 2025
10 checks passed
@jnm2 jnm2 deleted the jnm2/tools branch June 5, 2025 23:10
@jnm2
Copy link
Contributor Author

jnm2 commented Jun 5, 2025

Some of the new path filters may need to be reverted on the workflows which are referenced by required checks: #1342

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants