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

Add text-only package Polyfill 7.9.1 #1105

Merged
merged 1 commit into from
Dec 30, 2024

Conversation

Youssef1313
Copy link
Member

@Youssef1313 Youssef1313 commented Dec 29, 2024

Needed for microsoft/testfx#4453

Generated using .\generate.cmd -type text -package polyfill,7.9.1

@NikolaMilosavljevic NikolaMilosavljevic merged commit 80f1e84 into dotnet:main Dec 30, 2024
4 checks passed
@Youssef1313 Youssef1313 deleted the polyfill791 branch December 30, 2024 17:26
@ViktorHofer
Copy link
Member

@Youssef1313 the Polyfill package exists to polyfill newer APIs on an older runtime. When building source-only, we always target the latest TFM. I don't think we should introduce this package to SBRP and depend on it in testfx when building a recent enough TFM. cc @Evangelink @dotnet/source-build

@Youssef1313
Copy link
Member Author

We still use it on latest TFM to avoid #ifs. It will be more noisy to have to use #if when we can simply have the library doing it already. What is the problem with using the package on latest TFM?

@ViktorHofer
Copy link
Member

ViktorHofer commented Jan 2, 2025

What is the problem with using the package on latest TFM?

When you ask our security champs they will always recommend to not depend on external libraries in our own product (with product I mean the .NET SDK in this context). There are always associated risks with using external bits. I.e. how would you respond to a security incident inside the external library's code path as part of servicing and under time constraint?

It's an external library that doesn't participate in source-build. Therefore it needs to be kept up-to-date here in SBRP which as you noticed is a very manual process. When you look at the entirety of libraries in SBRP you will notice that we only have a handful of external ones. There is a very small gain in using this library when building at least from source. I could probably make the same statement even when not building from source but inserting into the .NET SDK. I assume that the .NET 9 TFM is used in that case for the test platform?

I tried removing the library when building for NetCurrent and the only thing that is now failing are the Guard.NotNull calls which shouldn't be necessary when targeting a runtime that has nullable annotations enabled (I think that was either .NET Core 3.1 or .NET 5). ViktorHofer/testfx@c262b28

cc @GrabYourPitchforks @jkotas for opinions

@jkotas
Copy link
Member

jkotas commented Jan 2, 2025

There are always associated risks with using external bits

These risks translate to costs. The team that introduced the dependency on the external bits has to be willing to pay these costs and they need to do an analysis to make sure that the savings from introducing the dependency are more than the costs of maintaining the dependency in a compliant way that is often surprisingly expensive.

@nohwnd @Evangelink Can you share you thinking about how much you are saving by taking the dependency on this nuget package?

@Evangelink
Copy link
Member

I would rather drop source build given this is putting a big constraint only for the sake of having a few classes reused inside of SDK. Ideally we would use something like Paket to clone the files but sadly NuGet is far from having this feature set. I would vote for dropping source build altogether and just manually copy-paste the few classes we need.

Adding the full ifdef complexity all over our codebase just for the sake of a few files integration seems like a big loss for me. If there is any MS based solution to use, fine for me. I am also fine if we have some people from runtime helping us convince management to let us drop unsupported or old TFMs from testing tools so we don't have to pay unecessary maintenance cost :)

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.

5 participants