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.

Sign up for GitHub

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

Jump to bottom

Fix emscripten build warning and add missing export #2367

Merged
merged 3 commits into from
Jan 20, 2024
Merged

Fix emscripten build warning and add missing export #2367

merged 3 commits into from
Jan 20, 2024

Conversation

Changqing-JING
Copy link
Contributor

@Changqing-JING Changqing-JING commented Jan 17, 2024 •
edited
Loading

  1. Fix wabtjs bug that missing writeAsciiToMemory built by new version emcc
  2. Refine the code quality and CI of wabtjs
    2.1: Add warning as error in emcc build
    2.2 Port the test cases from https://github.com/AssemblyScript/wabt.js/tree/main/tests

@Changqing-JING Changqing-JING changed the title refine wabtjs code quality refine code quality of emcc build Jan 17, 2024
@Changqing-JING Changqing-JING force-pushed the fix/emcc_export branch 3 times, most recently from 7869694 to abe3632 Compare January 17, 2024 05:54
@Changqing-JING
Copy link
Contributor Author

@sbc100 Sorry for creating another PR in such short time regarding wabtjs.
I just found another bug which caused by updating new version emsdk.
Beside the bug itself, recent bugs leads me to reflect the code quality of the emcc build. I think there are at least two problems:

  1. Warning as error is not enabled in CI
  2. No test run on the libwabt.js, so that many bug can't be detected by CI.

I ported the testcases for Assemblyscript/wabt.js to here, then some errors in wabt.post.js should can be found by CI.

sbc100
sbc100 reviewed Jan 17, 2024
View reviewed changes
CMakeLists.txt Outdated Show resolved Hide resolved
sbc100
sbc100 reviewed Jan 17, 2024
View reviewed changes
Copy link
Member

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this.

I'm not sure about adding this new test suite, especially since its comes from another project. Perhaps you could move that part into a separate PR?

src/c-writer.cc Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
@Changqing-JING
Copy link
Contributor Author

@sbc100
Thank you for review, I fixed your comments and split the test files out of this PR

sbc100 added a commit that referenced this pull request Jan 18, 2024
@sbc100
[ci] Build i686-debug configuration
8b2757d 
There was a missing cast here that only showed up in i686-debug.
See #2367.
@sbc100 sbc100 mentioned this pull request Jan 18, 2024
Open
sbc100 added a commit that referenced this pull request Jan 18, 2024
@sbc100
[ci] Build i686-debug configuration
a945925 
There was a missing cast here that only showed up in i686-debug.
See #2367.
@Changqing-JING
Copy link
Contributor Author

@sbc100 Can this PR be merged? Anything else to fix?

sbc100
sbc100 reviewed Jan 19, 2024
View reviewed changes
.github/workflows/build.yml
docker exec emscripten emcmake cmake .
docker exec emscripten make -j 2 VERBOSE=1
docker exec emscripten emcmake cmake -B emscripten -DWERROR=ON -DBUILD_TESTS=OFF
docker exec -w /src/emscripten emscripten make -j $(nproc)
Copy link
Member

Choose a reason for hiding this comment

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

Why drop the VERBOSE=1 here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just VERBOSE level trace is not quite necessary in most cases, because build warning and error show by default. VERBOSE level trace some time makes warning and error message not quite visible inside a large mount of text.
But it's not a big issue, I can also add it back. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

In think as long as we have -DWERROR=ON when its probably find to keep verbose build output when building during CI

sbc100
sbc100 reviewed Jan 19, 2024
View reviewed changes
.github/workflows/build.yml Outdated Show resolved Hide resolved
sbc100
sbc100 reviewed Jan 19, 2024
View reviewed changes
wasm2c/wasm-rt-impl.c
@@ -168,6 +169,7 @@ static int os_mprotect(void* addr, size_t size) {
static void os_print_last_error(const char* msg) {
perror(msg);
}
#endif
Copy link
Member

Choose a reason for hiding this comment

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

@keithw @shravanrn WDYT about this change?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure, this should be fine assuming the existing tests pass.

@sbc100 sbc100 changed the title refine code quality of emcc build Fix emscripten build warning and add missing export Jan 20, 2024
@sbc100 sbc100 merged commit cb76e5c into WebAssembly:main Jan 20, 2024
16 checks passed
This was referenced Jan 22, 2024
Open
Closed
@Changqing-JING Changqing-JING deleted the fix/emcc_export branch January 23, 2024 03:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shravanrn shravanrn shravanrn left review comments

@sbc100 sbc100 sbc100 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Changqing-JING @sbc100 @shravanrn