-
Notifications
You must be signed in to change notification settings - Fork 944
Fix and re-enable MSI build #1211
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
Conversation
I want to see if this works on AppVeyor, but there seems to be a problem with AppVeyor right now ... |
6af50fb
to
34694ac
Compare
☔ The latest upstream changes (presumably #1230) made this pull request unmergeable. Please resolve the merge conflicts. |
Thanks for this @Boddlnagg and sorry for the delay! It looks like this is just a few fixes here and there, so if you'd like to rebase and remove the temporary commits, I can r+ to land this |
d864993
to
e0ce4c6
Compare
@alexcrichton Thanks! I did the rebase. |
@@ -5,9 +5,8 @@ environment: | |||
- TARGET: x86_64-pc-windows-msvc | |||
ALLOW_PR: 1 | |||
- TARGET: i686-pc-windows-msvc |
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.
Should this be removed now that BUILD_MSI is enabled?
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 MSI version is nowhere near finished. I want to enable it on CI just so it won't break. Maybe it would make sense to also set ALLOW_PR: 1
for it.
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.
Ah ok, that makes sense and no worries! My only worry is that this'll conflict with the other i686 builder for the dist artifacts being produced. Enabling the MSI installer just builds more artifacts, right? It shouldn't change the existing binaries?
(if so we can probably just gate on the msi build)
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.
No artifacts are being produced for MSI currently, see https://github.com/rust-lang-nursery/rustup.rs/blob/ac5bffad7ba126b2313692c3dfce83ec91cb7fea/ci/prepare-deploy-appveyor.ps1#L9-L12
Changing that situation was part of #661, but I didn't include it here. Will do in another PR.
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.
Thanks! Looks like CI is failing though?
src/rustup-win-installer/build.rs
Outdated
"VS2017" | ||
} else { | ||
panic!("Unknown VS version with compiler path {:?}", path); | ||
}; |
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.
Could you use find_vs_version instead? https://docs.rs/gcc/0.3.51/gcc/windows_registry/fn.find_vs_version.html
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.
Thanks! I hadn't seen that function.
The CI failure is weird:
Other crates in the same workspace use Edit: Ah, I see, it's missing a |
Oh nice! In that case... @bors: r+ |
📌 Commit eaed698 has been approved by |
Fix and re-enable MSI build This supersedes #661, though it does not enable deployment yet.
☀️ Test successful - status-appveyor, status-travis |
This supersedes #661, though it does not enable deployment yet.