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

Adopt consistent approach for defining InternalsVisibleTo in the repo #45748

Merged
merged 9 commits into from
Dec 28, 2022

Conversation

silamon
Copy link
Contributor

@silamon silamon commented Dec 24, 2022

Fixes #41244. This pull request moves all InternalsVisibleTo from AssemblyInfo.cs to their respective project file (.csproj). When reviewing, compare them 2-by-2 since the csproj file is always next to the AssemblyInfo.cs file.

One exception: the RateLimiting project had already the InternalsVisibleTo declared. In this project the AssemblyInfo.cs is removed without applying changes to .csproj project file.

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

ghost commented Dec 24, 2022

Thanks for your PR, @silamon. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

@dnfadmin
Copy link

dnfadmin commented Dec 24, 2022

CLA assistant check
All CLA requirements met.

@Tratcher
Copy link
Member

Wow, that's a bigger change than I expected. Good job.

Something's failing in DataProtection?
https://github.com/dotnet/aspnetcore/pull/45748/checks?check_run_id=10286080432

@Tratcher Tratcher self-assigned this Dec 27, 2022
@Tratcher Tratcher added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Dec 27, 2022
@ghost
Copy link

ghost commented Dec 27, 2022

Hey @dotnet/aspnet-build, looks like this PR is something you want to take a look at.

@silamon
Copy link
Contributor Author

silamon commented Dec 27, 2022

Something's failing in DataProtection? https://github.com/dotnet/aspnetcore/pull/45748/checks?check_run_id=10286080432

Yes, thanks for looking. Actually, it's not only in DataProtection. There are also a few cases in Kestrel.
Strange thing is that I've included the correct InternalsVisibleTo in the project. At least I think so, but I may miss some knowledge. :)

For example:
2022-12-24T11:26:45.6372501Z D:\a_work\1\s\src\Servers\Kestrel\Kestrel\src\WebHostBuilderKestrelExtensions.cs(37,76): error CS0122: 'KestrelServerOptionsSetup' is inaccessible due to its protection level [D:\a_work\1\s\src\Servers\Kestrel\Kestrel\src\Microsoft.AspNetCore.Server.Kestrel.csproj]

There's a InternalsVisibleTo declared in Microsoft.AspNetCore.Server.Kestrel.Core for Microsoft.AspNetCore.Server.Kestrel. So, I'm not really sure why it remains inaccessible. The namespace of KestrelServerOptionsSetup does have "Internal" and most of those DataProtection classes also have that, but this is more of an assembly thing if I understand correctly.

May it have something to do with the fact that I omitted the PublicKey?

@silamon
Copy link
Contributor Author

silamon commented Dec 27, 2022

Nevermind, I think I saw the problem.

@Tratcher Tratcher enabled auto-merge (squash) December 27, 2022 21:38
@Tratcher Tratcher added this to the 8.0-preview1 milestone Dec 27, 2022
auto-merge was automatically disabled December 28, 2022 05:44

Head branch was pushed to by a user without write access

@JamesNK JamesNK enabled auto-merge (squash) December 28, 2022 05:46
@JamesNK
Copy link
Member

JamesNK commented Dec 28, 2022

The tests that use Moq and Castle.DynamicProxy are failing.

auto-merge was automatically disabled December 28, 2022 18:01

Head branch was pushed to by a user without write access

@Tratcher Tratcher merged commit 400a989 into dotnet:main Dec 28, 2022
@Tratcher
Copy link
Member

Thanks

@silamon silamon deleted the internalvisibleto branch December 29, 2022 06:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adopt consistent approach for defining InternalsVisibleTo in the repo
4 participants