feat(csharp): A Windows Installer for the Deephaven Excel Add-In #6378
Conversation
kosak
added
NoDocumentationNeeded
NoReleaseNotesNeeded
| Advanced Installer package. This custom action does the actions | ||
| needed to manipulate the Windows Registry in order to do things like | ||
|
|
||
| 1. detect whether the version of Office installed is 32 or 64 bit |
There was a problem hiding this comment.
Capital D detect to be in line with point 2?
| This library is a .NET 4.8.0 Class Library with some special boilerplate | ||
| code provided by Advanced Installer. | ||
|
|
||
| .NET 4.8.0 is pretty old at this point, but I chose it because (I believe) |
There was a problem hiding this comment.
I suggest to replace "I chose" with "it was chosen", and removing "(I believe)". It you think is important to keep it as is, I would then replace "I chose" with "I (Corey) chose". My reasoning for the suggestions: giving a rationale for why something was chosen makes sense. Any statement is assumed to come from some set of "I believe" so that's redundant unless you want to be explicitly apologetic, and I don't think this is the time or place to be (because IMHO that part doesn't add value in the context of somebody reading this text with a need for context towards some concrete goal).
|
|
||
| https://www.advancedinstaller.com/user-guide/create-dot-net-ca.html | ||
|
|
||
| Basically the steps are: |
There was a problem hiding this comment.
Suggests to remove "basically"
| * From the list of templates, select the C# Custom Action template or the | ||
| C# Custom Action (.NET Framework) template, depending on your needs | ||
|
|
||
| Because of the above compatibility requirements I have decided that |
There was a problem hiding this comment.
I don't understand this paragraph.
| HKEY_LOCAL_MACHINE\Software\Microsoft\Office\${VERSION}\Outlook | ||
| ``` | ||
|
|
||
| And yes, this information is stored at the "Outlook" part of the path, |
There was a problem hiding this comment.
Suggest replace "And yes," with "Notably, "
| not Excel. This key contains an entry with the name | ||
| which contains the name "Bitness" and the values "x86" or "x64". | ||
|
|
||
| When I say ${VERSION} I mean one of the known versions of Office, one of |
There was a problem hiding this comment.
Suggest replace "When I say ${VERSION} I mean..." with "${VERSION} means ..."
| for Deephaven purposes we can hardcode this to 16.0 and ignore previous | ||
| versions we might find. | ||
|
|
||
| Note that for reasons when we look up this key programmatically, we need |
There was a problem hiding this comment.
The wording here sounds funny; I am not an English native speaker, but perhaps
"Note that due to the need for looking up this key programmatically, we need..."
| var bitnessValue = subKey.GetValue(RegistryKeys.Bitness.Name); | ||
| ``` | ||
|
|
||
| Apparently, the Bitness key for a 32 bit installation of Office will still |
There was a problem hiding this comment.
I am not sure how to word this, but I have a sense that it may be best to be more explicit here; it sounds to me like you are trying to be more explicit about the fact that we have not tested this on a combination of {64 bit machine + 32 bit installation of excel}, and this is our guess of what would happen in that case; I think saying that explicitly might be better than the way you are wording it now.
| /R "C:\Users\kosak\Desktop\exceladdin-v7\ExcelAddIn-AddIn64-packed.xll" | ||
| ``` | ||
|
|
||
| The fact that I have installed my addin on the Desktop is not a best practice. The point here is to show the syntax. |
There was a problem hiding this comment.
I think my general comment about this file is to try to avoid wording it as "I ..." and instead do a more normative "Do X.." or "an example is...". My suggestion is, in places where we are not certain, just be explicit about the uncertainty. The document can evolve to reflect new understanding over time; whatever our belief is now makes sense to be said explicitly, and whenever the threshold of certainty is such that we feel we need to warn the reader, just do that explicitly. This is my sense of how this file would be most useful to other people reading it.
| /// <param name="is64Bit"></param> | ||
| /// <param name="failureReason"></param> | ||
| /// <returns></returns> | ||
|
|
There was a problem hiding this comment.
Is this space between the comment block and the function signature intended?
| <startup> | ||
| <supportedRuntime version="v4.0" sku=".NETFramework,Version=v4.8" /> | ||
| </startup> | ||
| </configuration> |
There was a problem hiding this comment.
No newline before EOF? (disregard if generated file)
| </ProjectReference> | ||
| </ItemGroup> | ||
| <Import Project="$(MSBuildToolsPath)\Microsoft.CSharp.targets" /> | ||
| </Project> |
There was a problem hiding this comment.
No newline before EOF? (disregard if generated file)
This PR contains a few related items:
csharp/ExcelAddInInstaller/ExcelAddInInstaller.aip). This is not expected to be human-readableYou may also want to review m Confluence writeup:
https://deephaven.atlassian.net/wiki/spaces/ED/pages/152862817/Creating+a+Windows+MSI+Installer+for+the+Deephaven+Excel+Add-In+2nd+gen