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

Cargo should use Node.js to launch targets built for asmjs #3626

Closed
koute opened this issue Jan 31, 2017 · 21 comments · Fixed by #3954
Closed

Cargo should use Node.js to launch targets built for asmjs #3626

koute opened this issue Jan 31, 2017 · 21 comments · Fixed by #3954

Comments

@koute
Copy link
Member

koute commented Jan 31, 2017

Currently when doing cargo run --target=asmjs-unknown-emscripten or cargo test --target=asmjs-unknown-emscripten you get the following error:

error: could not execute process `target/asmjs-unknown-emscripten/debug/foobar.js` (never executed)

While the asmjs backend is mostly meant to be used on the Web it would be quite useful to have the targets compiled for the asmjs to be launched through Node.js, if nothing else for running tests.

There is, of course, a partial workaround for Linux where you could technically do this:

echo ":js:E::js::/usr/bin/node:" | sudo tee /proc/sys/fs/binfmt_misc/register

and the OS would take care of launching the files through Node.js; this, however, doesn't work currently as Cargo doesn't set the executable bits on the .js files it produces.

@alexcrichton
Copy link
Member

Cargo doesn't currently have any knowledge of targets, it just assumes that if you request cargo run that the build process created a binary that it can run.

@brson do you have thoughts about whether Cargo should special case here?

@alexcrichton
Copy link
Member

(I'm tempted to say that this error is expected and we should rely on users to run node manually)

@est31
Copy link
Member

est31 commented Feb 11, 2017

Just note it should use "nodejs" on debian like systems, to conform with http://lists.debian.org/debian-devel-announce/2012/07/msg00002.html (TLDR: official package for node.js is called nodejs, and the command to launch node.js is called nodejs as well)

@koute
Copy link
Member Author

koute commented Feb 12, 2017

Actually, now that I think about it I'm more inclined to agree with Alex.

The problem space is a little more complex than just simply running Node.js; in fact I've wrote my own Cargo subcommand for this very use case. In the vast majority of cases when someone compiles something to asm.js/webasm he/she does that to run it in a web browser on the client-side (because otherwise why aren't you simply building a native binary?); running that in Node.js is not ideal since Node.js (obviously) doesn't support most Web APIs.

What you actually would want to do here is to either start an embedded web server and allow the user to access it with his/her web browser (cargo run equivalent), or run it in a headless web browser (cargo test equivalent) and dump the output in the terminal. This is both complex and niche enough that it feels vastly out of scope of what Cargo itself should be able to do.

So, yes, we probably shouldn't add a special case for running the build results through Node.js, but on the other hand a could not execute process error message is not very helpful. Maybe we could add a special case where Cargo won't try to run it and display a more helpful error message instead?

@alexcrichton
Copy link
Member

@koute yeah I'd be totally up for improving error messages here in any case. We could detect cross compiles and then detect an error to execute and then print out a message saying that this likely wasn't going to work in the first place (or something along those lines)

@RReverser
Copy link
Contributor

This could be really useful for running tests though, as output names are not very predictable. How about, instead of specializing for Emscripten, provide generic argument like --run-with=... which would be used to execute output files when given? Then this would allow people to flexibly switch between Node.js / D8 / custom scripts to run tests in browser / etc., and at the same time would not be bound to Emscripten in any way.

@alexcrichton
Copy link
Member

@RReverser heh there's actually a surprising number of related proposals currently in flight as well. For example I think #3866 would help with that?

Some other references: #3670, #1763, #1924.

Right now the sort of "conventional" way to do this (albeit not an easy way) would be to:

  • Use a custom subcommand with --message-format=json
  • Read out tests that are built during cargo test
  • Use the JSON blobs to learn about paths to tests and run them.

@RReverser
Copy link
Contributor

Right now the sort of "conventional" way to do this (albeit not an easy way) would be to

Sure, but such conventional way can't be exactly called convenient :) Especially when just running commands yourself from console, without involving yet another build wrapper around cargo like Make :)

These proposals indeed sound relevant. Is there a plan to merge it yet? Looks like they're all postponed or just in discussion phase, would love to track in some centralized place.

@alexcrichton
Copy link
Member

Yeah there's currently not a huge driving force on these unfortunately. Re-reviewing though some of it's been lack of motivation and/or not the strongest of use cases. Looking back I'm sort of tempted to just add --with foo which acts like:

  • Accepted by cargo run and cargo test
  • All executed binaries are executed with --with foo instead, e.g. instead of target/debug/bar it's foo target/debug/bar.
  • Binaries are already executed serially, so no need to worry about concurrency there (it's up to the binary to be internally parallelized).
  • Binaries can share stdin as they're executed serially.

That would enable use cases like:

# debug a test
cargo test --with gdb --test foo

# run all tests on node
cargo test --with node --target asmjs-unknown-emscripten

# run a binary with strace
cargo run --with strace

# Print the arguments/env vars without actually running anything
cargo run --with my-custom-tool

@RReverser would you be up for spearheading discussion here and helping to drive this forward?

@RReverser
Copy link
Contributor

RReverser commented Apr 24, 2017

Sounds great! Few questions:

  1. Accepted by cargo run and cargo test

    And cargo bench, right? :)

  2. Will --with accept just binary name or some host tool arguments too? (e.g. to run Node.js with debug or tracing flags)

  3. Will be this configurable through cargo config? So that, again, using Emscripten as example, we could permanently set Node.js to be used to run any binaries built for asmjs-unknown-emscripten.

  4. would you be up for spearheading discussion here

    I'd need to know - what exactly is required apart from our existing ongoing discussion here? 😄 But yes, I'll be happy to help in the ways I can.

@alexcrichton
Copy link
Member

Oh right yeah this'd be accepted anywhere it made sense. For arguments we could add something like --with-arg and --with-args (or the like), and yeah seems reasonable to have global configuration for it.

For discussion basically just leading the charge on determining questions like you brough up, working to reach a consensus on what the answer should be, and then (optionally) implementing and/or just making sure it's written down for the next person!

@RReverser
Copy link
Contributor

Yeah, I'm happy to help with this - can't guarantee spending much time, but answering questions and implementing should be pretty straightforward.

@alexcrichton
Copy link
Member

ok, I'll cc some others who may be interested -- @matklad @brson @wycats @carols10cents

@matklad
Copy link
Member

matklad commented Apr 25, 2017

Looks like the end effect would be almost the same as in #3887, but for the resulting binaries instead of compiler. I wonder if these two use cases should share similar interface.

For example, with --with-args we can pass arguments to the wrapper process. @luser for the sscache use case, can the need arise to pass some command line parameters to the sscache itself? Something like sscache --foo --bar rustc --crate-type lib ....

@luser
Copy link
Contributor

luser commented Apr 25, 2017

For example, with --with-args we can pass arguments to the wrapper process. @luser for the sscache use case, can the need arise to pass some command line parameters to the sscache itself? Something like sscache --foo --bar rustc --crate-type lib ....

That's not something we've needed yet since sccache takes all of its configuration from environment variables currently, but I could see it being useful in the future. I am planning on implementing config file support for sccache, so it might be nice to be able to pass --config /path/to/config when that happens.

Related to the issue at hand: for Firefox test harnesses we added --debugger and --debugger-args commandline options a while ago for a similar use case. We have a mozdebug Python module that handles some common bits. One thing we did was encode common arguments necessary to run debuggers so that you can just pass --debugger=gdb and have things work properly even though gdb requires you to run gdb --args binary args if you're passing arguments to the binary. We also note whether the debugger is interactive--that is, whether it needs stdin hooked up to the terminal--since we normally pipe the output from our test harnesses. I don't know if cargo pipes the output of cargo run or cargo test by default, but if so that's something to consider.

@alexcrichton
Copy link
Member

@matklad yeah it's definitely similar, but I'd be wary of creating an abstraction that may not be too useful in the end, do we have a use case for wrapping literally everything Cargo spawns (rustc and binaries alike)? (is that IntelliJ's use case?)

@matklad
Copy link
Member

matklad commented Apr 25, 2017

is that IntelliJ's use case?

No definitely not.

yeah it's definitely similar, but I'd be wary of creating an abstraction that may not be too useful in the end,

Yeah, I'm saying that we should keep the other use case in mind and, for example, use similar named configuration option name.

@jan-hudec
Copy link

Duplicate of #1411. Which asks for .cargo/config option rather than command-line one, but for asmjs that is more appropriate anyway.

@RReverser
Copy link
Contributor

RReverser commented Apr 25, 2017

@est31 If you're referring to the current discussion, this doesn't matter at all, as Cargo won't be searching for Node.js executable, just using what user explicitly configured to run this target with. For some users, node will make sense, for others nodejs, for others d8 etc. etc.

Also, when you install emsdk, it provides its own node among other tools, so that should be easily found whenever you have Emscripten environment configured for build itself.

@RReverser
Copy link
Contributor

RReverser commented Apr 25, 2017

@jan-hudec I like that proposal, .cargo/config would be a viable minimum for now. Working on a PR right now...

@RReverser
Copy link
Contributor

@jan-hudec Made an initial PR in #3954 (the only difference with your proposal is that I chose runner instead of run to align better with linker).

RReverser added a commit to RReverser/cargo that referenced this issue May 12, 2017
When `target.$triple.runner` is specified, it will be used for any execution commands by cargo including `cargo run`, `cargo test` and `cargo bench`. The original file is passed to the runner executable as a first argument.

This allows to run tests when cross-comping Rust projects.

This is not a complete solution, and might be extended in future for better ergonomics to support passing extra arguments to the runner itself or overriding runner from command line, but it should already unlock major existing usecases.

Fixes rust-lang#1411
Resolves rust-lang#3626
bors added a commit that referenced this issue May 12, 2017
Add support for custom target-specific runners

When `target.$triple.runner` is specified, it will be used for any execution commands by cargo including `cargo run`, `cargo test` and `cargo bench`. The original file is passed to the runner executable as a first argument.

This allows to run tests when cross-comping Rust projects.

This is not a complete solution and might be extended in future for better ergonomics to support passing extra arguments to the runner itself or overriding runner from the command line, but it should already unlock major existing use cases.

Fixes #1411
Resolves #3626
bors added a commit that referenced this issue May 13, 2017
Add support for custom target-specific runners

When `target.$triple.runner` is specified, it will be used for any execution commands by cargo including `cargo run`, `cargo test` and `cargo bench`. The original file is passed to the runner executable as a first argument.

This allows to run tests when cross-comping Rust projects.

This is not a complete solution and might be extended in future for better ergonomics to support passing extra arguments to the runner itself or overriding runner from the command line, but it should already unlock major existing use cases.

Fixes #1411
Resolves #3626
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants