Compile successfully on FreeBSD #80

dlrobertson · 2016-06-05T21:00:50Z

Add static MAKE_CMD variable for the make command
replace instances of make with $(MAKE)

This change is

mbrubeck · 2016-06-05T21:05:57Z

build.rs

 fn main() {
    let out_dir = env::var("OUT_DIR").unwrap();
    let target = env::var("TARGET").unwrap();
-    let result = Command::new("make")
+    let result = Command::new(MAKE_CMD)


Maybe this should use the $MAKE env var if it's set (and fall back to the constant if it's not), so people on other BSDs or similar systems can more easily build without patching the build script.

dlrobertson · 2016-06-05T21:59:29Z

build.rs

+fn find_make() -> String {
+    Command::new("gmake").status()
+                         .map(|_| "gmake".to_string())
+                         .unwrap_or("make".to_string())


Is this the best way to find if gmake exists?

I'll let @mbrubeck do the final review, though if it works it's probably ok.

For reference, this is what it's used to find python in the style crate, though your version looks way cleaner.

emilio · 2016-06-05T22:26:09Z

build.rs

 fn main() {
    let out_dir = env::var("OUT_DIR").unwrap();
    let target = env::var("TARGET").unwrap();
-    let result = Command::new("make")
+    let result = Command::new(env::var("MAKE").unwrap_or(find_make()))


nit: unwrap_or_else(find_make) won't call the function if MAKE is found.

Ah! good call

emilio · 2016-06-05T23:35:51Z

build.rs

 fn main() {
    let out_dir = env::var("OUT_DIR").unwrap();
    let target = env::var("TARGET").unwrap();
-    let result = Command::new("make")
+    let result = Command::new(env::var("MAKE").unwrap_or_else(find_make()))


nit again: you're still calling the function, remove the two parens because it's not building now :P

dlrobertson · 2016-06-05T23:47:22Z

Updated

mbrubeck · 2016-06-06T17:59:15Z

@bors-servo delegate+

r=mbrubeck with or without some optional suggestions below

Previously, danlrobertson (Dan Robertson) wrote…

Updated

Reviewed 1 of 2 files at r1, 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

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

Previously, emilio (Emilio Cobos Álvarez) wrote…

I'll let @mbrubeck do the final review, though if it works it's probably ok.

For reference, this is what it's used to find python in the style crate, though your version looks way cleaner.

This seems fine to me. My only other idea is to iterate through `["gmake", "make"]` and try running the actual command with each one, continuing if it returns an `std::io::ErrorKind::NotFound` and breaking on any other result.

build.rs, line 18 [r3] (raw file):

Previously, emilio (Emilio Cobos Álvarez) wrote…

nit again: you're still calling the function, remove the two parens because it's not building now :P

nit: Could use `env::var_os` here, I think.

build.rs, line 11 [r4] (raw file):

fn find_make() -> String {
    Command::new("gmake").status()
                         .map(|_| "gmake".to_string())

Personally I'd find a match slightly more readable than map/unwrap_or here, but it's up to you.

Comments from Reviewable

bors-servo · 2016-06-06T17:59:16Z

✌️ @danlrobertson can now approve this pull request

dlrobertson · 2016-06-10T00:55:39Z

Thanks for the good suggestions!

Previously, mbrubeck (Matt Brubeck) wrote…

@bors-servo delegate+

r=mbrubeck with or without some optional suggestions below

Review status: all files reviewed at latest revision, 4 unresolved discussions.

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

Previously, mbrubeck (Matt Brubeck) wrote…

This seems fine to me. My only other idea is to iterate through ["gmake", "make"] and try running the actual command with each one, continuing if it returns an std::io::ErrorKind::NotFound and breaking on any other result.

This would add more information in the case that neither one is present (although this is almost certainly very rare). I like the idea, but I think I'll stick with a `match` and `Command::status`

build.rs, line 18 [r3] (raw file):

Previously, mbrubeck (Matt Brubeck) wrote…

nit: Could use env::var_os here, I think.

I'll try it out and see what happens!

build.rs, line 11 [r4] (raw file):

Previously, mbrubeck (Matt Brubeck) wrote…

Personally I'd find a match slightly more readable than map/unwrap_or here, but it's up to you.

Now that I think about it, I agree.

Comments from Reviewable

bors-servo · 2016-06-10T00:55:41Z

✌️ @danlrobertson can now approve this pull request

bors-servo · 2016-06-10T00:57:05Z

✌️ @danlrobertson can now approve this pull request

dlrobertson · 2016-06-10T00:57:29Z

^ highfive bug?

jdm · 2016-06-10T07:43:26Z

Homu instead of highfive, but yes.


        Compile successfully on FreeBSD

- Add static MAKE_CMD variable for the make command - replace instances of `make` with $(MAKE)

dlrobertson · 2016-06-10T21:01:49Z

@bors-servo r=mbrubeck

Previously, jdm (Josh Matthews) wrote…

Homu instead of highfive, but yes.

Review status: 1 of 2 files reviewed at latest revision, 4 unresolved discussions.

Comments from Reviewable

bors-servo · 2016-06-10T21:01:50Z

📌 Commit 5f81658 has been approved by mbrubeck

bors-servo · 2016-06-10T21:01:51Z

⌛ Testing commit 5f81658 with merge 67b5e70...


        Auto merge of #80 - danlrobertson:freebsd, r=mbrubeck

Compile successfully on FreeBSD - Add static MAKE_CMD variable for the make command - replace instances of `make` with $(MAKE)  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/mozjs/80)

bors-servo · 2016-06-10T21:08:47Z

☀️ Test successful - travis

dlrobertson · 2016-06-10T21:43:03Z

servo/angle#8 failed on appveyor... currently investigating

mbrubeck reviewed Jun 5, 2016
View changes

mbrubeck self-assigned this Jun 5, 2016

dlrobertson force-pushed the dlrobertson:freebsd branch from 27d1bb1 to d5114d8 Jun 5, 2016

dlrobertson reviewed Jun 5, 2016
View changes

emilio reviewed Jun 5, 2016
View changes

dlrobertson force-pushed the dlrobertson:freebsd branch from d5114d8 to 834cf11 Jun 5, 2016

emilio reviewed Jun 5, 2016
View changes

dlrobertson force-pushed the dlrobertson:freebsd branch from 834cf11 to d294feb Jun 5, 2016

Compile successfully on FreeBSD

Loading status checks…

5f81658

- Add static MAKE_CMD variable for the make command - replace instances of `make` with $(MAKE)

dlrobertson force-pushed the dlrobertson:freebsd branch 2 times, most recently from 94bc6a5 to 5f81658 Jun 10, 2016

bors-servo merged commit 5f81658 into servo:master Jun 10, 2016
2 of 3 checks passed

2 of 3 checks passed

code-review/reviewable 1 file, 4 discussions left (emilio, mbrubeck)
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

dlrobertson deleted the dlrobertson:freebsd branch Nov 11, 2016

servo / mozjs

Compile successfully on FreeBSD #80

Compile successfully on FreeBSD #80

dlrobertson commented Jun 5, 2016 •

edited by larsbergstrom

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

dlrobertson commented Jun 5, 2016

mbrubeck commented Jun 6, 2016

bors-servo commented Jun 6, 2016

dlrobertson commented Jun 10, 2016 •

edited

bors-servo commented Jun 10, 2016

bors-servo commented Jun 10, 2016

dlrobertson commented Jun 10, 2016

jdm commented Jun 10, 2016

dlrobertson commented Jun 10, 2016

bors-servo commented Jun 10, 2016

bors-servo commented Jun 10, 2016

bors-servo commented Jun 10, 2016

dlrobertson commented Jun 10, 2016

servo / mozjs

Join GitHub today

Compile successfully on FreeBSD #80

Compile successfully on FreeBSD #80

Conversation

dlrobertson commented Jun 5, 2016 • edited by larsbergstrom

This comment has been minimized.

mbrubeck Jun 5, 2016

This comment has been minimized.

dlrobertson Jun 5, 2016

This comment has been minimized.

emilio Jun 5, 2016

This comment has been minimized.

emilio Jun 5, 2016

This comment has been minimized.

dlrobertson Jun 5, 2016

This comment has been minimized.

emilio Jun 5, 2016

dlrobertson commented Jun 5, 2016

mbrubeck commented Jun 6, 2016

bors-servo commented Jun 6, 2016

dlrobertson commented Jun 10, 2016 • edited

bors-servo commented Jun 10, 2016

bors-servo commented Jun 10, 2016

dlrobertson commented Jun 10, 2016

jdm commented Jun 10, 2016

dlrobertson commented Jun 10, 2016

bors-servo commented Jun 10, 2016

bors-servo commented Jun 10, 2016

bors-servo commented Jun 10, 2016

dlrobertson commented Jun 10, 2016

dlrobertson commented Jun 5, 2016 •

edited by larsbergstrom

dlrobertson commented Jun 10, 2016 •

edited