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

WiX: Improvements to Windows packaging #139

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

stevapple
Copy link
Contributor

  • Update Program Files entry name to Swift, as respecting Windows convention.
  • Add missing Cxx library in SDK and runtime profiles.

@stevapple
Copy link
Contributor Author

cc @egorzhdan , did you add anything besides the Cxx module for C++ interop?

Copy link
Collaborator

@compnerd compnerd left a 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">
Copy link
Collaborator

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.

Copy link
Contributor Author

@stevapple stevapple Aug 21, 2022

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On my machine this rule gets fully addressed except for dotnet and nodejs. I don’t think we should be the rule breaker if we can avoid that.

image

Copy link
Collaborator

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.

Copy link
Contributor Author

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.

Copy link
Collaborator

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.

Copy link
Contributor Author

@stevapple stevapple Aug 23, 2022

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">
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar

@egorzhdan
Copy link
Contributor

cc @egorzhdan , did you add anything besides the Cxx module for C++ interop?

I don't recall adding something else, other than the C++ stdlib overlay (swiftstd) and the CxxShim module, both of which don't currently work on Windows.

@compnerd
Copy link
Collaborator

compnerd commented Mar 9, 2023

@stevapple could you rebase this change please?

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.

None yet

3 participants