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 support for autogenerated binding redirects that are not source controlled #73

Open
julealgon opened this issue Mar 5, 2024 · 7 comments
Labels
enhancement New feature or request up for grabs An issue that is up for grabs by the community, but is unlikely to be worked on otherwise.

Comments

@julealgon
Copy link

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
Copy link
Owner

@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)

  • The project may not build at all without the redirects file
  • Intellisense etc. won't work on the first compile, so a developer would have to build multiple times after checking out the code.
  • Similarly, it could require multiple builds before it could run.
  • A build server would fail the first build and require special logic.
  • configSource is a bit flaky for bindings as the folder path is not always relative to the project root.
  • Publishing a project is more difficult as the non-controlled file is difficult to include.
  • You don't have an easy way to make the project deterministic for rebuilding older versions.
  • The mechanism uses the net sdk built in mechansim for generating redirects which does not support this and thus it would require custom code, and/or a lot of additional messing about to get it to work.
  • This is more complicated when you have multiple web.config files, such as when you are using MVC and/or RazorGenerator.

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
Copy link

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
Copy link
Contributor

leusbj commented Mar 19, 2024

@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 web.config file itself can be excluded from source repo (line item in .gitignore; .tfignore)
  • All developer chosen items/values are defined in some other baseconfig file which IS included in the source control
  • The web.config can be made to be generated "early" in the build process so it is present before GenerateBindingRedirects process
  • The web.config can be updated directly by the GenerateBindingRedirects process since the file is entirely generated by the build
  • The web.config file in project root will be present by "run/debug" time, because build runs before debug start, so local debugging will work
  • The Web.config file in project root will be present by "package" time, because build runs before package step, so MSDeploy and folder packaging will include it.

The Con's:

  • Pattern of having web.config be ignored by source control will seem odd/unexpected to many developers
  • Pattern of having Developer chosen items/values stored in a differnt place will need to be communicated to all members to the dev team
  • No "easy button" (meaning no automated tooling) mechanism to convert projects to this new pattern, so a manual process will need to be used for each project
  • Older nuget packages authored expecting Packages.config style references frequently make use of install.ps1, web.config.install.xdt, and hardcoded assumptions about "web.config" being present, so each individual referenced package would need to be evaluated regarding any elements that need to be placed into the baseconfig file

I haven't yet built out this idea robust enough to offer to include it with this project type, so I don't have a ready-made solution for you. But perhaps in your situation you can mitigate the negatives in order to gain the benefits.

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 CZEMacLeod added enhancement New feature or request help wanted Extra attention is needed labels Jun 10, 2024
@CZEMacLeod
Copy link
Owner

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
Copy link
Author

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 CZEMacLeod added the up for grabs An issue that is up for grabs by the community, but is unlikely to be worked on otherwise. label Jun 10, 2024
@CZEMacLeod
Copy link
Owner

@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.

@CZEMacLeod CZEMacLeod removed the help wanted Extra attention is needed label Jun 10, 2024
@julealgon
Copy link
Author

julealgon commented Jun 10, 2024

@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.

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request up for grabs An issue that is up for grabs by the community, but is unlikely to be worked on otherwise.
Projects
None yet
Development

No branches or pull requests

4 participants