Fix build under MSYS2/MINGW64 #54

vvuk · 2015-09-22T15:27:33Z

This is a set of changes needed to build mozjs under msys2/mingw64.

Core changes:

Allow building with msys make
Don't strip "lib" prefix from library names
Use system nspr with correct lib names (nspr-config gives incorrect values)
Force using native win32 python instead of the mingw64/msys python

I'm not really happy with these changes since these are all basically core m-c js files, and I don't think we could take some of these changes in m-c. But, the majority of changes are correct or are resolving deeper issues in the m-c build system (e.g. the need to use the native win32 python instead of the msys one).

The ICU libprefix stuff can probably be resolved a different way, I just wasn't sure how to go about it.

metajack · 2015-09-22T15:56:07Z

I'm mostly concerned about the upstream build system modifications. Have you talked to the JS team about these changes and issues?

cc @tschneidereit

Reviewed 7 of 7 files at r1.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

^{makefile.cargo, line 49 [r1] (raw file):}
Is this path guaranteed to exist or just where you happened to install the tool?

^{makefile.cargo, line 58 [r1] (raw file):}
Our other tools are tolerant of python3 vs. python2 stuff. Is it possible not to hardcode a python version here?

Also this is the only difference in the two branches. Could we pull that out and then only have one all rule?

^{mozjs/config/baseconfig.mk, line 34 [r1] (raw file):}
We don't typically change these files since they are from upstream. Does upstream specifically forbid msys?

^{mozjs/config/external/icu/Makefile.in, line 30 [r1] (raw file):}
is the libs intentional? This looks like a typo. For example it's lib/s in the else branch.

Comments from the review on Reviewable.io

tschneidereit · 2015-09-22T16:24:08Z

It'd be great if you could split these changes into upstreamable and hacky bits. Then we can take the former on mozilla-central and deal with the hacky bits later. It's not like we don't have build system hacks already.

Ms2ger · 2015-09-24T17:33:48Z

And rebase, please.

vvuk · 2015-09-24T18:16:09Z

Rebased and commits split out.

The missing #! to milestone.py, and the allowing of msys make for mingw64/msys2 could be upstreamed. The latter patch would ideally need a reliable way of actually detecting MSYS2 -- mozilla-build currently has $MSYSTEM = MINGW32 for historical raisins, even though that's mostly a lie.

The ICU changes are... correct, but it depends on what you're trying to accomplish. If we're trying to use mingw to create windows-style libraries, then they're not correct (we don't want lib* as a prefix). But if we're trying to make static libs, then they are. The right thing here might be to have an entirely separate platform config file that's something like mingw64-msys2, and then find a way to explicitly tell ICU to use that config.

The makefile.cargo fixes are unfortunate, for the python change. But I couldn't find any way to get around it, given that the spidermonkey tree has its own copy of virtualenv.py that it really wants to use.

adamncasey · 2015-10-18T22:42:36Z

Would be nice to see this added soon, so servo on windows can move forward

jdm · 2015-10-19T14:40:51Z

I'd like @metajack to look at the new changes, but he's out all week :/

metajack · 2015-10-19T18:40:59Z

I appreciate the extra comments in the weird spots. There are still two comments that appear unanswered.

In makefile.cargo on line 49 there is a hardcoded path, which I asked if that was guaranteed to be there.

In the icu makefile there seems to be a typo between "lib" and "libs" in the two branches. Is that intentional or an error?

Reviewed 5 of 6 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

^{build.rs, line 23 [r3] (raw file):}
Why do we only need to link nspr on windows?

Comments from the review on Reviewable.io

vvuk · 2015-10-20T18:42:32Z

In makefile.cargo on line 49 there is a hardcoded path, which I asked if that was guaranteed to be there.

The default install location is now hardcoded, along with a NATIVE_WIN32_PYTHON env var fallback. There's a message that gets printed if python can't be found using either of the two.

In the icu makefile there seems to be a typo between "lib" and "libs" in the two branches. Is that intentional or an error?

Intentional -- both branches are moving libraries that have an "s" prefix to ones that don't (libsicu -> libicu, or sicu -> icu).

build.rs, line 23 [r3](raw file): Why do we only need to link nspr on windows?

JS doesn't have the "mini nspr" for Windows that posixy platforms have in vm/PosixNSPR.{cpp,h} We need to link with real nspr until someone writes that code... it shouldn't be difficult to do, other than the lack of native condvars on Windows.

metajack · 2015-10-20T23:33:23Z

The path I am talking about is /mingw64/include/nspr which is not related to python. What about that path?

Other than that, you've answered the questions I had.

vvuk · 2015-10-27T13:11:15Z

Oh, sorry, was looking at the wrong line. That path is where the nspr includes will be from the mingw64 nspr package. They will always be in that location (/mingw64/include/nspr will get mapped to wherever the base msys2 install happened in the actual filesystem).

metajack · 2015-10-27T15:36:34Z

Squash and r=me

Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

Comments from the review on Reviewable.io


        win32: change ICU build on mingw/mingw64, to not strip 'lib' prefix


        win32: on MINGW64/MSYS2, the library names now consistently start wit…

…h lib


        Add missing #! to milestone.py


        win32: allow msys make when in mingw64/msys2

vvuk · 2015-10-27T17:52:16Z

I'm working on changing the linking here a bit -- figured out how to link with nspr statically, to avoid the DLL deps. I'll update this PR once things check out.


        win32: build.rs & makefile.cargo fixes


        Remove bogus inline that's causing Windows build issues (missing symb…

…ol in import library)

vvuk · 2015-10-27T19:24:26Z

static nspr linking didn't work out -- it has issues with threading, since some of the nspr threading functionality depends on DllMain being notified of thread attach/detach.

metajack · 2015-11-03T21:32:58Z

@vvuk Is this ready for re-review?

vvuk · 2015-11-11T20:39:06Z

Sorry, yes -- this should be ready for re-review.

metajack · 2015-11-11T21:09:23Z

@bors-servo r+

Reviewed 4 of 4 files at r4.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from the review on Reviewable.io

jdm · 2015-11-11T21:21:57Z

@bors-servo: r=metajack

bors-servo · 2015-11-11T21:21:57Z

📌 Commit 268f7ac has been approved by metajack

bors-servo · 2015-11-11T21:21:58Z

⌛ Testing commit 268f7ac with merge 446871d...


        Auto merge of #54 - vvuk:win32, r=metajack

Fix build under MSYS2/MINGW64 This is a set of changes needed to build mozjs under msys2/mingw64. Core changes: - Allow building with msys make - Don't strip "lib" prefix from library names - Use system nspr with correct lib names (nspr-config gives incorrect values) - Force using native win32 python instead of the mingw64/msys python I'm not really happy with these changes since these are all basically core m-c js files, and I don't think we could take some of these changes in m-c. But, the majority of changes are correct or are resolving deeper issues in the m-c build system (e.g. the need to use the native win32 python instead of the msys one). The ICU libprefix stuff can probably be resolved a different way, I just wasn't sure how to go about it.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/mozjs/54)

bors-servo · 2015-11-11T21:33:23Z

☀️ Test successful - travis

vvuk force-pushed the vvuk:win32 branch from 297406e to bb3dbb9 Sep 24, 2015

vvuk force-pushed the vvuk:win32 branch 2 times, most recently from 68185cd to 1d13f00 Sep 24, 2015

vvuk force-pushed the vvuk:win32 branch from 1d13f00 to 197cbb7 Sep 29, 2015

jdm assigned metajack Oct 19, 2015

vvuk added 4 commits Sep 24, 2015

win32: change ICU build on mingw/mingw64, to not strip 'lib' prefix

5a94194

win32: on MINGW64/MSYS2, the library names now consistently start wit…

08d813d

…h lib

Add missing #! to milestone.py

b21e094

win32: allow msys make when in mingw64/msys2

fa4659f

vvuk added 2 commits Sep 24, 2015

win32: build.rs & makefile.cargo fixes

90fe130

Remove bogus inline that's causing Windows build issues (missing symb…

Loading status checks…

268f7ac

…ol in import library)

vvuk force-pushed the vvuk:win32 branch from 265c176 to 268f7ac Oct 27, 2015

metajack mentioned this pull request Nov 3, 2015

Get servo working on win32 servo/servo#7878

Closed

bors-servo mentioned this pull request Nov 11, 2015

Updates to make mozjs work with newer SM versions #56

Closed

servo / mozjs

Fix build under MSYS2/MINGW64 #54

Fix build under MSYS2/MINGW64 #54

vvuk commented Sep 22, 2015

metajack commented Sep 22, 2015

tschneidereit commented Sep 22, 2015

Ms2ger commented Sep 24, 2015

vvuk commented Sep 24, 2015

adamncasey commented Oct 18, 2015

jdm commented Oct 19, 2015

metajack commented Oct 19, 2015

vvuk commented Oct 20, 2015

metajack commented Oct 20, 2015

vvuk commented Oct 27, 2015

metajack commented Oct 27, 2015

vvuk commented Oct 27, 2015

vvuk commented Oct 27, 2015

metajack commented Nov 3, 2015

vvuk commented Nov 11, 2015

metajack commented Nov 11, 2015

jdm commented Nov 11, 2015

bors-servo commented Nov 11, 2015

bors-servo commented Nov 11, 2015

bors-servo commented Nov 11, 2015

servo / mozjs

Join GitHub today

Fix build under MSYS2/MINGW64 #54

Fix build under MSYS2/MINGW64 #54

Conversation

vvuk commented Sep 22, 2015

metajack commented Sep 22, 2015

tschneidereit commented Sep 22, 2015

Ms2ger commented Sep 24, 2015

vvuk commented Sep 24, 2015

adamncasey commented Oct 18, 2015

jdm commented Oct 19, 2015

metajack commented Oct 19, 2015

vvuk commented Oct 20, 2015

metajack commented Oct 20, 2015

vvuk commented Oct 27, 2015

metajack commented Oct 27, 2015

vvuk commented Oct 27, 2015

vvuk commented Oct 27, 2015

metajack commented Nov 3, 2015

vvuk commented Nov 11, 2015

metajack commented Nov 11, 2015

jdm commented Nov 11, 2015

bors-servo commented Nov 11, 2015

bors-servo commented Nov 11, 2015

bors-servo commented Nov 11, 2015