search

brutaldev / StrongNameSigner

Strong-name sign third party .NET assemblies without the source code.
https://brutaldev.com/post/net-assembly-strong-name-signer
Other
320 stars 68 forks source link

Refactor the code so that the UI and the Console version share more logic. #22

Closed bradphelan closed 4 years ago

bradphelan commented 8 years ago

I created AssemblyCollection and AssemblyHolder to unify all the shared logic in the UI and the console versions. Logging is now an inject-able dependency that is different in the UI and console versions.

The code should also be much faster as it doesn't write and read the assemblies as often, especially in the loop where the references are updated. I save and sign all the assemblies at the end.

brutaldev commented 8 years ago

@bradphelan

I'm going to take certain parts of this pull request because I'm not convinced about all the changes. Secondly, it won't merge without some serious effort so hand-picking the good parts will be easier.

Just at a glance, I do like the idea of signing all at once right at the end, and I appreciate adding the PDB functionality, the rest may be a bit much. The public API is now broken by introducing new classes instead of the previous strings, existing callers and scripts will break. New classes are also not fully documented and they are public. Adding console prints to the helper class isn't a good idea, this is an API assembly so having console specific code in their doesn't feel right. You've also made a number of methods public which really shouldn't be, such as the key generator which is cached to ensure the same key is used after multiple calls for reference fixing, this may not be intuitive to an external caller. These now public methods also remain undocumented. I'll need to spend some time verifying the logic separated from each client (Console and WinForms), neither are unit tested so there is a big risk here and to be honest it worked well so already. I think the refactor was unnecessary and too risky just to pull in, smaller, focused changes would have been much easier to manage.

bradphelan commented 8 years ago

Sorry about the bulk dump of the pull request. It was all done in a bit of a panic. In the end today we have decided to steer clear of strong naming for what we are doing. We were trying to get it integrated into our build process but can't find a good spot in the MSBUILD process to inject the renaming. We might come back to it when we have less pressing things to take care of.

So instead of just burying the changes we made I thought I give them to you. Pick and choose what you wish. I hope it is useful.

With regards to the console logging for the UI version, take a closer look. You can inject any type of logging you wish. It doesn't have to be to console, it could be to a in memory DB or remote logger. The signiture is just Action<string, LogLevel, Color?> We did this because we noticed the UI and Console version were sharing a great deal of code and the only difference really was the logging, so we pulled that out as dependency injection.

The command line should work as before. Not sure what public API is broken. Did you intend it to be used as a library? The public classes can all be made internal. They are just internal abstractions to make it easier to track what is going on during the signing and copying stages. Previously it was just lists of strings and it wasn't always clear what those strings were for. I think the behaviour is as before.

It's true that there are lacking unit tests but given that I changed so much the tests still passed with little change required. This suggests that the tests were no so comprehensive in the first place. My suggestion is to take a bunch of dll's in a directory. Sign them using the old version. MD5 the resulting directory and then do that again with the console version of the new version. That will test the entire stack pretty easily.

But as I say we sort of rushed into this, hacked around and then decided that strong naming was too much of a brain strain at the moment. The problem is we have a bunch of libraries, some that are shared and some that end up as plugins and some that get accessed by an XUnit extension that we wrote. With so many separate assemblies with shared dependencies, strong naming always ended up with some wierd DLL loading error. We will come back to it some time. I hope you find some or all bits usefull. Regards and thanks for your effort on the project :)

Brad

On Wed, Apr 27, 2016 at 2:50 PM Werner van Deventer < notifications@github.com> wrote:

@bradphelan https://github.com/bradphelan

I'm going to take certain parts of this pull request because I'm not convinced about all the changes. Secondly, it won't merge without some serious effort so hand-picking the good parts will be easier.

Just at a glance, I do like the idea of signing all at once right at the end, and I appreciate adding the PDB functionality, the rest may be a bit much. The public API is now broken by introducing new classes instead of the previous strings, existing callers and scripts will break. New classes are also not fully documented and they are public. Adding console prints to the helper class isn't a good idea, this is an API assembly so having console specific code in their doesn't feel right. You've also made a number of methods public which really shouldn't be, such as the key generator which is cached to ensure the same key is used after multiple calls for reference fixing, this may not be intuitive to an external caller. These now public methods also remain undocumented. I'll need to spend some time verifying the logic separated from each client (Console and WinForms), neither are unit tested so there is a big risk here and to be honest it worked well so already. I think the refactor was unnecessary and too risky just to pull in, smaller, focused changes would have been much easier to manage.

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/brutaldev/StrongNameSigner/pull/22#issuecomment-215071582

brutaldev commented 8 years ago

@bradphelan

I do appreciate the changes, specifically the PDB work which has been an open feature request for quite some time which I will certainly add in for another release.

I'll review things carefully when I get a chance, at a glance the changes were overwhelming so I may have misinterpreted things.

The signing assembly is most certainly used as a library and direct calls are made by users via PowerShell. This public method change for example is a breaking change.

The unit tests are quite comprehensive and test every aspect of signing that the project offers, however, there are no tests for the UI logic. The reason your test would still be passing is because the signing, reference and version correction logic would have remained intact. MD5 checks would help too, but internal checks are performed against the test assemblies as well as loading and running them to ensure they are not corrupted.

Not sure when I'll get around to adding/merging some of these changes but I will do eventually. Thanks for the work, rather you backup your changes here so that they can be used than losing them in the void. Good luck with your project, it sounds complicated...

brutaldev commented 4 years ago

Too much has changed in the request to take it now. Will use these ideas when cleaning up for the next major version.