Allow linking JS libraries to Emscripten target

[RReverser]

This allows crates to inject own JS functions they want to link to in Rust code, without using link_args hack which is not possible to populate from build script with computed path.

"Framework" seems to be most suitable section for such JS code.

See https://kripken.github.io/emscripten-site/docs/porting/connecting_cpp_and_javascript/Interacting-with-code.html for more details on --js-library feature.

This is a follow up to #39490.

Fixes #38492.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @pnkfelix (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[RReverser]

r? @alexcrichton

[alexcrichton]

Awesome thanks for the PR @RReverser! This is an interesting question actually to me. The intention of framework was that it corresponded to OSX frameworks, where at least within the context of OSX it has a specific meaning (as opposed to library).

So more generally, all linkage directives have a particular kind associated with them. This is dylib by default but can also be static or framework. Out of curiosity, do any kind directives work with Emscripten today? Or do they all generate errors?

If we need to add support for JS libs specially I'd be tempted to add kind = "js" just to be clear about what's going on and avoid overloading framework, a currently OSX-specific term. How's that sound to you?

[RReverser]

Idk, personally to me it seems more reasonable to reuse the existing kind rather than create a new one specific for each target.

From the code at least, it seemed that the framework being used only by OSX is rather an implementation detail than intentional (because only OSX among the existing targets had special frameworks), and could be used as such for Emscripten libraries too, which are also specially crafted items that can be linked and used as a normal C API.

[alexcrichton]

Yeah it's true, and I wouldn't mind much about using framework in this regard really. Do you know whether kind = "static" or kind = "dylib" work for Emscripten today?

[RReverser]

@alexcrichton static and dylib currently result in the same behaviour, but yeah, they work in the sense that they allow to link other lib*.a previously built by Emscripten. Adding proper dynamic linkage through corresponding Emscripten functionality is on my TODO list, but I'm afraid it's outside of scope of this PR :)

[alexcrichton]

Ok let's see if others from @rust-lang/tools have an opinion.

Any thoughts on using the framework library kind for JS libs? Currently that's reserved for OSX frameworks but I think @RReverser makes some good points. I'm not sure there's any particular reason to indefinitely require that it's just OSX frameworks, giving it a platform-specific meaning seems reasonable to me.

@RReverser so this shows a bug on -L framework=foo, is that intentional? Does emscripten need to know about lookup search paths for --js-library arguments?

[michaelwoerister]

Any thoughts on using the framework library kind for JS libs?

I'm fine with that.

[RReverser]

@alexcrichton It's intentional, as Emscripten doesn't perform search in -L-provided paths or whatever and instead accepts only relative/absolute paths in --js-library.

We could, of course, implement such search ourselves, but I'm not sure it's worth it and seems easier just to let crates provide absolute paths on their own.

[alexcrichton]

Ok cool makes sense to me, mind changing framework_path to provide a nicer error message in that case? (right now bug! == ICE I think)

[RReverser]

Ah sure. Should I just use panic! or something specific?

[alexcrichton]

Ideally something like sess.err(..) would be used, panics also tend to translate to ICEs.

It may be a case where when constructing the linker object the Session will just need to be stored in one of the struct fields.

[RReverser]

Sorry, but what is ICE?

[alexcrichton]

Oh apologies! ICE == "Internal compiler error", you can explore the issue tracker for examples.

[RReverser]

Ah ok.

[RReverser]

Ok I've decided to use self.sess.warn as this is not a critical error, but rather just a redundant argument (and if the actual framework library can't be found, compilation will fail anyway).

[alexcrichton]

Hm does this patch work locally for your? I'd expect these lines to cause problems.

[RReverser]

Argh, you're probably right and I should've waited for the actual build rather than sending modifications from Github. Let me recheck everything.

[RReverser]

Ok fixed and actually checked that it works now.

For example:

• hello.rs:

#[link(name = "hello-lib.js", kind = "framework")]
extern {
  fn hello();
}

fn main() {
  unsafe { hello() }
}

• hello-lib.js:

mergeInto(LibraryManager.library, {
  hello: function() { console.log('hello world') }
});

To compile:

$ ./build/x86_64-apple-darwin/stage2/bin/rustc --target=asmjs-unknown-emscripten hello.rs
$ node hello
hello world

[RReverser]

Btw, regarding checks: is it just me or changing a single line in the linker really requires rebuilding entire Rust from scratch?

I've tried all the suggested and possible configs to avoid rebuilding stdlib, keeping stage 0, make builds incremental etc., but it either didn't compile with them or just didn't have any effect.

For example, the last rebuild for the fix above took 2 hours on my machine.

[RReverser]

Ok everything seems to work / pass now. @alexcrichton

[RReverser]

Also retested with putting JS dependency in library crate, and using it as dependency in another one - something that was not possible before even with #[link_args = "--js-library ..."] - and it works now 🙌 So we can build library crates with their own JS code.

[alexcrichton]

Looks good to me, thanks @RReverser!

Btw, regarding checks: is it just me or changing a single line in the linker really requires rebuilding entire Rust from scratch?

Unfortunately due to the way Rust bootstraps itself any change will unconditionally require a compilation of the entire compiler at least once (typically ~1.5x depending on where the change was located). You can typically work around with with manual cleaning and manual "touch this file back into th 1990s" but it gets hazardous. I'll give a few more days for @rust-lang/tools to weigh in, but I think we're likely to merge this.

[RReverser]

@Diggsey Adding completely new kind with such interaction would require an overhaul of a quite big part of the compiler, while suggested solution piggybacks on an existing method already used for platform-specific functionality.

Also unclear - what would be the point to specify platform:js on library that can only be compiled & used on Emscripten or platform:framework on library that, apparently, can work only in combination with MacOS framework?

[Diggsey]

@RReverser It would have no purpose on other platforms. However, other platforms may have more than one platform-specific link kind. Rather than having the compiler be aware of each platform-specific link kind, it would make more sense to teach it to recognise any platform-specific link kind, and to pass it through.

[RReverser]

Yeah but it still needs to know how to convert link paths to the actual linker, linkers don't just accept some generic JSON or raw link(...) metadata, it has to be normalized on Rust side. Otherwise this sounds like just link_args which already exists but doesn't work for this use case.

[brson]

Another point to consider here is that, whatever the solution is, it is not appropriate for it to be insta-stable. Even if we were to re-purpose "framework", which I think we should not, it should still be feature-gated.

Perhaps "js" is not forward-compatible enough. I don't know. Adding a general platform-specific linkage mechanism as suggested by @Diggsey starts to sound RFC-worthy.

Another approach would be to add a new general-purpase linker args mechanism, if link_args is insufficient, but again that sounds like the kind of thing that requires broader feedback.

[brson]

@RReverser My preference is still to add a "js" linkage type and put it behind a feature gate. As a minor gated feature, we can try to get it in without going to a full RFC (though it is bending the rules, I think it's an obscure, and small, enough experimental thing that it doesn't matter all that much), and reevaluate it before stabilization.

Will you make that change?

[RReverser]

@brson

Another point to consider here is that, whatever the solution is, it is not appropriate for it to be insta-stable.

Indeed, in any case I just wanted to have some feature available in nightly that users could play with - not an "insta-stable" one.

Even if we were to re-purpose "framework", which I think we should not, it should still be feature-gated.

Oh yeah, I could totally just feature-gate the existing change if that was proposed earlier :)

Perhaps "js" is not forward-compatible enough. I don't know.
My preference is still to add a "js" linkage type and put it behind a feature gate.

js really doesn't sound future-proof to me either as it can and likely will have completely different format on future targets (like mir2wasm), while this one is purely Emscripten specific format. So if this is still preferred over reusing a single kind for any platform-specific APIs, I'd probably call it something like emlibrary or emjs or whatever to reduce risk of it clashing with future additions?

Other than that, yeah, by now I'm happy with whichever outcome, although don't know how much work it takes to add a new linkage kind on all levels - let me try.

[RReverser]

Should I reuse E0455 but change the extended diagnostic message to "Linking with this kind is supported only on specific target" and leaving the MacOS one as an example, or should I allocate a new error message for this? If so, how do I find out which one is free?

[RReverser]

@brson @alexcrichton Updated a PR with the suggested approach (except the question above remains, for now it just uses same error code, didn't even change the error message - which way's preferred?).

[alexcrichton]

Thanks @RReverser! This looks great to me. I think reusing the error code in this case is ok, but you can update the associated text if you'd like as well. Looks like tidy is failing I believe though?

[carols10cents]

Friendly ping to keep this on your radar @RReverser! Looks like this tidy failure still needs to be fixed? Let us know if you need help or have any questions! ❤️

[00:03:09] Building stage0 tool tidy (x86_64-unknown-linux-gnu)
[00:03:09]    Compiling tidy v0.1.0 (file:///checkout/src/tools/tidy)
[00:03:17]     Finished release [optimized] target(s) in 7.70 secs
[00:03:17] tidy check (x86_64-unknown-linux-gnu)
[00:03:17] * 473 error codes
[00:03:17] * highest error code: E0592
[00:03:18] tidy error: Found 1 features without a gate test.
[00:03:18] Expected a gate test for the feature 'link_js'.
[00:03:18] Hint: create a file named 'feature-gate-link_js.rs' in the compile-fail
[00:03:18]       test suite, with its failures due to missing usage of
[00:03:18]       #![feature(link_js)].
[00:03:18] Hint: If you already have such a test and don't want to rename it,
[00:03:18]       you can also add a // gate-test-link_js line to the test file.
[00:03:18] tidy error: Unstable language feature 'link_js' needs to have a section within the 'language features' section of The Unstable Book
[00:03:18] some tidy checks failed
[00:03:18] 
[00:03:18] 
[00:03:18] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools/x86_64-unknown-linux-gnu/release/tidy" "/checkout/src" "--no-vendor"
[00:03:18] expected success, got: exit code: 1
[00:03:18] 
[00:03:18] 
[00:03:18] Build completed unsuccessfully in 0:01:01
[00:03:18] make: *** [tidy] Error 1
[00:03:18] Makefile:74: recipe for target 'tidy' failed

[RReverser]

Yeah, thanks!

But apart from that, there is one more thing I don't like about current implementation and will still look into fixing - in particular, I want to pack .js statically into .rlib rather than just passing the path up to the final linkage.

[vvanders]

Just going to jump in here quickly and say this is sorely needed for any Emscripten target.

I have a production app that we're looking at targeting with Emscripten + Rust, however the only thing that works currently is link_flags which is behind a feature gate in nightly. We can't use the nightly compiler since we want consistent builds. Anything more than the most trivial Emscripten usage is going to want to use the C API for deeper interop.

[Mark-Simulacrum]

@RReverser Looks like Travis is failing; did you have a chance to look into the static packing of .js files?

[RReverser]

We can't use the nightly compiler

I'm afraid this feature won't help you either then, as it will be also behind nightly feature gate (at least for a while).

[japaric]

@vvanders You can probably use RUSTFLAGS to pass custom arguments to the linker:

$ RUSTFLAGS="-C link-arg=--js-library -C link-arg=whatever" cargo build --target wasm32-unknown-emscripten

Or for something more permanent you can put the rustflags in a project local .cargo/config file:

# .cargo/config
[target.wasm32-unknown-emscripten]
rustflags = [
    "-C", "link-arg=--js-library",
    "-C", "link-arg=whatever",
]

This should work on stable.

[RReverser]

Just a heads up: I'm now trying instead to bring some changes upstream in Emscripten itself which should make this PR either much easier or not needed at all.

[vvanders]

@japaric That did the trick, thanks! Hopefully it'll help anyone else who stumbles across this.

[alexcrichton]

@RReverser is there a place to link to for the emscripten changes as well?

[RReverser]

Still waiting for feedback on the issue (emscripten-core/emscripten#5221) plus there were apparently two previous PRs that tried to implement that. All of them are linked above by Github.

I guess let me just close this for now to avoid confusion and reopen later with proper implementation (with static linking) either on Rust side or on Emscripten one.

[tigercosmos]

Are there any new commits base on this?

[RReverser]

@tigercosmos See the linked issue above. Unfortunately, it's very slow to get any contributions into Emscripten, so I put more hope into the new native Rust support for WASM (although that one is still in flux too).

[ramith]

@RReverser any ideas on when this feature will be available?