-
Notifications
You must be signed in to change notification settings - Fork 120
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
Fix C source files compilation #57
Conversation
@@ -19,7 +19,7 @@ | |||
"cbuild": "cbuild", | |||
"dump-lib": "dump-em-lib dist/em-api.js", | |||
"lint": "tslint -c src/tslint.json src/*.ts src/em/*.ts", | |||
"prepublish": "npm run lint && tsc -p src/em && tsc -p src && cbuild -x -v -s dist/bundle/em/em-api.js -o dist/em-api.js", | |||
"prepublish": "tsc -p src/em && tsc -p src && cbuild -x -v -s dist/bundle/em/em-api.js -o dist/em-api.js && npm run lint", |
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 change was made to allow compiling the project even if lint eventually fails?
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.
Yes, so that I would be able to use arcanis/nbind
until a new version is pushed. Unfortunately, it doesn't work anyway, since npm is removing .ts
files even when installing from a repository (it apparently follows the .npmignore
rules even in this case) ... do you want me to reverse this 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.
I guess reversing would be best, so it checks style before typing issues.
Installing from a repo also doesn't install devDependencies so the only way to get that working is to put compiled .js
files in the repo. I'll do a release tomorrow to avoid problems.
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.
Thanks 👍
I've force-pushed and removed the commit from the PR.
Thank you for the PR! |
Summary: - As mentioned in the title, this PR adds Javascript support to Yoga. Two different builds are included in this PR thanks to [nbind](https://github.com/charto/nbind), which conveniently allow to target both Node.js' native addons and browser environments via asmjs with approximately the same codebase. That should solve #215. - All tests successfully pass on both codepaths. You can run `yarn test:all` inside the `javascript` directory to test it. - Because of a bug in nbind, the [following PR](charto/nbind#57) needs to be merged and a new version released before this one can be safely merged as well. - I had to use `double` types instead of `float` in the C++ bindings because of an Emscripten [bug](emscripten-core/emscripten#3592) where symbols aren't correctly exported when using floats. - There's some tweaks to do before this PR is 100% ready to merge, but I wanted to have your opinion first. What do you think of this? --- To do: - [x] Ensure th Closes #304 Reviewed By: mikearmstrong001 Differential Revision: D4375187 Pulled By: emilsjolander fbshipit-source-id: 47248558a9506b7c512b5ef281cd12fe1a60cab7
The current flags break C compilation when building with
--asmjs
, because emscripten doesn't understand why C++-specific flags are used with the C compiler. This commit fix this by splitted the flags ascflags
andcflags_cc
.