-
Notifications
You must be signed in to change notification settings - Fork 38
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
WiX: Improvements to Windows packaging #139
base: main
Are you sure you want to change the base?
Conversation
cc @egorzhdan , did you add anything besides the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch on the new module!
@@ -21,7 +21,7 @@ | |||
</Directory> | |||
</Directory> | |||
|
|||
<SetDirectory Id="INSTALLDIR" Value="[ProgramFiles64Folder]swift"> | |||
<SetDirectory Id="INSTALLDIR" Value="[ProgramFiles64Folder]Swift"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was intentionally swift
. I don't think that there is any strict suggestion/requirement/pattern that says that this should be Swift
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MSDN suggests the installation path to be [Program64FilesFolder]\ISV Name\Application Name\x64
(take x86_64 as example), and it’s clear to use title case with respect to blank spaces.
In practice, ISV Name
is sometimes ignored, arch directory is ignored if it doesn’t support multi-arch installation, version number is appended after Application Name
if it supports multi-version installation.
This means if possible we should use [Program64FilesFolder]\Swift\5.8-dev\runtime\x64
or [Program64FilesFolder]\Swift\5.8-dev\runtime
.
cr. https://docs.microsoft.com/en-us/windows/win32/msi/single-package-authoring
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The suggested layout is being followed - swift
is the ISV, runtime-...
is the versioned product directory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"ISV Name" uses title case, and I believe you’re confusing the "Application Name" here. An application on Windows can contain several components.
For Swift, "ISV Name" should be Swift
or Swift.org
, and "Application Name" should be Swift
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is often in title case, but I think that the lower case is acceptable as well. I intentionally left the .org
off to make it easier to work with.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Swift
looks more fit and I don’t think we have reason to refuse that🤷♂️ Other OSS projects like Git, CMake and Vim also respect title case on Windows — when in Rome, do as Romans do.
Also, according to Swift editorial guidelines:
When using the name Swift in titles, headlines, or body copy, always typeset Swift with an uppercase S followed by lowercase letters.
We should prefer Swift
to swift
where title case is allowed.
@@ -21,7 +21,7 @@ | |||
</Directory> | |||
</Directory> | |||
|
|||
<SetDirectory Id="INSTALLDIR" Value="[ProgramFiles64Folder]swift"> | |||
<SetDirectory Id="INSTALLDIR" Value="[ProgramFiles64Folder]Swift"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar
I don't recall adding something else, other than the C++ stdlib overlay ( |
@stevapple could you rebase this change please? |
Swift
, as respecting Windows convention.Cxx
library in SDK and runtime profiles.