-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Conversation
tests/test_core.py
Outdated
@@ -1866,36 +1866,36 @@ def test_life(self): | |||
src = open(path_from_root('tests', 'life.c'), 'r').read() | |||
self.do_run(src, '''-------------------------------- | |||
[] [] [][][] | |||
[] [] [] [][] [] [] [] |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
On the one hand, this has some risk of confusing people (that's why we have As for the PR details, this looks quite good. The one thing missing is to document it under |
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? |
Yes, that would be best, thanks. |
There's no documentation for the -l and -L args. |
Yeah, I guess (But, still waiting to hear feedback from @juj on this. Maybe there is a downside I forgot.) |
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
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? |
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 ( |
ok, rollup.js is doing that quite nicely.
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.
So, we should use another file extension to distinguish between js-executables and js-libraries? eg ".jslib"? edit: rather: ".emlib.js"
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. |
i closed this in favor of rust-lang/rust#41409 , but then again: rust-lang/rust#41409 (comment) |
@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? |
what would you change on this one?
|
Looks ok to me too, I think what is left is to understand @juj's concern of
@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. |
Reading back my earlier comment, it looks like using a special suffix However I still think this is a bit dangerous. There is not much more standard behavior about this even if
Is the root problem for having this kind of feature that If a mechanism was developed for |
Emscripten already supports other suffixes different from
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 I totally agree that |
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. |
allow linkage to .js files
rust-lang/rust#38492