compile-test: allow overriding nodejs binary location #37611
Conversation
|
r? @brson (rust_highfive has picked a reviewer for you, use r? to override) |
Mark-Simulacrum
force-pushed
the
tari-nodejs-runner-detect
branch
from
513a907
to
ed9a372
Compare
|
There already seems to be a |
|
☔ The latest upstream changes (presumably #37597) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Mh, there is some detection in |
|
Thanks for the PR! I agree with @TimNN that the best place for this is probably in rustbuild itself, we already need the node executable to run other unit test suites like the standard library. Perhaps this logic could be moved there and continue to be communicated to compiletest via this flag? |
|
@Mark-Simulacrum thoughts about updating to place this logic in rustbuild's detection and passing it down to compiletest that way? |
|
@alexcrichton: I'm pretty sure that logic is already present in rustbuild, isn't it? See https://github.com/rust-lang/rust/blob/master/src/bootstrap/sanity.rs#L84-L89: // Look for the nodejs command, needed for emscripten testing
if let Some(node) = have_cmd("node".as_ref()) {
build.config.nodejs = Some(node);
} else if let Some(node) = have_cmd("nodejs".as_ref()) {
build.config.nodejs = Some(node);
} |
Mark-Simulacrum
force-pushed
the
tari-nodejs-runner-detect
branch
2 times, most recently
from
e9d9d46
to
c3ed357
Compare
|
I think I updated this as @alexcrichton suggested (with rustbuild), but I don't have a good way to test, since I don't use rustbuild locally. If someone can give me a set of commands to run then I can run those. |
| @@ -86,6 +86,10 @@ pub fn check(build: &mut Build) { | |||
| build.config.nodejs = Some(node); | |||
| } else if let Some(node) = have_cmd("nodejs".as_ref()) { | |||
| build.config.nodejs = Some(node); | |||
| } else if let Some(node) = have_cmd("node.exe".as_ref()) { | |||
There was a problem hiding this comment.
No need for those extra checks, have_cmd will automatically consider .exe as well for each command.
|
I don't think the current implementation needs much adjusting -- the only problem I can see with it is that it does not allow specifying the path to node in |
|
Hmm, yeah, I see that now. Can someone point me to where I can make those changes (if we want to)? Otherwise we can just close this. |
| @@ -110,6 +110,9 @@ pub fn compiletest(build: &Build, | |||
|
|
|||
| if let Some(nodejs) = build.config.nodejs.as_ref() { | |||
| cmd.arg("--nodejs").arg(nodejs); | |||
| } else { | |||
| // FIXME: Print a message about being unable to detect a nodejs binary; | |||
| // and suggest a fix (passing --nodejs to configure?). | |||
There was a problem hiding this comment.
I think it's fine to leave this kind of error reporting to compiletest rather than in rustbuild itself
| @@ -173,6 +173,9 @@ pub struct Config { | |||
| // status whether android device available or not | |||
| pub adb_device_status: bool, | |||
|
|
|||
| // Name with which to invoke Node for asmjs targets | |||
| pub node_path: Option<String>, | |||
There was a problem hiding this comment.
This should be set at some point, right? (from the command-line flag)
There was a problem hiding this comment.
There already is a nodejs option for compiletest (a few lines down).
|
@Mark-Simulacrum you can take a look at |
|
Also you'll probably want to update the logic here to only try to auto detect node if it has not been specified by the user (somewhat like it's done for gdb just below). |
Mark-Simulacrum
force-pushed
the
tari-nodejs-runner-detect
branch
3 times, most recently
from
bfd3c58
to
f752c98
Compare
|
Alright, rebased and [hopefully] made this work with the comments suggested above. |
|
@bors: r+ Thanks! |
|
📌 Commit f752c98 has been approved by |
Allow passing a custom nodejs directory in configure.
Mark-Simulacrum
force-pushed
the
tari-nodejs-runner-detect
branch
from
f752c98
to
c524c50
Compare
|
@bors did not seem to notice this was r+ed. Edit: Bors noticed, but I pushed afterwards for some reason. I'm not sure why. |
|
@bors r=alexcrichton |
|
📌 Commit c524c50 has been approved by |
Mark-Simulacrum
changed the title
Add a command-line argument to manually specify which nodejs binary should be used,
which disables the default search.
Original work done by @tari.
Fixes #34188.