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

Remove target that warns that user needs to upgrade to a newer SDK NetAnalyzers version #7040

Merged
merged 1 commit into from
Nov 22, 2023

Conversation

carlossanlop
Copy link
Member

Addresses part of the problem reported in #6878 by deleting the target that we have determined does not add enough value.

@carlossanlop carlossanlop self-assigned this Nov 17, 2023
@carlossanlop carlossanlop requested a review from a team as a code owner November 17, 2023 17:22
@carlossanlop
Copy link
Member Author

@mavasani Two questions:

  • Can you tell me how to test that this works?
  • Should I backport this to 8.0?

Copy link

codecov bot commented Nov 17, 2023

Codecov Report

Merging #7040 (bb4b456) into main (e600e79) will decrease coverage by 0.01%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7040      +/-   ##
==========================================
- Coverage   96.43%   96.43%   -0.01%     
==========================================
  Files        1412     1412              
  Lines      336528   336528              
  Branches    11119    11119              
==========================================
- Hits       324528   324519       -9     
- Misses       9198     9206       +8     
- Partials     2802     2803       +1     

@mavasani
Copy link
Contributor

@carlossanlop There isn’t a non-trivial way to test this as local build will generate a 9.0 versioned preview NuGet package, which will report a warning on a higher versioned analyzer assembly/SDK, which isn’t available yet. I think it’s fine to just delete this target without any manual testing.

Regarding whether we should backport this to 8.0 branch - backporting will help when we release a new .NET SDK which has a higher versioned NetAnalyzers assembly whose corresponding NuGet package isn’t published simultaneously on NuGet.org. I wouldn’t say it is critical, especially if we remember publishing the NuGet package along with the SDK release, but I won’t push back if you feel more comfortable backporting

@mavasani
Copy link
Contributor

@carlossanlop Is this good to be merged?

@carlossanlop carlossanlop merged commit 23bb98b into dotnet:main Nov 22, 2023
14 checks passed
@carlossanlop carlossanlop deleted the RemoveFlaggingTarget branch November 22, 2023 16:52
@carlossanlop
Copy link
Member Author

/backport to release/8.0.2xx

Copy link
Contributor

Started backporting to release/8.0.2xx: https://github.com/dotnet/roslyn-analyzers/actions/runs/6960400450

Copy link
Contributor

@carlossanlop backporting to release/8.0.2xx failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Remove flagging target
Using index info to reconstruct a base tree...
M	src/Tools/GenerateDocumentationAndConfigFiles/Program.cs
Falling back to patching base and 3-way merge...
Auto-merging src/Tools/GenerateDocumentationAndConfigFiles/Program.cs
CONFLICT (content): Merge conflict in src/Tools/GenerateDocumentationAndConfigFiles/Program.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Remove flagging target
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

Copy link
Contributor

@carlossanlop an error occurred while backporting to release/8.0.2xx, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

@carlossanlop
Copy link
Member Author

Oops, just realized the backport failed. I'll need to create the PR manually.

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.

2 participants