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

Create reference point for v4.0.0 rc4 #811

Merged
merged 8 commits into from
Feb 17, 2023
Merged

Conversation

shaopeng-gh
Copy link
Collaborator

@shaopeng-gh shaopeng-gh commented Feb 16, 2023

Create reference point for v4.0.0 rc4 without actual release.

@@ -4,9 +4,9 @@ namespace Microsoft.CodeAnalysis.IL
{
public static class VersionConstants
Copy link
Collaborator Author

@shaopeng-gh shaopeng-gh Feb 16, 2023

Choose a reason for hiding this comment

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

VersionConstants

I found this file, I think we should also change it, if so we should add in the Release-Process md file #Resolved

Copy link
Collaborator

Choose a reason for hiding this comment

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

@shaopeng-gh Agree. Please update the release process.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated


set MAJOR=2
Copy link
Collaborator Author

@shaopeng-gh shaopeng-gh Feb 16, 2023

Choose a reason for hiding this comment

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

2

I think we should have changed to 4 before this PR. #Resolved

Copy link
Collaborator

Choose a reason for hiding this comment

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

@shaopeng-gh I am confused at this point. The current internal version is 4.0.0-rc3 correct? Then why should we have changed it to 4 before this PR? It seems what you have done here is correct.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, this was missed in the previous PR, we should have changed it at that time.

@suvamM
Copy link
Collaborator

suvamM commented Feb 16, 2023

@shaopeng-gh Please don't leave the PR description blank.


In reply to: 1433423241


set MAJOR=2
Copy link
Collaborator

Choose a reason for hiding this comment

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

@shaopeng-gh I am confused at this point. The current internal version is 4.0.0-rc3 correct? Then why should we have changed it to 4 before this PR? It seems what you have done here is correct.

@@ -99,7 +99,7 @@ public void AnalyzingTarget(IAnalysisContext context)
{
}

public void Log(ReportingDescriptor rule, Result result)
public void Log(ReportingDescriptor rule, Result result, int? extensionIndex = null)
Copy link
Collaborator

@suvamM suvamM Feb 16, 2023

Choose a reason for hiding this comment

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

@shaopeng-gh What does this change do? Please provide some additional details via PR comments. #Resolved

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is due to a breaking change that add a optional parameter:
BRK: Add ToolComponent argument to IAnalysisLogger.Log(ReportingDescriptor, Result) method. microsoft/sarif-sdk#2611

@@ -4,9 +4,9 @@ namespace Microsoft.CodeAnalysis.IL
{
public static class VersionConstants
Copy link
Collaborator

Choose a reason for hiding this comment

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

@shaopeng-gh Agree. Please update the release process.

## **v4.0.0-rc5** UNRELEASED

## **v4.0.0-rc4**
* DEP: Update Sarif.Sdk submodule from [235394a to ec93dccd](https://github.com/microsoft/sarif-sdk/compare/235394a...ec93dccd). Full [SARIF SDK Release History](https://github.com/microsoft/sarif-sdk/blob/ec93dccd/src/ReleaseHistory.md).
Copy link
Collaborator

@suvamM suvamM Feb 16, 2023

Choose a reason for hiding this comment

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

@shaopeng-gh Note that Mike's PR on threading fixes got merged: microsoft/sarif-sdk#2618

So we should update the sarif-sdk submodule to reference the appropriate point in main. #Resolved

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated to latest main.

@@ -42,7 +42,7 @@ public void AnalyzingTarget(IAnalysisContext context)
{
}

public void Log(ReportingDescriptor rule, Result result)
public void Log(ReportingDescriptor rule, Result result, int? extensionIndex = null)
Copy link
Collaborator

@suvamM suvamM Feb 16, 2023

Choose a reason for hiding this comment

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

@shaopeng-gh Again, unclear what this change does. #Closed

@shaopeng-gh
Copy link
Collaborator Author

added.


In reply to: 1433423241

@shaopeng-gh shaopeng-gh requested a review from suvamM February 16, 2023 23:04
@HulonJenkins HulonJenkins merged commit 574d695 into main Feb 17, 2023
@HulonJenkins HulonJenkins deleted the release/v4.0.0-rc4 branch February 17, 2023 19:27
@shaopeng-gh shaopeng-gh restored the release/v4.0.0-rc4 branch February 17, 2023 19:31
@shaopeng-gh shaopeng-gh deleted the release/v4.0.0-rc4 branch February 17, 2023 19:35
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