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

allow linkage to .js files #4816

Closed
wants to merge 5 commits into from
Closed

allow linkage to .js files #4816

wants to merge 5 commits into from

Conversation

chpio
Copy link

@chpio chpio commented Dec 22, 2016

allow linkage to .js files
rust-lang/rust#38492

@@ -1866,36 +1866,36 @@ def test_life(self):
src = open(path_from_root('tests', 'life.c'), 'r').read()
self.do_run(src, '''--------------------------------
[] [] [][][]
[] [] [] [][] [] [] []
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this whitespace actually matters, I'm afraid - the test expects it the way it was before your change

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, i will redo the changes with ws trimming disabled

@kripken
Copy link
Member

kripken commented Dec 22, 2016

On the one hand, this has some risk of confusing people (that's why we have --js-library as a special flag), but on the other, it does make sense since we can't accept JS files as inputs otherwise. So I am leaning towards liking this :) But I'd like to hear other opinions on it, in particular from @juj.

As for the PR details, this looks quite good. The one thing missing is to document it under site/ (in the emcc documentation, and perhaps also in the other places where --js-library is mentioned).

@chpio
Copy link
Author

chpio commented Dec 22, 2016

just noticed that the master branch is really outdated (but the contrib page states it's the stable version = latest release). should i rebase it onto incoming?

edit: i added an "editorconfig" (http://editorconfig.org/) i hope you don't mind that?

@kripken
Copy link
Member

kripken commented Dec 22, 2016

Yes, that would be best, thanks.

@chpio
Copy link
Author

chpio commented Dec 27, 2016

There's no documentation for the -l and -L args.

@kripken
Copy link
Member

kripken commented Jan 3, 2017

Yeah, I guess -I, -L are just documented in clang's help. But we do document --js-library, so perhaps the locations where we do that are good places to document this change.

(But, still waiting to hear feedback from @juj on this. Maybe there is a downside I forgot.)

@chpio
Copy link
Author

chpio commented Jan 5, 2017

Are ther any plans for handling JS-Library files properly? I Just noticed they aren't handled right now, which makes this PR useless for me.

"properly handled" as in:

// local functions
function printFoo(foo) {
  console.log("foo:", foo);
}

// exported functions
LibraryManager.library.my_lib_bar = function(foo) {
  printFoo(foo);
};

printFoo never ends up in the generated file.

Maybe there is a workaround for this? i tried:

// local functions
LibraryManager.library.my_lib_local_fns = function() {
  function printFoo(foo) {
    console.log("foo:", foo);
  }
  return {printFoo: printFoo};
}

// exported functions
LibraryManager.library.my_lib_bar = function(foo) {
  var printFoo = my_lib_local_fns().printFoo;
  printFoo(foo);
};

but no luck

.editorconfig Outdated
root = true

[*]
end_of_line = lf
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use operating system native end of line markers, i.e. files are not LF on Windows. Does this confuse line endings on Windows on editors that acknowledge this file?

Copy link
Author

@chpio chpio Jan 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You check the native eol into git? That is files written by Windows are crlf and unix lf and are put into the git db without normalisation?

core.autocrlf = false This is the default. The result of using false is that Git doesn’t ever mess with line endings on your file. You can check in files with LF or CRLF or CR or some random mix of those three and Git does not care.
http://adaptivepatchwork.com/2012/03/01/mind-the-end-of-your-line/

will delete that line on the next rebase

@juj
Copy link
Collaborator

juj commented Jan 5, 2017

The concern with this has been that compressing JS libraries under same filename space than general library files has a danger of picking up unintended files in certain build systems. After this we would have a different behaviour compared to native that build output executable files will then be potential library link input files, a behaviour which native build systems don't have (compare .exe on Windows and no suffix on Linux/OS X, those never have the danger of getting picked up with -l).

There has not been a pressing need to namespace these under the same umbrella so far because our .js library files are never results of static library/archive outputs, so library .js files are strictly input files rather than ever intermediate build files that build systems would manage the creation of and subsequent linking of. That is, it's never the build system that generates linking to these automatically, but developers are who specify them as source files to the build. At that point, whether a developer specifies the string "--js-library path/to/jslibrary/foo.js" vs "-L path/to/jslibrary -lfoo" has not had a big difference, and it has been clear a bit safer to do it this way.

I'm curious, what is the rationale for this on Rust side?

@kripken
Copy link
Member

kripken commented Jan 5, 2017

Regarding the "properly" issue: we don't automatically pull in code that is captured via a closure. Doing so would require static analysis that probably can never be complete in a dynamic language like JS. Instead JS library linking just pulls in symbols as they are needed, and has explicit dependencies (__deps etc.) - you can add utility functions that way, see examples in the system JS libs. Often you just create a utility object like e.g. FS or SDL for all the helper stuff, and add a dep on it.

@chpio
Copy link
Author

chpio commented Jan 5, 2017

Doing so would require static analysis that probably can never be complete in a dynamic language like JS.

ok, rollup.js is doing that quite nicely.

you can add utility functions that way, see examples in the system JS libs. Often you just create a utility object like e.g. FS or SDL for all the helper stuff, and add a dep on it.

damn it, i hoped to be able to build the js file with rollup, but that isn't going to work. But it's at least working in some way :)

edit: maybe there is a chance of writing a rollup plugin, which would use the rollup dependency tree to automatically generate the __deps entries.

The concern with this has been that compressing JS libraries under same filename space than general library files has a danger of picking up unintended files in certain build systems. After this we would have a different behaviour compared to native that build output executable files will then be potential library link input files, a behaviour which native build systems don't have (compare .exe on Windows and no suffix on Linux/OS X, those never have the danger of getting picked up with -l).

So, we should use another file extension to distinguish between js-executables and js-libraries? eg ".jslib"?

edit: rather: ".emlib.js"

At that point, whether a developer specifies the string "--js-library path/to/jslibrary/foo.js" vs "-L path/to/jslibrary -lfoo" has not had a big difference, and it has been clear a bit safer to do it this way.

I'm curious, what is the rationale for this on Rust side?

Yeah, i have the issue that an cargo/rust-library isn't allowed to set custom linker arguments (eg --js-library ;D), just the standard -l and -L args. And im about to write a web rust+js library which then should be usable from an executable rust-crate. That's why i thought it would be a nice feature to have in emscripten and (imho) it would fit the semantics of the -l & -L args.

Hmm, but that patch could also be applied to rust. So rustc would take the -L & -l args, do the resolution of js-libs as intended by my pr and then call emcc with the --js-library arg.

@chpio
Copy link
Author

chpio commented May 16, 2017

i closed this in favor of rust-lang/rust#41409 , but then again: rust-lang/rust#41409 (comment)

@chpio chpio reopened this May 16, 2017
@RReverser
Copy link
Collaborator

@chpio Ha, I was going to do exactly the opposite :) It seems to be much easier to fix this on Emscripten side without bloating Rust codebase with Emscripten-specific JS library linkage code.

Are you willing to continue this PR? Do you need any help?

@chpio
Copy link
Author

chpio commented May 19, 2017

what would you change on this one?

@RReverser
Copy link
Collaborator

RReverser commented May 19, 2017

I don't know - question is to @kripken and @juj. Looks great to me.

@kripken
Copy link
Member

kripken commented May 19, 2017

Looks ok to me too, I think what is left is to understand @juj's concern of

a danger of picking up unintended files in certain build systems

@juj, perhaps you can elaborate on the risk here?

We may need to add some more checks or warnings here to reduce the risk, but first need to understand it fully.

@juj
Copy link
Collaborator

juj commented May 30, 2017

Reading back my earlier comment, it looks like using a special suffix .emlib.js like is done here would avoid the namespace collision issue, so that concern is probably not valid in the structure of the proposed PR.

However I still think this is a bit dangerous. There is not much more standard behavior about this even if -l foo.js is used instead of --js-library foo.js. I think making .js linking and LLVM linking look more like the same on the command line should be done only if we strive to develop this to behave the same as well. There are notable differences that currently exist. Referencing the documentation of -l directive at https://gcc.gnu.org/onlinedocs/gcc-4.7.0/gcc/Link-Options.html:

  • "It makes a difference where in the command you write this option; the linker searches and processes libraries and object files in the order they are specified.": this does not currently apply to linking .js libraries, but these are always weakly linked to as the very last step. Things such as command line link groups (--start-group, --end-group) do not apply to .js libraries.
  • GCC documents that it's looking up liblibrary.a on -llibrary, but this would be looking for library.emlib.js. Is this by design? Although iirc GCC is more flexible here than what those pages currently document and -llibrary also looks up library.a and library.so, so what would the exact rules be here? Should the .emlib.js files be the weakest if there are multiple matching files in a directory, i.e. if both liblibrary.a and library.emlib.js exist in -L directories, always pick up .a?

Is the root problem for having this kind of feature that --js-library directives need to currently find the library by absolute path name? In that case, would it make sense to resolve the issue by allowing specifying relative files in --js-library and searching those from the set of specified -L directories as well? I feel that given that "-l linking" and .js linking currently behave differently, making these the overlap in the same command line flag is a fallacious feature.

If a mechanism was developed for .js files to have standard dependencies and exports such at they could partake to -l linking like normal libs, such that the ordering and --start-group and --end-group and other behavior works as one would expect, then it would make more sense to treat these in this type of more unified manner.

@RReverser
Copy link
Collaborator

RReverser commented May 30, 2017

but this would be looking for library.emlib.js. Is this by design?

Emscripten already supports other suffixes different from .a (with custom handling), so I don't see why not.

Is the root problem for having this kind of feature that --js-library directives need to currently find the library by absolute path name? In that case, would it make sense to resolve the issue by allowing specifying relative files in --js-library and searching those from the set of specified -L directories as well?

That's only part of the problem. The other is that top-level tools that invoke the linker - whether it's Rust compiler, CMake or something else - know about -L and -l and can already pass them to any downstream linker that supports these options, while --js-library is a flag very specific to Emscripten and teaching tools about it brings more overhead than letting Emscripten accept JS libraries via -l as well.

I totally agree that -L should be working for JS libraries too, but not so sure that support for --{start,end}-group is critical in this regard given how JS libraries are linked outside of the asm.js/wasm itself, at least for initial stage.

@RReverser
Copy link
Collaborator

Any conclusion here? What does it take to move this PR forward? I'd like to help if it's needed, but would love to know if this PR will be accepted at all first. Even in the current form it would be already very helpful for the Rust compiler.

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 this pull request may close these issues.

4 participants