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

Fix of temporary file handling and missing dll's in tests. #48

Merged
merged 3 commits into from
Aug 29, 2018

Conversation

alphaleonis
Copy link
Contributor

@alphaleonis alphaleonis commented Aug 29, 2018

In reference to #47

  • Updated a couple of test assemblies with PDB-files and added a couple of tests to verify some previously failing scenarios.

  • Added strong references to the Mono.Cecil.Pdb, Mdb and Rocks libraries so that they are among other things properly copied to the test project. (See AssemblyInfo.cs). If the Mono.Cecil.Pdb.dll is not copied locally (which it wasn't), then the symbol reading doesn't work.

  • Fixed temporary file handling in SigningHelper, so that it now uses a temporary directory to store the generated files during generation and ensures they are deleted afterwards. I refactored out the file handling into its own private class OutputFileManager which takes care of creating the temporary directory etc. Cleans up the methods using it considerably and avoids repetitive code.

  • Also added an .editorconfig so that the indentation remained consistent.

Peter Palotas added 3 commits August 29, 2018 10:58
Added .vs directory to .gitignore.
- Added strong references to the Mono.Cecil.Pdb, Mdb and Rocks libraries so that they are among other things properly copied to the test project.
- Fixed temporary file handling in SigningHelper, so that it now uses a temporary directory to store the generated files during generation and ensures they are deleted afterwards.
@brutaldev brutaldev merged commit e7eeac5 into brutaldev:master Aug 29, 2018
@brutaldev
Copy link
Owner

These changes were released in v2.3.0.

@brutaldev brutaldev self-requested a review August 29, 2018 13:17
@alphaleonis alphaleonis deleted the bug/47 branch May 8, 2019 10:58
@robertmclaws
Copy link

robertmclaws commented Sep 17, 2019

This temp file algorithm is producing unwanted side effects in MSTest 2.0 projects in SDK apps. It keeps trying to write temp files to locations like C:\Program Files (x86)\Reference Assemblies\Microsoft\Framework\.NETFramework\v4.7.2\Facades\StrongNamerTemp.20252.dvstbhbm.34h, which is not correct. Isn't there a Path.GetTempPath method that would be better?

@brutaldev
Copy link
Owner

@robertmclaws Sure, the code that assigns the TestAssemblyDirectory could be better, the actual test assemblies would need to be copied there as well assuming this is what you are referring to?

@robertmclaws
Copy link

So here's the situation. I was getting ready to release a signed version of the RESTier library for Microsoft. The main libraries signed fine on their own, but the unit tests failed, and I couldn't figure out why. I figured I would add this package to the tests, so the build process would sign whatever packages it needed.... since the test assemblies aren't shipped anyway.

But this line of code broke that concept, because the build target tried (79,000 times! which seems to be another bug) to write the temp file to the .NET Framework Facades folder instead of the system temp file location.

So, if the Targets are doing the signing in the right place, it should just sign the files in the temp folder and copy them back to the $(OutDir).

The logic flaw in the code you pointed out is that the unit tests for end user projects aren't going to be signing assemblies during unit tests, so they shouldn't be trying to use the GetCurrentProcess() folder at all, because that would be the build system executable folder in ProgramFiles.

@brutaldev
Copy link
Owner

So really the line in question just needs to look like this to avoid writing to a potentially protected location?
tempDir = Path.Combine(Path.GetTempPath(), $"StrongNamerTemp.{Process.GetCurrentProcess().Id}.{Path.GetRandomFileName()}");

@robertmclaws
Copy link

I believe so, yes :)

@brutaldev
Copy link
Owner

Updated in version 2.4.0 along with updating Mono.Cecil to the latest.

@robertmclaws
Copy link

Sweet, thanks! I'll see about giving it another try.

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.

3 participants