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.

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

Nullable annotations for System.Security.Cryptography.Xml #67198

Merged
merged 67 commits into from
Sep 27, 2022
Merged

Nullable annotations for System.Security.Cryptography.Xml #67198

merged 67 commits into from
Sep 27, 2022

Conversation

SteveDunn
Copy link
Contributor

@SteveDunn SteveDunn commented Mar 27, 2022 •
edited

Fixes #67197

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Mar 27, 2022
@ghost
Copy link

ghost commented Mar 27, 2022

Tagging subscribers to this area: @dotnet/area-system-security, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

This is initially a draft pull request as there are a few questions that need answering as I go through

Author: SteveDunn
Assignees: -
Labels:

area-System.Security, community-contribution
Milestone: -

@SteveDunn SteveDunn changed the title Fixes #67197 nullable annotations for annotations for System.Security.Cryptography.Xml (DRAFT) Fixes #67197 nullable annotations for System.Security.Cryptography.Xml (DRAFT) Mar 27, 2022
@SteveDunn SteveDunn changed the title Fixes #67197 nullable annotations for System.Security.Cryptography.Xml (DRAFT) Nullable annotations for System.Security.Cryptography.Xml (DRAFT) Mar 27, 2022
@maxkoshevoi
Copy link
Contributor

If this is not ready for review yet, please make this PR an actual draft. There should be a button to do that:

image

@SteveDunn SteveDunn marked this pull request as draft March 27, 2022 11:58
@SteveDunn
Copy link
Contributor Author

If this is not ready for review yet, please make this PR an actual draft. There should be a button to do that:

image

Thanks @maxkoshevoi - I was wondering where that option was. Hope you're keeping well and safe!

@maxkoshevoi
Copy link
Contributor

Thanks @maxkoshevoi - I was wondering where that option was. Hope you're keeping well and safe!

Thanks, I'm in relatively safe city, still have my job, roof over the head, and something to do in free time (walking around the city, and annotating runtime 😄), so things are quite good over all =)
Hope it'll be safe to return to my home in a month or so.

@SteveDunn SteveDunn changed the title Nullable annotations for System.Security.Cryptography.Xml (DRAFT) Nullable annotations for System.Security.Cryptography.Xml Mar 29, 2022
@SteveDunn SteveDunn marked this pull request as ready for review March 30, 2022 06:01
maxkoshevoi
maxkoshevoi reviewed Mar 30, 2022
View reviewed changes
...ty.Cryptography.Xml/src/System/Security/Cryptography/Xml/AncestralNamespaceContextManager.cs Outdated Show resolved Hide resolved
...ty.Cryptography.Xml/src/System/Security/Cryptography/Xml/AncestralNamespaceContextManager.cs Outdated Show resolved Hide resolved
...ryptography.Xml/src/System/Security/Cryptography/Xml/C14NAncestralNamespaceContextManager.cs Outdated Show resolved Hide resolved
...ystem.Security.Cryptography.Xml/src/System/Security/Cryptography/Xml/CanonicalXmlNodeList.cs Show resolved Hide resolved
...ibraries/System.Security.Cryptography.Xml/src/System/Security/Cryptography/Xml/CipherData.cs Outdated Show resolved Hide resolved
...ibraries/System.Security.Cryptography.Xml/src/System/Security/Cryptography/Xml/DataObject.cs Outdated Show resolved Hide resolved
@SteveDunn SteveDunn mentioned this pull request Mar 31, 2022
Closed
36 tasks
@SteveDunn
Copy link
Contributor Author

I've just tidied up a few build errors after the recent rebase from main (after all the !!'s were removed).

I'm getting strange errors such as:

1>C:\git\runtime\src\libraries\Common\src\Interop\Windows\Advapi32\Interop.CryptAcquireContext.cs(25,36,25,55): error CA1420: Setting SetLastError to 'true' requires runtime marshalling to be enabled
1>C:\git\runtime\src\libraries\Common\src\Interop\Windows\Advapi32\Interop.CryptAcquireContext.cs(26,32,26,38): error CA1420: By-ref parameters require runtime marshalling to be enabled

Any ideas?

@SteveDunn SteveDunn mentioned this pull request Sep 3, 2022
Closed
@SteveDunn
Copy link
Contributor Author

LGTM after resolving comments and merge conflicts

I've rebased and looked at the issues you've mentioned.

krwq
krwq approved these changes Sep 8, 2022
View reviewed changes
Copy link
Member

@krwq krwq left a comment

Choose a reason for hiding this comment

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

looks good, there seem to be couple of unaddressed comments

@krwq
Copy link
Member

krwq commented Sep 8, 2022

Please do not rebase unless there are merge conflicts. Just push changes with those three fixes (or respond to the comments if not addressing for whatever reason)

@SteveDunn
Copy link
Contributor Author

Please do not rebase unless there are merge conflicts. Just push changes with those three fixes (or respond to the comments if not addressing for whatever reason)

I don't rebase unless there are merge conflicts. Unfortunately, last time, there were merge conflicts. I think I've done 4 or 5 rebases for this PR!

@SteveDunn
Copy link
Contributor Author

Hi - I think everything is now addressed here. Might we be able to merge it before a rebase is due again? :)

@SteveDunn
Copy link
Contributor Author

Hi again, can this be merged/closed before another rebase is required? Thanks in advance.

@stephentoub
Copy link
Member

@bartonjs, can you help land this?

krwq
krwq approved these changes Sep 27, 2022
View reviewed changes
Copy link
Member

@krwq krwq left a comment

Choose a reason for hiding this comment

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

Looks good from my side. Thank you!

@krwq krwq merged commit 0075639 into dotnet:main Sep 27, 2022
@krwq
Copy link
Member

krwq commented Sep 27, 2022

Contributes to: #41720

@stephentoub
Copy link
Member

Thank you, @SteveDunn.

@ghost ghost locked as resolved and limited conversation to collaborators Oct 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@stephentoub stephentoub stephentoub left review comments

@am11 am11 am11 left review comments

@maxkoshevoi maxkoshevoi maxkoshevoi left review comments

@bartonjs bartonjs bartonjs left review comments

@krwq krwq krwq approved these changes

Assignees

@krwq krwq

Labels
area-System.Security community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Group 5] Enable nullable annotations for System.Security.Cryptography.Xml
9 participants
@SteveDunn @maxkoshevoi @bartonjs @stephentoub @krwq @ViktorHofer @danmoseley @vcsjones @am11