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

Windows: updated instructions #11392

Closed
wants to merge 1 commit into from
Closed

Windows: updated instructions #11392

wants to merge 1 commit into from

Conversation

@perlun
Copy link
Contributor

perlun commented May 24, 2016

Details about changes:

  • MSYS2: Removed the reference to the update-core script, which has been removed in MSYS2: Alexpux/MSYS2-pacman#26.
  • Added more details about strange prompts that might appear. :)
  • Added more details on how to install Python correctly.

With these changes in place, I am successfully able to at least get the build starting. (getting non-Python related errors, but will file a separate issue about that)


This change is Reviewable

@perlun
Copy link
Contributor Author

perlun commented May 24, 2016

@highfive highfive assigned metajack and unassigned nox May 24, 2016

Install MSYS2 from [here](https://msys2.github.io/). After you have done so, open an MSYS shell
window and update the core libraries and install new packages:

```sh
update-core
pacman -Su
pacman -Sy git mingw-w64-x86_64-toolchain mingw-w64-x86_64-freetype \

This comment has been minimized.

@aneeshusa

aneeshusa May 24, 2016

Member

Partial upgrades are unsupported by pacman (at least natively on Arch, not sure about on MSYS). We should combine these two lines into pacman -Syu git mingw-w64-x86_64-toolchain ....

@metajack
Copy link
Contributor

metajack commented May 24, 2016

This looks good, but have you verified this works on a fresh install? I seem to recall that deep withing some configure script it relied on mingw python not the system python. this seems to remove the instructions to install mingw python (and then let native python through for python2.7). I'm very happy if this is actually possible, but skeptical considering how much fighting @larsbergstrom and @vvuk did with this.

If it does work, this is r=me with the nit from @aneeshusa fixed.

Previously, perlun (Per Lundberg) wrote…

r? @metajack


Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


README.md, line 84 [r1] (raw file):

Previously, aneeshusa (Aneesh Agrawal) wrote…

Partial upgrades are unsupported by pacman (at least natively on Arch, not sure about on MSYS). We should combine these two lines into pacman -Syu git mingw-w64-x86_64-toolchain ....

👍 x

Comments from Reviewable

@vvuk
Copy link
Contributor

vvuk commented May 25, 2016

Oops, sorry; it's late and I didn't see the change to msys2 to have pacman natively understand system updates. Carry on :p

@aneeshusa
Copy link
Member

aneeshusa commented May 25, 2016

@vvuk Actually, I looked into a bit more after your comment and I think my earlier comment may not be correct. It seems that after a pacman -Syu, it may be required to restart MSYS (and maybe run pacman -Syu again? not sure) before installing packages.

See commonquail/pa#2 and the warnings in the image from Alexpux/MSYS2-pacman#26.

@perlun
Copy link
Contributor Author

perlun commented May 25, 2016

Thanks for the feedback. Will retry with these instructions on a fresh MSYS2 install just to ensure that they are fully followable. 😄

@vvuk
Copy link
Contributor

vvuk commented May 25, 2016

@aneeshusa Yeah, I'm guessing the "native support" just does an update to the core packages first, and then prompts you to restart.. after that, a second update would be required to take care of the rest of the packages. It's better than before, where doing a full update that picked up core packages would cause a lot of stuff after it to fail, leaving the system in an inconsistent state.

@highfive
Copy link

highfive commented May 25, 2016

New code was committed to pull request.

Per Lundberg
* MSYS2: Removed the reference to the update-core script, which has been removed in MSYS2: Alexpux/MSYS2-pacman#26.
* Added more details about strange prompts that might appear. :)
* Added more details on how to install Python correctly.

With these changes in place, I am successfully able to at least get the build starting. (getting non-Python related errors, but will file a separate issue about that)
@perlun perlun force-pushed the perlun:patch-2 branch from 0d6d55f to 61dc2f5 May 25, 2016
@highfive
Copy link

highfive commented May 25, 2016

New code was committed to pull request.

@perlun
Copy link
Contributor Author

perlun commented May 31, 2016

I'm very happy if this is actually possible, but skeptical considering how much fighting @larsbergstrom and @vvuk did with this.

You're right, it seems to not fully work; I am getting errors in mozjs_sys which seems to have some of its own magic going on. I will try to see if that's easily solvable upstream. Relying on a single Python would, like you say, be a more natural approach to the build tooling.

@metajack
Copy link
Contributor

metajack commented Jun 9, 2016

@perlun Any new info on this?

@perlun
Copy link
Contributor Author

perlun commented Jun 10, 2016

@perlun Any new info on this?

Ah, sorry for lack of feedback on my behalf.

I tried to get `mozjs_sysz to compile using a native Python; managed to get some stuff working but ended up banging my head into the wall because of lack of enough Python-fu skills. 😇 So, I couldn't get it working and kind of gave up; haven't tried anything more the last few days.

So, it wasn't completed.

I wonder if we should try it the other way around instead? I.e. make it work with the MSYS/MINGW-provided Python instead, to remove the need to require a native Python. The text indicates that this doesn't work "out of the box" but does anyone (you or others) have an idea of how much work it would take? I would be willing to experiment a bit there I think.

I'm also kind of wondering: since you are doing a bit of work on the "other" Windows-based toolchain support (i..e MSVC/Visual Studio), which one is going to be "the way" going forward? Do we intend to support both of them (like rustc does) or is the plan to pick the one we feel is best and drop support for the other one? To put it bluntly: would it make more sense that I help out in getting the MSVC-based port more complete? What do you think?

@metajack
Copy link
Contributor

metajack commented Jun 10, 2016

The best scenario would be to support both with msvc builds requiring normal python, and msys builds requiring msys python, but not having both.

I'd also probably be fine supporting only msvc builds since that is probably what people will want given the debugging situation. I'll ask others.

In the meantime, should we close this?

perlun pushed a commit to perlun/servo that referenced this pull request Jun 10, 2016
It's `pacman -Su` nowadays, per Alexpux/MSYS2-pacman#26.

(This is a very selective cherry-pick of servo#11392. The other parts were more controversial; they didn't fully work so let's disregard them for now.)
perlun pushed a commit to perlun/servo that referenced this pull request Jun 10, 2016
It's `pacman -Su` nowadays, per Alexpux/MSYS2-pacman#26.

(This is a very selective cherry-pick of servo#11392. The other parts were more controversial; they didn't fully work so let's disregard them for now.)
@perlun
Copy link
Contributor Author

perlun commented Jun 10, 2016

The best scenario would be to support both with msvc builds requiring normal python, and msys builds requiring msys python, but not having both.

Alright, that sounds reasonable. I'll see how hard it would be to get the msys build working with msys python.

In the meantime, should we close this?

Yes. I opened a new one with the update-core change, since that should be a no-brainer to merge. Thanks for taking some time to ensure we get things closed & moving forward! Good for everybody.

@perlun perlun closed this Jun 10, 2016
@perlun perlun deleted the perlun:patch-2 branch Jun 10, 2016
bors-servo added a commit that referenced this pull request Jun 10, 2016
Removed reference to update-core

It's `pacman -Su` nowadays, per Alexpux/MSYS2-pacman#26.

(This is a very selective cherry-pick of #11392. The other parts were more controversial; they didn't fully work so let's disregard them for now.)

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11718)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this pull request Jun 12, 2016
Removed reference to update-core

It's `pacman -Su` nowadays, per Alexpux/MSYS2-pacman#26.

(This is a very selective cherry-pick of #11392. The other parts were more controversial; they didn't fully work so let's disregard them for now.)

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11718)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this pull request Jun 12, 2016
Removed reference to update-core

It's `pacman -Su` nowadays, per Alexpux/MSYS2-pacman#26.

(This is a very selective cherry-pick of #11392. The other parts were more controversial; they didn't fully work so let's disregard them for now.)

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11718)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this pull request Jun 12, 2016
Removed reference to update-core

It's `pacman -Su` nowadays, per Alexpux/MSYS2-pacman#26.

(This is a very selective cherry-pick of #11392. The other parts were more controversial; they didn't fully work so let's disregard them for now.)

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11718)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.