Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add compilation to NodeJS module #111
Add compilation to NodeJS module #111
Changes from 38 commits
5ac67bd
88a888d
9b23e19
4f6debc
d260472
b7b28ef
b6baf6c
a934ec2
94445ba
c51d50b
c0a82ce
8c7d18a
007b79e
1837643
7f3d0ce
9a8003a
2551769
90423ff
677aedc
8a22587
93d2f3b
6e6fe20
f59b47d
64fefe7
6d16790
14991f4
ce8bf3d
563864d
010a2ed
140545e
881b883
f57ab96
cece20d
87dd77b
d88ddb2
3072c03
63da721
39e5047
f36d62c
cdebf69
7740626
fcbe00f
4918507
0a82add
a772ff1
32d4b23
8990b2a
05925f2
892c9b5
4e02465
229c0d4
4c0c74f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 should check out a specific tag here. Otherwise we build against a different version every time.
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 are using the version defined in
EMSCRIPTEN_VERSION
env which is set now to the revfd61bacaf40131f74987e649a135f1dd559aff60
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.
Are those all the files that are needed? What about the hand written javascript files in
/js
? Or are you planning to do the full npm package release work flow as follow up?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 will add npm package with its workflow in next PR. Now we need these 2 files only.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
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.
done
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.
Should clone into
$LLVM_SRC_PREFIX
instead of a hardcoded path (if it goes into a dedicated script it should be an argument instead)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.
done
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.
Why is this changed? Before the target build directory was outside the
llvm-project
source directoryThere 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 have aligned it to this what i have in emscripten build:
LLVM_NATIVE=$LLVM_SRC/build-native
LLVM_WASM=$LLVM_SRC/build-wasm
We can reorganise it if you want
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 please I think it is a standard practice to keep the build outside the source, i.e. leave it as it is now.
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.
done