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

[msvc] allow mozjs to be built using MSVC on windows #82

Merged
merged 6 commits into from Aug 5, 2016

Conversation

@vvuk
Copy link
Contributor

vvuk commented Jun 22, 2016

This change is Reviewable

@Ms2ger
Copy link
Collaborator

Ms2ger commented Jun 23, 2016

Seems like some of those should go upstream too.

@vvuk
Copy link
Contributor Author

vvuk commented Jun 23, 2016

WinNSPR isn't upstream yet :(
JS_PUBLIC_API stuff should though.

@vvuk vvuk force-pushed the vvuk:rust-msvc branch from 28dd1e6 to eaf5c83 Jul 12, 2016
@metajack
Copy link
Contributor

metajack commented Jul 14, 2016

Is WinNSPR something we made then? I don't understand.

@Ms2ger should sign off on this since he's in the middle of spidermonkey upgrade and these will need to be accounted for there too.

Build stuff looks fine to me.


Reviewed 6 of 7 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 3 unresolved discussions.


build.rs, line 35 [r2] (raw file):

            println!("cargo:rustc-link-lib=stdc++");
        }
        //println!("cargo:rustc-link-lib=mozjs-48a1");

Can we remove this commented out line?


makefile.cargo, line 46 [r2] (raw file):

        # Visual Studio build
  OUT_DIR:=$(shell cygpath "$(OUT_DIR)")
  CONFIGURE_FLAGS += --target=x86_64-pc-mingw32

The target is not MSVC?


makefile.cargo, line 51 [r2] (raw file):

  MOZ_TOOLS=/

  ifneq (,$(wildcard c:/python27/python.exe))

Can we factor this into a branch for both windows targets so we aren't duplicating these lines?


Comments from Reviewable

@vvuk vvuk force-pushed the vvuk:rust-msvc branch 6 times, most recently from bd54748 to d6325ad Jul 25, 2016
let path = env::var_os("PATH").unwrap();
let mut paths = env::split_paths(&path).collect::<Vec<_>>();
paths.push(PathBuf::from(moztools));
let new_path = env::join_paths(paths).unwrap();

This comment has been minimized.

@Ms2ger

Ms2ger Aug 1, 2016

Collaborator

Can we use Iterator::chain instead?

build.rs Outdated
if target.contains("gnu") {
println!("cargo:rustc-link-lib=stdc++");
}
//println!("cargo:rustc-link-lib=mozjs-48a1");

This comment has been minimized.

@Ms2ger

Ms2ger Aug 1, 2016

Collaborator

Rm

build.rs Outdated
println!("cargo:rustc-link-lib=static=js_static");
} else {
println!("cargo:rustc-link-lib=stdc++");
println!("cargo:rustc-link-lib=static=js_static");

This comment has been minimized.

@Ms2ger

Ms2ger Aug 1, 2016

Collaborator

Let's move this back out of the if/else

@Ms2ger
Copy link
Collaborator

Ms2ger commented Aug 1, 2016

You'll need to update etc/patches

@vvuk vvuk force-pushed the vvuk:rust-msvc branch from 8efbb36 to 59cb166 Aug 4, 2016
@vvuk
Copy link
Contributor Author

vvuk commented Aug 4, 2016

Review status: 2 of 8 files reviewed at latest revision, 3 unresolved discussions.


makefile.cargo, line 46 [r2] (raw file):

Previously, metajack (Jack Moffitt) wrote…

The target is not MSVC?

Not for gecko -- it uses the target as written and does internal stuff to figure out what compiler is being used. Gecko's build system is a mess, just nod and smile :|

makefile.cargo, line 51 [r2] (raw file):

Previously, metajack (Jack Moffitt) wrote…

Can we factor this into a branch for both windows targets so we aren't duplicating these lines?

Done

Comments from Reviewable

@metajack
Copy link
Contributor

metajack commented Aug 4, 2016

New changes look good to me. ms2ger, anything left you are waiting on?


Reviewed 1 of 5 files at r3, 2 of 2 files at r4, 3 of 3 files at r5.
Review status: all files reviewed at latest revision, 3 unresolved discussions.


Comments from Reviewable

@vvuk
Copy link
Contributor Author

vvuk commented Aug 4, 2016

New changes look good to me. ms2ger, anything left you are waiting on?

I'm not sure how to make Iterator::chain happen btw, would be happy to if someone can guide me :)

@Ms2ger
Copy link
Collaborator

Ms2ger commented Aug 5, 2016

@bors-servo r=metajack

@bors-servo
Copy link
Contributor

bors-servo commented Aug 5, 2016

📌 Commit 59cb166 has been approved by metajack

@bors-servo
Copy link
Contributor

bors-servo commented Aug 5, 2016

Testing commit 59cb166 with merge ac9fcbe...

bors-servo added a commit that referenced this pull request Aug 5, 2016
[msvc] allow mozjs to be built using MSVC on windows

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/mozjs/82)
<!-- Reviewable:end -->
@larsbergstrom
Copy link
Contributor

larsbergstrom commented Aug 5, 2016

@larsbergstrom larsbergstrom merged commit 977eb88 into servo:master Aug 5, 2016
1 of 3 checks passed
1 of 3 checks passed
code-review/reviewable 3 discussions left (metajack, vvuk)
Details
homu Testing commit 59cb166 with merge ac9fcbe...
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
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

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