-
Notifications
You must be signed in to change notification settings - Fork 34
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
Support Rust files in wat2wasm4cpp.py script #704
Conversation
14f80bc
to
e547964
Compare
Codecov Report
@@ Coverage Diff @@
## master #704 +/- ##
=======================================
Coverage 99.27% 99.27%
=======================================
Files 73 73
Lines 10883 10883
=======================================
Hits 10804 10804
Misses 79 79
Flags with carried forward coverage won't be shown. Click here to find out more. |
Yes that is a bit annoying, but still better to have this automated. |
./wat2wasm4tests.py test/unittests/validation_test.cpp | ||
./wat2wasm4tests.py test/unittests/wasm_engine_test.cpp | ||
./wat2wasm4tests.py test/spectests/spectests.cpp | ||
./wat2wasm4tests.py test/testfloat/testfloat.cpp | ||
git diff --color --exit-code |
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 also add bindings/rust/src/lib.rs
to the CI.
@@ -31,10 +31,16 @@ | |||
WAT2WASM_TOOL = 'wat2wasm' | |||
WAT2WASM_DEFAULT_OPTIONS = ['--disable-saturating-float-to-int', | |||
'--disable-sign-extension', '--disable-multi-value'] | |||
FORMAT_TOOL = 'clang-format' | |||
CPP_FORMAT_TOOL = ['clang-format', '-i'] | |||
RS_FORMAT_TOOL = ['rustfmt'] |
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.
Did you try this on ci? Doesn't it need to be installed on the run?
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.
Actually it would ignore failures if rustfmt were not found, let me check it then.
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 were right, it didn't work. I fixed it by moving into bindings-rust
job, where it's already installed (but then wabt needs to be installed there, too)
30711b7
to
d7530f0
Compare
dbe4034
to
0f4dc74
Compare
@@ -35,7 +35,7 @@ commands: | |||
else | |||
[[ $OSTYPE = darwin* ]] && os=macos || os=ubuntu | |||
cd /usr/local | |||
curl -L https://github.com/WebAssembly/wabt/releases/download/1.0.20/wabt-1.0.20-$os.tar.gz | sudo tar xz --strip 1 | |||
curl -L https://github.com/WebAssembly/wabt/releases/download/1.0.20/wabt-1.0.20-$os.tar.gz | (sudo tar xz --strip 1 || tar xz --strip 1) |
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 solves the problem of sudo
not working in Rust docker (user is already root there), probably there are more elegant ways to solve it, but this works.
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.
It might be better to migrate to cimg/rust where build are probably not executed as root and there may be more CI related benefits.
circle.yml
Outdated
name: Check wat2wasm4tests | ||
command: | | ||
rustfmt --version | ||
./wat2wasm4tests.py bindings/rust/src/lib.rs |
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 not use find
here too (like for C++)?
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.
That would be good to.
Closes #593.
I used
rustfmt
becaust it takes a single file as argument.However, it doesn't seem much useful, e.g. long hex strings are not broken down (this option is unstable and off by default
https://rust-lang.github.io/rustfmt/?version=v1.4.33&search=#format_strings
rust-lang/rustfmt#3353)