CZEMacLeod / MSBuild.SDK.SystemWeb

This MSBuild SDK is designed to allow for the easy creation and use of SDK (shortform) projects targeting ASP.NET 4.x using System.Web.
MIT License
151 stars 8 forks source link

Add support for autogenerated binding redirects that are not source controlled #73

Open julealgon opened 4 months ago

julealgon commented 4 months ago

I've been using this SDK for a while now in a bunch of our legacy projects to great success, however there is one pain point which I wanted to document here in the form of a proposal.

We are leveraging GeneratedBindingRedirectsAction as Overwrite in our projects. Whenever packages are updated in our solution, we need to make sure to rebuild it fully so that binding redirects in the legacy projects are adjusted as needed, before we can commit the changes. On our solution, this is a tedious process as it takes several minutes to build the entire project. It is also error prone, since if we don't perform this step, it ends up generating pending changes for other developers later, which is not something we want of course.

For modern projects, this issue has been resolved as binding redirects are "magically" taken care of: they never show up and are always up-to-date without the developer needing to do anything extra. I'd like an experience similar to that with this SDK.

Would it be possible to generate the binding redirects in a way that keeps them "decoupled" from the main web.config in such a way that the file can be generated and linked at build time, but doesn't need to be merged into source control? This would allow us to update the packages and don't have to worry about manually refreshing the binding redirects with a local build right after. No matter where the project was built, it would generate the redirects (probably in a separate file) and link it with the original web.config, using, say, a configSource path.

CZEMacLeod commented 3 months ago

@julealgon This has been tried by various people, but there are a large number of downsides to this that have been encountered. The main points (from memory)

If you can get a reliable system in place for this, I'm open to a PR to add the additional mode, but I feel there would have to be a lot of documentation about the drawbacks and limitations.

If you are working in a multi-developer scenario, I would hope that you are running unit tests before checking in the code/merging changes. You could set up a check to prevent accepting PRs if the build results in a binding issue. Alternatively (and it is what I do) - if a build on the build server after a check in of say a package update, results in changes to web.config, add that change as a new commit. (This is what I do.)

NickCraver commented 3 months ago

I don't have a great solution to this at the moment either, but the #1 problem here is over redirecting because of the commits, as well as cherry-picking back to release branches. Both of these get pretty painful.

For example if you have a package that depends on X and causes a redirect for X because it ultimately bumped X to say 5.0.0, we get a redirect. That redirect is then committed to source control. Package gets updated, drops the need for X, the redirect to 5.0.0, which isn't the version we're at anymore (say it's back to 2.0.0). If no redirect is needed at all (everything else needed 2.0.0), everything is happy but the redirect is wrong and we get a runtime boom. Compared to any app.config where everything is generated to the output directory, but not checked in...this all just works, with the right redirects.

But I take some the above points, doing this only in publish (basically generate, without erroring in the build) may not be for everyone. Just adding some thoughts in case it adds ideas here, I'd love to make this work as it's one of the main pain points we have left in builds/runtime at all.

leusbj commented 3 months ago

@NickCraver, @julealgon

A stopgap idea might be to create a build target that runs only during a clean and uses a XmlPeek/XmlPoke/TransformXML task to entirely remove the <AssemblyBinding> section of the web.config. You could then ask all members of the team to only "check-in" after having "cleaned" the solution. This would keep your repo clean of redirects and give a stable starting point for each team members.

There might be one other possibility that I haven't seen yet mentioned as a solution to this situation, and that is to "treat" the project root web.config file as an "intermediate" or "generated" file.

The Pro's:

The Con's:

Some combination of ideas (t4 generation or TransfromXML build steps) will allow you to generate the web.config dynamically at build time (building it up from other files can be included in source control) https://devblogs.microsoft.com/dotnet/asp-net-web-projects-web-debug-config-web-release-config https://dougrathbone.com/blog/2011/09/14/using-custom-webconfig-transformations-in-msbuild https://johan.driessen.se/posts/Applying-MSBuild-Config-Transformations-to-any-config-file-without-using-any-Visual-Studio-extensions/ https://www.damirscorner.com/blog/posts/20120108-TransformationOfConfigurationFilesInBuildProcess.html https://stackoverflow.com/questions/3922291/use-visual-studio-web-config-transform-for-debugging#3994081 https://geoffhudik.com/tech/2010/10/19/webconfig-automation-with-t4-and-a-macro-html/

CZEMacLeod commented 3 weeks ago

Is anyone looking any further at this mechanism? I'm not adversed to having it as an option, but I don't know if there is a huge gain from the amount of work required to implement it. I remember coming across a situation where it is possible to require redirects for packages not directly referenced by the project, such as compilers, webpages, and other libraries referenced in web.config for dynamic load that may break if all redirects are dropped. I do quite like the idea of having an option for clean or rebuild to delete the redirects and recreate them during the build. I don't think it should be enabled by default, and it may require something where specific redirects are kept to avoid the situation I mentioned above (which can cause the compiler to fail to build the project at all). If there is still interest in this - would it be okay to move it to a discussion until a PR is possible, and if no-one has any appetite for it, perhaps it can be closed for now?

julealgon commented 3 weeks ago

Is anyone looking any further at this mechanism?

Not on my side. I was hoping someone else with more experience on MSBuild and the standard targets would take a stab at it.

If there is still interest in this - would it be okay to move it to a discussion until a PR is possible, and if no-one has any appetite for it, perhaps it can be closed for now?

I'd absolutely love for this to remain open to at least signal it is open for grabs by the community. I think it is unlikely I'll be able to dedicate any time to it myself due to priorities in the company, but this does impact us directly quite a bit, and there have been many situations already since we migrated that developers ended up merging code without rebuilding the affected projects locally after a package update that ended up generating a diff on all other devs' machines later and required a patch PR. Having this autogenerated on build would completely eliminate that concern.

I can only see this same situation happening again and again with some of our projects as they have tons of dependencies that have to be upgraded fairly often which affect binding redirects.

Having to build a project after a package upgrade is not a normal requirement so it naturally becomes hard to enforce. In the meantime, we might consider adding something to our build pipeline that detects the generation of any pending changes in the build agent workspace and fail the build when that happens though.

CZEMacLeod commented 3 weeks ago

@julealgon Having to (build and) run the project and tests after upgrading packages to ensure that nothing breaks should be a normal requirement, so I am not entirely in agreement with that.

I would recommend using gated check-ins for PRs which break if there are any binding redirects detected. I go (slightly) the opposite direction and automatically add a commit with the binding updates if they are detected in the CI build. This does cause it to build again in CI, but the second time it passes.

It does feel like this might be slightly outside the scope of the SDK though and might be something that could be in another package brought in to do just this, if indeed it should be an MSBuild thing, rather than a CI/CD thing, as this feels more like process and team specific than the SDK itself.

As I said before though - if someone can create something lightweight and robust, and put it in a PR behind a flag of some sort, I'll happily add it in.

julealgon commented 3 weeks ago

@julealgon Having to (build and) run the project and tests after upgrading packages to ensure that nothing breaks should be a normal requirement, so I am not entirely in agreement with that.

Oh, to be clear, I'm not challenging that notion at all... I was just arguing that enforcing that behavior is harder when the build can generate further changes that are source controlled. Developers don't expect that, and they might've already pushed their changes before testing and end up merging the pr and disregarding their extra local changes.

I've also hit situations in the recent past where the next build right after a package update would, for whatever reason, not update the binding redirects. Then I'd go ahead, test and merge, and on another build, when working on the next task, they would be updated "out of nowhere". I'm not sure if there is an associated bug there but I have no idea how to repro this. I suspect it might've happened with some of my colleagues as well and that's where some of the patch PRs were necessary.

I would recommend using gated check-ins for PRs which break if there are any binding redirects detected.

Yeah, this is likely what I'll pursue as we migrate from our very legacy and inflexible build process to Github Actions.

I go (slightly) the opposite direction and automatically add a commit with the binding updates if they are detected in the CI build. This does cause it to build again in CI, but the second time it passes.

Interesting. I'm not very fond of the CI process generating commits though, but thanks for sharing the idea.

It does feel like this might be slightly outside the scope of the SDK though and might be something that could be in another package brought in to do just this, if indeed it should be an MSBuild thing, rather than a CI/CD thing, as this feels more like process and team specific than the SDK itself.

That's fair, but I disagree that this is not a concern of the SDK. To me it is a direct responsibility of it since it is the one that generates the binding redirects in the first place.

This is highly debatable though, so I won't push on this point further.

As I said before though - if someone can create something lightweight and robust, and put it in a PR behind a flag of some sort, I'll happily add it in.

👍