-
Notifications
You must be signed in to change notification settings - Fork 877
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
[WIP] MSI Installer #635
[WIP] MSI Installer #635
Conversation
There is another thing to be clarified here: I have added a feature flag
|
😮 I will review soon. |
OK, I'm so sorry it's taken me so long to get back to this @Boddlnagg! This looks amazing. The directory structure you've laid out looks fine, for now at least. Perhaps at some point we can put all this stuff into a workspace so we don't end up dumping an extra target directory under Once the msi installer is ready to go I expect it to be the only way to install rustup on windows. So ultimately we won't need to make a distinction. But we will need to get the msi-rustup deployed alongside non-msi-rustup in the interim while we test it and develop an upgrade path. A scheme that might work here is to name this new binary It doesn't look like this iteration is invasive at all. Would it be reasonable to merge it as-is and keep iterating? Again, I'm really sorry to block you on this for so long. |
Oh, I may not understand the issues around the hash. Presumably the installation artifact once this is all done will be If that's the case then I would expect there to be no sha256 or verification of the executable at all, but instead to verify the hash of the msi. Does that make sense? |
@brson: Yes, the installation artifact will be |
@brson wrote:
I would like to add the necessary changes to |
80b64b5
to
5035bcc
Compare
Apparently the version of |
Ah thanks for clarification about the hashing issue. So in msi-world the only thing we need the hash for is determining whether an update is available. I'd say give it a unique name, like That's funny that we're still pinned to such an old toolchain. There's no reason you shouldn't be able to switch it to |
I went ahead and opened a PR just to do the toolchain upgrade #651 |
Unfortunately, |
I wonder if it's really necessary to build for all four targets on AppVeyor. Which one is served at https://win.rustup.rs/ ? Right now I've configured it so that the MSI is only built on @brson wrote:
Sound good to me. I thought about a possible transition strategy: When the MSI installer is ready, we can change the Update: I just noticed that this won't work because we can't expect users to be using the latest pre-MSI version of rustup which contains the updated upgrade logic. But still the MSI installer should be able to deal gracefully with an existing pre-MSI rustup installation without deleting installed toolchains or the cargo cache. |
And another comment (sorry ...): I already ran into issues with diverging |
We could stop building all but one, though I like knowing they all build. i686-pc-windows-gnu is the one served by default. We use GNU by default because MSVC requires the CRT. MSVC would be better.
I think we can. The old rustup will eventually upgrade itself to a rustup-init.exe that understands how to find the msi.
I'll upgrade to nightly. |
Rather, because MSVC dynamically links a version of the CRT that doesn't come with every version of Windows we support. The only thing that needs to not depend on a component that might not be installed is the installer itself. It can install the necessary bits though so rustup itself could dynamically link to CRT stuff just fine. If the installer has Rust code in it though, then it would be a bit tricky, possibly requiring rust-lang/libc#290 to be fixed. |
Ugh ... yes, there actually will be Rust code running inside the installer. So the recommended way is to use MSVC and link statically, but that's currently not supported according to @retep998? With GNU there is the problem that we want to (statically) link the WiX Custom Actions helper library One can always embed the merge module for the VC Runtime inside the MSI, but that won't help us here, because we run Rust code quite early during the installation procedure and the Runtime would not have been installed by then (also it would probably lead to problems on uninstall, when the VC Runtime is deleted before we've finished executing the Rust code). At least this allows us to install a dynamically linked MSVC version of @brson wrote:
Thanks. Should I setup a cargo workspace right away? (Update: I just went ahead and did it, after rebasing on top of the version update.) |
You can statically link the CRT just fine as long as you pass to the linker
I'm not sure how soon I'll be able to get to this due to the winapi 0.3 overhaul I'm working on. |
|
I doubt |
@retep998 Okay, I was reading through that RFC thread and in search of a workaround I tried, based on a comment by @vadimcn (without actually knowing what I'm doing):
This seems to work as DependencyWalker doesn't show a dependency on Update: This also seems to work and gets rid of a few more dynamic dependencies:
and
Only overriding Update 2: Okay, I added some more code to the library so that it actually does something, and now the linker complains about unresolved symbols |
5b2fe2c
to
c426cae
Compare
@retep998 Using a local patched version of I don't know what the best way forward would be right now (as long as rust-lang/rfcs#1684 is not resolved). I could try to use the MinGW toolchain, i.e. drop the dependency on the WiX CA helper library |
@Boddlnagg Trying to build on msvc to see the dependency issues. How is the workspace set up (it looks to me like the top-level Cargo.toml is not a workspace but I don't know much about it)? Are your patches to do the static linking pushed? It looks like no. Can you push another commit with those? In the meantime I'll try to reproduce based on your description of the solution. Until we come up with a solution I think it's fine for us to merge with dynamic linking and just have it break when the crt dylib isn't available. We've got more iteration yet before we have to deploy. |
The top level Cargo.toml defines the workspace, see the
I didn't push anything to do the static linking, because I can't get it to work. But it's really only about adding a few lines to
So you would be okay with not being able to build on the GNU toolchain? |
As long as we continue to have GNU builds for the non-msi rustup-init.exe, yes. I think that's the situation here, right? It leaves all other build configurations intact, creating one new one for i686-pc-windows-msvc for the msied rustup. |
Yes, that's right. Then you could merge this as-is. AppVeyor builds fine, although it prints some output in red because cargo sends status messages to stderr and PowerShell thinks that having anything written to stderr means that an error occurred 😒 |
OK. I'm going to go ahead and merge this as-is then. It's great progress. |
The build failed, but totally unrelated, probably due to a change in the latest nightly. |
Looks like the breakage will be fixed in the next nightly rust-lang/rust#35721 |
This is some initial setup and prototyping work for the rustup MSI installer. I don't think this is ready to be merged (*), but I wanted to collect some feedback on the general structure and see how it works with the CI (but I haven't touched
appveyor.yml
yet).I created the
rustup-win-installer
project withinsrc
, which is the library containing the custom actions, and themsi
subdirectory contains the WiX/MSI source files. The WiX tools are invoked by a Powershell script (build.ps1
) and the final MSI is placed into\src\rustup-win-installer\msi\target
. I'm not sure whether it should actually be the other way round, because the WiX project depends on the custom actions library (and on the rustup binary, which is expected to be found at\target\release\rustup-init.exe
).The gfx is copied from rust-packaging.
(*) It currently only installs
rustup.exe
into%USERPROFILE%\.rustup-test\bin
, because I don't know how to easily test it without overwriting my actual rustup installation.