Skip to content

Fix C# language name in spec #57427

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Nov 15, 2021
Merged

Fix C# language name in spec #57427

merged 2 commits into from
Nov 15, 2021

Conversation

@tmat
Copy link
Member

@tmat tmat commented Oct 27, 2021

No description provided.

@ghost ghost added the Area-Compilers label Oct 27, 2021
@runfoapp runfoapp bot mentioned this pull request Oct 27, 2021
Open
Youssef1313
Youssef1313 reviewed Oct 28, 2021
View reviewed changes
@tmat @Youssef1313
Update docs/features/pdb-compilation-options.md
37e199f 
Co-authored-by: Youssef Victor <youssefvictor00@gmail.com>
@tmat
Copy link
Member Author

tmat commented Nov 8, 2021

@jaredpar PTAL

Also, why do we run CI on doc changes?

@jaredpar
Copy link
Member

jaredpar commented Nov 9, 2021

Also, why do we run CI on doc changes?

Several of our CI legs are marked as required. GitHub interprets this as the legs must pass for the PR to be mergable, not that the legs can't fail. Hence if you disable CI on docs changes the legs don't run and GitHub does not consider the PR to be mergable.

Yes this comment deserves a sad face so here you go 😦

jaredpar
jaredpar reviewed Nov 9, 2021
View reviewed changes
@Youssef1313
Copy link
Member

Youssef1313 commented Nov 9, 2021

Several of our CI legs are marked as required. GitHub interprets this as the legs must pass for the PR to be mergable, not that the legs can't fail. Hence if you disable CI on docs changes the legs don't run and GitHub does not consider the PR to be mergable.

I'm feeling there could be a way (or a hack) to let the required legs "pass" without building/running tests. Some of the ideas:

  • Update the .cmd scripts that're run in CI to detect which files are modified (possibly by git reset --soft followed by git status --porcelain and parse the output. Then, based on the changed files, we either run the script as normal, or return 0. If we returned 0, then GitHub will be convinced that the leg was run and passed.
  • Update the Azure yaml files in a similar manner, instead of using the triggers exclude, we may try wrapping the script in a condition:

image

NOTE: I'm not sure how hard this can get, and whether any of the two options can work. Just sharing my thoughts here :)

@jaredpar
Copy link
Member

jaredpar commented Nov 9, 2021 •
edited
Loading

The runtime repository takes an approach similar to what you are suggesting. Essentially they have a script they run which more / less runs a git status call then sets a number of variables that represent the areas of the code that changed. That lets them condition later runs on items like only run this if libraries changed.

It's necessary for them as they have a giant repo and it pays off massively for them to partition this way. I'm not sure we'd get the same benefits here. Have to weigh it against the cost of having tigers / admins force merge doc changes that run into CI issues.

@tmat tmat enabled auto-merge (squash) November 9, 2021 17:56
jcouv
jcouv approved these changes Nov 15, 2021
View reviewed changes
Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 2)

@tmat tmat merged commit 431f38b into main Nov 15, 2021
@ghost ghost added this to the Next milestone Nov 15, 2021
@jcouv jcouv self-assigned this Nov 15, 2021
333fred added a commit to 333fred/roslyn that referenced this pull request Nov 17, 2021
@333fred
Merge remote-tracking branch 'upstream/main' into ioperation-childope…
97c0f76 
…rations

* upstream/main: (3387 commits)
  Fix ValueTracking for index parameters (dotnet#57727)
  Avoid accessing current assembly identity while reporting an accessibility diagnostics for an inaccessible internal symbol. (dotnet#57783)
  Include a type for NoneOperations in VB, print the type in tests (dotnet#57664)
  Don't throw exceptions for file changes after a project is unloaded
  Check up front for being called to remove more than once
  Fix C# language name in spec (dotnet#57427)
  Add test
  Fix null ref in navbars
  Ensure that getting the checksum for a project cone is resilient to its project references being missing
  Check constraints on lifted operator types (dotnet#57050)
  Adjust tests for Windows 11 changes (dotnet#57678)
  Add comment
  Load SVsShellDebugger before calling IVsSolution.CreateSolution
  Remove extra EnsureEditableDocuments  calls (dotnet#57725)
  Don't show nullable annotation in completion items of static field/property
  Don't analyze local function bodies as though they are top level code (dotnet#57623)
  update error code to fix main break (dotnet#57739)
  Error when ref is used on a parameter or return type of an UnmanagedCallersOnly method (dotnet#57043)
  Simplify code from review
  Fix featureflag name for .net 6 host in UI
  ...
@allisonchou allisonchou modified the milestones: Next, 17.1.P2 Nov 30, 2021
@JoeRobich JoeRobich deleted the dev/tmat/langnamec# branch May 9, 2022 21:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jaredpar jaredpar jaredpar left review comments

@jcouv jcouv jcouv approved these changes

+1 more reviewer

@Youssef1313 Youssef1313 Youssef1313 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@jcouv jcouv

Labels

Area-Compilers Documentation

Projects

None yet

Milestone

17.1.P2

Development

Successfully merging this pull request may close these issues.

6 participants

@tmat @jaredpar @Youssef1313 @jcouv @allisonchou