Better manifest file detection #758
Conversation
…e 0/1 files with a .manifest extension
…correctly parsed so that the existing tests pass
| // we only try and parse absolute uris, because a relative uri can just be treated like a file path for our purposes | ||
| if (Uri.TryCreate(applicationManifestFileNameAttribute.Value, UriKind.Absolute, out var uri)) | ||
| { | ||
| Path.GetFileName(uri.LocalPath); // works for http(s) and file:// uris |
There was a problem hiding this comment.
There is a bug here. I think you meant:
| Path.GetFileName(uri.LocalPath); // works for http(s) and file:// uris | |
| return Path.GetFileName(uri.LocalPath); // works for http(s) and file:// uris |
Seems worth adding a unit test for this case. I think you can convert your new, existing unit test into a theory and parameterize codebase with both scenarios.
|
|
||
| FileInfo? manifestFile = filteredFiles.SingleOrDefault(f => ".manifest".Equals(f.Extension, StringComparison.OrdinalIgnoreCase)); | ||
| var appManifestFromDeploymentManifest = GetApplicationManifestForDeploymentManifest(file); | ||
| FileInfo? manifestFile = filteredFiles.SingleOrDefault(f => f.Name.Equals(appManifestFromDeploymentManifest, StringComparison.OrdinalIgnoreCase)); |
There was a problem hiding this comment.
This is avoidable if appManifestFromDeploymentManifest is null.
Also, can we rename manifestFile to applicationManifestFile since now we're dealing with potentially many different kinds of manifest files. We had deployment and application manifests, but now we're dealing with other manifests (like assembly manifests).
| } | ||
|
|
||
| /// <summary> | ||
| /// Try and find the application manifest (.manifest) file from a clickonce application manifest (.application / .vsto |
There was a problem hiding this comment.
Shouldn't it be:
| /// Try and find the application manifest (.manifest) file from a clickonce application manifest (.application / .vsto | |
| /// Try and find the application manifest (.manifest) file from a clickonce deployment manifest (.application / .vsto |
| var xml = new XmlDocument(); | ||
| xml.Load(deploymentManifest.FullName); | ||
| // there should only be a single result here, if the file is a valid clickonce manifest. | ||
| var dependentAssemblies = xml.GetElementsByTagName("dependentAssembly"); |
There was a problem hiding this comment.
We prefer using the explicit type if the implied type is not super obvious. It will facilitate understanding both in a browser and IDE.
|
@jackmtpt, thank you for your contribution! We appreciate it. Can you open an issue with repro steps and link it? It would help us better understand the scenario you're trying to improve. I left some feedback. I also created a branch with my suggested changes. You can cherry pick the latest commit if you want. |
|
There's been no activity here for weeks, so I'm closing this in favor of #773. |
3 participants
This PR changes the ClickOnce signer to better detect which .dll.manifest or .exe.manifest Application manifest file to sign. It reads the supplied .vsto/.application Deployment manifest file and parses out the name of the Application manifest that's referenced, and then passes that to
magefor updating.