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

Poison SBRP #17339

Merged
merged 8 commits into from
Oct 20, 2023
Merged

Poison SBRP #17339

merged 8 commits into from
Oct 20, 2023

Conversation

ellahathaway
Copy link
Member

@ellahathaway ellahathaway commented Sep 12, 2023 •
edited
Loading

Fixes dotnet/source-build#2817

These changes detect reference assembly leaks by checking for System.Reflection.AssemblyMetadataAttribute("source", "source-build-reference-packages") in built tarballs.

@ellahathaway ellahathaway force-pushed the poison-ref-assemblies branch 2 times, most recently from c77f382 to 22b41a2 Compare September 26, 2023 22:45
@ellahathaway ellahathaway marked this pull request as ready for review September 26, 2023 22:49
@ellahathaway ellahathaway requested a review from a team as a code owner September 26, 2023 22:49
@ellahathaway ellahathaway mentioned this pull request Sep 27, 2023
Closed
@ellahathaway ellahathaway force-pushed the poison-ref-assemblies branch from ff0ea37 to 7e4372e Compare October 16, 2023 20:18
MichaelSimons
MichaelSimons reviewed Oct 17, 2023
View reviewed changes
Copy link
Member

@MichaelSimons MichaelSimons left a comment

Choose a reason for hiding this comment

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

FYI the required SBRP changes flowing into installer in main is blocked as noted in #17310. It might be best to target release/8.0.

...Build/content/eng/tools/tasks/Microsoft.DotNet.SourceBuild.Tasks.LeakDetection/PoisonType.cs Outdated Show resolved Hide resolved
@MichaelSimons
Copy link
Member

One thing you may consider for testing purposes is to enable the poison check in the PR validation temporarily until you validate the changes. Then you can revert that change before you checkin.

@ellahathaway ellahathaway changed the base branch from main to release/8.0.1xx October 17, 2023 16:12
@ellahathaway ellahathaway requested a review from a team as a code owner October 17, 2023 16:12
@MichaelSimons
Copy link
Member

FYI the required SBRP changes flowing into installer in main is blocked as noted in #17310. It might be best to target release/8.0.

To clarify, I mean it might be best to have this PR target release/8.0 since the required SBRP changes have already flown into it and we want this in 8.0 anyways.

@ellahathaway ellahathaway force-pushed the poison-ref-assemblies branch 2 times, most recently from 3ce418a to 5e596d9 Compare October 17, 2023 17:11
@ellahathaway ellahathaway removed the request for review from a team October 17, 2023 17:21
@ellahathaway ellahathaway force-pushed the poison-ref-assemblies branch 2 times, most recently from 3ce418a to 55ab6d5 Compare October 19, 2023 20:09
@ellahathaway
Copy link
Member Author

installer-source-build and installer-source-build (VMR Source-Build CentOSStream8_Online_MsftSdk_x64) failed as expected due to 4 detected poison leaks.

MichaelSimons
MichaelSimons reviewed Oct 19, 2023
View reviewed changes
Copy link
Member

@MichaelSimons MichaelSimons left a comment

Choose a reason for hiding this comment

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

Can you please update the poison baseline with this PR for the known leaks noted in dotnet/source-build#3670. If this is not done, it will fail the CI builds.

@ellahathaway
Copy link
Member Author

Can you please update the poison baseline with this PR for the known leaks noted in dotnet/source-build#3670. If this is not done, it will fail the CI builds.

@MichaelSimons Yes! Updated in d1010d0

@ellahathaway ellahathaway changed the title Poison current SBRP Poison SBRP Oct 20, 2023
@ellahathaway ellahathaway merged commit 7f1c8c7 into release/8.0.1xx Oct 20, 2023
18 checks passed
@ellahathaway ellahathaway deleted the poison-ref-assemblies branch October 20, 2023 18:19
@ellahathaway ellahathaway mentioned this pull request Oct 23, 2023
Closed
@ellahathaway ellahathaway mentioned this pull request Jun 20, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ViktorHofer ViktorHofer ViktorHofer left review comments

@MichaelSimons MichaelSimons MichaelSimons approved these changes

@mthalman mthalman Awaiting requested review from mthalman

Assignees
No one assigned
Labels
Area-SourceBuild
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Poison] Add detection of reference assemblies
4 participants
@ellahathaway @MichaelSimons @ViktorHofer @mthalman