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

Enable exception catching; Separate build targets for node and worker environments. #11

Merged
merged 21 commits into from
Nov 1, 2024
Merged

Enable exception catching; Separate build targets for node and worker environments. #11

Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
333dd2c
Enable exception catching with CMake "Debug" build type.
junhaoliao Oct 1, 2024
3b18e26
Separate targets for different environments such as `worker` and `node`.
junhaoliao Oct 5, 2024
9cdcb3b
Update target names in Taskfile.yml.
junhaoliao Oct 5, 2024
4bd6677
Update "exports" in package.json to include the new "node" target.
junhaoliao Oct 5, 2024
9000016
Specify "module" type in `package.json`.
junhaoliao Oct 5, 2024
990fd0c
Pass `-fwasm-exceptions` to also the compiler in addition to linker.
junhaoliao Oct 5, 2024
34ca552
Merge remote-tracking branch 'yscope/main' into enable-exception
junhaoliao Oct 5, 2024
27337c8
Remove leading slash in files[]."dist" in package.json .
junhaoliao Oct 5, 2024
b6b4dba
Revert commented out "Unix Makefiles" generator enforcement.
junhaoliao Oct 5, 2024
d6d7327
Add EOL to EOF in CMakeLists.txt .
junhaoliao Oct 5, 2024
3d2b3df
Merge branch 'main' into enable-exception
junhaoliao Oct 8, 2024
33565a8
Rename `CLP_FFI_JS_ENVIRONMENTS` -> `CLP_FFI_JS_SUPPORTED_ENVIRONMENT…
junhaoliao Oct 8, 2024
7607a9e
Refactor compile and link options generations; add missing `-flto` in…
junhaoliao Oct 8, 2024
48a9910
Add missing `-sENVIRONMENT=${env}` option into CLP_FFI_JS_LINK_OPTIONS.
junhaoliao Oct 8, 2024
c0f4d68
Update docs in CMakeLists.txt
junhaoliao Oct 8, 2024
3f9775b
Put list items on new lines - Apply suggestions from code review
junhaoliao Oct 31, 2024
d3caf42
Add period to log message - Apply suggestions from code review
junhaoliao Oct 31, 2024
ad8f9c4
Move compile and link options log after adding the options.
junhaoliao Oct 31, 2024
6df29f3
Remove `G_CLP_FFI_JS_TARGET_NAMES` and make `G_CLP_FFI_JS_ENV_NAMES` …
junhaoliao Oct 31, 2024
3d7a40f
Remove extra hyphen in `package` command.
junhaoliao Oct 31, 2024
340cca5
Add extra line - Apply suggestions from code review
junhaoliao Oct 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
122 changes: 72 additions & 50 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,59 +58,28 @@ message(STATUS "Fetching Boost.")
FetchContent_MakeAvailable(Boost)
message("Boost sources successfully fetched into ${boost_SOURCE_DIR}")

set(CLP_FFI_JS_BIN_NAME
"ClpFfiJs"
CACHE STRING
"Binary name for the generated .js and .wasm files."
)
add_executable(${CLP_FFI_JS_BIN_NAME})

target_compile_features(${CLP_FFI_JS_BIN_NAME} PRIVATE cxx_std_20)

set(CMAKE_EXECUTABLE_SUFFIX ".js" CACHE STRING "Binary type to be generated by Emscripten.")

# Set up common compile and link options to be merged with other options as necessary.
set(CLP_FFI_JS_COMMON_COMPILE_OPTIONS
-fwasm-exceptions
)
set(CLP_FFI_JS_COMMON_LINK_OPTIONS
-fwasm-exceptions
-sALLOW_MEMORY_GROWTH
-sEXPORT_ES6
-sMODULARIZE
-sWASM_BIGINT
)
if(CMAKE_BUILD_TYPE MATCHES "Release")
set(CLP_FFI_JS_EXTRA_LINKER_FLAGS
list(APPEND CLP_FFI_JS_COMMON_COMPILE_OPTIONS
-flto
)
list(APPEND CLP_FFI_JS_COMMON_LINK_OPTIONS
-flto
--closure=1
-sENVIRONMENT=worker
)
else()
set(CLP_FFI_JS_EXTRA_LINKER_FLAGS -sENVIRONMENT=node)
endif()
message(
"CMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE}: Extra linker flags: ${CLP_FFI_JS_EXTRA_LINKER_FLAGS}"
)
target_link_options(
${CLP_FFI_JS_BIN_NAME}
PRIVATE
${CLP_FFI_JS_EXTRA_LINKER_FLAGS}
-sALLOW_MEMORY_GROWTH
-sEXPORT_ES6
-sMODULARIZE
-sWASM_BIGINT
--emit-tsd ${CLP_FFI_JS_BIN_NAME}.d.ts
)
target_link_libraries(${CLP_FFI_JS_BIN_NAME} PRIVATE embind)

target_compile_definitions(${CLP_FFI_JS_BIN_NAME} PUBLIC SPDLOG_FMT_EXTERNAL=1)

# NOTE: We mark the include directories below as system headers so that the compiler (including
# `clang-tidy`) doesn't generate warnings from them.
target_include_directories(
${CLP_FFI_JS_BIN_NAME}
SYSTEM
PRIVATE
${boost_SOURCE_DIR}
src/submodules/clp/components/core/src
src/submodules/clp/components/core/src/clp
src/submodules/clp/components/core/submodules
src/submodules/fmt/include
src/submodules/spdlog/include
src/submodules/zstd/lib
)

target_include_directories(${CLP_FFI_JS_BIN_NAME} PRIVATE src/)

set(CLP_FFI_JS_SRC_MAIN src/clp_ffi_js/ir/StreamReader.cpp)

Expand Down Expand Up @@ -139,11 +108,64 @@ set(CLP_FFI_JS_SRC_ZSTD
src/submodules/zstd/lib/decompress/zstd_decompress.c
)

target_sources(
${CLP_FFI_JS_BIN_NAME}
PRIVATE
set(CLP_FFI_JS_SUPPORTED_ENVIRONMENTS
node
worker
CACHE INTERNAL
"List of supported environments."
)

foreach(env ${CLP_FFI_JS_SUPPORTED_ENVIRONMENTS})
set(CLP_FFI_JS_BIN_NAME "ClpFfiJs-${env}")
add_executable(${CLP_FFI_JS_BIN_NAME})

# Set up compile options
target_compile_features(${CLP_FFI_JS_BIN_NAME} PRIVATE cxx_std_20)
target_compile_definitions(${CLP_FFI_JS_BIN_NAME} PUBLIC SPDLOG_FMT_EXTERNAL=1)
target_compile_options(${CLP_FFI_JS_BIN_NAME} PRIVATE ${CLP_FFI_JS_COMMON_COMPILE_OPTIONS})

# Set up link options
target_link_libraries(${CLP_FFI_JS_BIN_NAME} PRIVATE embind)
set(CLP_FFI_JS_LINK_OPTIONS
${CLP_FFI_JS_COMMON_LINK_OPTIONS}
kirkrodrigues marked this conversation as resolved.
Show resolved Hide resolved
--emit-tsd=${CLP_FFI_JS_BIN_NAME}.d.ts
kirkrodrigues marked this conversation as resolved.
Show resolved Hide resolved
-sENVIRONMENT=${env}
)
target_link_options(
${CLP_FFI_JS_BIN_NAME}
PRIVATE
${CLP_FFI_JS_LINK_OPTIONS}
)
message(
junhaoliao marked this conversation as resolved.
Show resolved Hide resolved
"CLP_FFI_JS_BIN_NAME=\"${CLP_FFI_JS_BIN_NAME}\". \
CMAKE_BUILD_TYPE=\"${CMAKE_BUILD_TYPE}\". \
Compile options: ${CLP_FFI_JS_COMMON_COMPILE_OPTIONS}. \
Link options: ${CLP_FFI_JS_LINK_OPTIONS}."
)

# NOTE: We mark the include directories below as system headers so that the compiler (including
# `clang-tidy`) doesn't generate warnings from them.
target_include_directories(
${CLP_FFI_JS_BIN_NAME}
SYSTEM
PRIVATE
${boost_SOURCE_DIR}
src/submodules/clp/components/core/src
src/submodules/clp/components/core/src/clp
src/submodules/clp/components/core/submodules
src/submodules/fmt/include
src/submodules/spdlog/include
src/submodules/zstd/lib
)

target_include_directories(${CLP_FFI_JS_BIN_NAME} PRIVATE src/)

target_sources(
${CLP_FFI_JS_BIN_NAME}
PRIVATE
${CLP_FFI_JS_SRC_MAIN}
${CLP_FFI_JS_SRC_CLP_CORE}
${CLP_FFI_JS_SRC_FMT}
${CLP_FFI_JS_SRC_ZSTD}
)
)
endforeach()
20 changes: 14 additions & 6 deletions Taskfile.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@ vars:
G_BUILD_DIR: "{{.ROOT_DIR}}/build"
G_CLP_FFI_JS_BUILD_DIR: "{{.G_BUILD_DIR}}/clp-ffi-js"
G_CLP_FFI_JS_CHECKSUM: "{{.G_BUILD_DIR}}/clp-ffi-js.md5"
G_CLP_FFI_JS_TARGET_NAME: "ClpFfiJs"
G_CLP_FFI_JS_ENV_NAMES: ["node", "worker"]
G_CLP_FFI_JS_TARGET_PREFIX: "ClpFfiJs-"
G_DIST_DIR: "{{.ROOT_DIR}}/dist"
G_EMSDK_DIR: "{{.G_BUILD_DIR}}/emsdk"
G_EMSDK_CHECKSUM: "{{.G_BUILD_DIR}}/emsdk.md5"
Expand Down Expand Up @@ -50,7 +51,11 @@ tasks:
DATA_DIR: "{{.OUTPUT_DIR}}"
cmds:
- task: "config-cmake-project"
- "cmake --build '{{.OUTPUT_DIR}}' --parallel --target '{{.G_CLP_FFI_JS_TARGET_NAME}}'"
- >-
cmake
--build '{{.OUTPUT_DIR}}'
--parallel
--target {{range .G_CLP_FFI_JS_ENV_NAMES}}"{{$.G_CLP_FFI_JS_TARGET_PREFIX}}{{.}}" {{end}}
# This command must be last
- task: "utils:compute-checksum"
vars:
Expand Down Expand Up @@ -115,10 +120,12 @@ tasks:
DATA_DIR: "{{.OUTPUT_DIR}}"
cmds:
- "rm -rf {{.OUTPUT_DIR}}"
- >-
rsync -a
"{{.G_CLP_FFI_JS_BUILD_DIR}}/{{.G_CLP_FFI_JS_TARGET_NAME}}."{d.ts,js,wasm}
"{{.OUTPUT_DIR}}/"
- for:
var: "G_CLP_FFI_JS_ENV_NAMES"
cmd: >-
rsync -a
"{{.G_CLP_FFI_JS_BUILD_DIR}}/{{.G_CLP_FFI_JS_TARGET_PREFIX}}{{.ITEM}}."{d.ts,js,wasm}
"{{.OUTPUT_DIR}}/"
- "npm pack"
# This command must be last
- task: "utils:compute-checksum"
Expand All @@ -138,6 +145,7 @@ tasks:
deps: ["emsdk"]
cmd: |-
cmake \
-DCLP_FFI_JS_SUPPORTED_ENVIRONMENTS="{{.G_CLP_FFI_JS_ENV_NAMES | join ";"}}" \
-DCMAKE_TOOLCHAIN_FILE="{{.G_EMSDK_DIR}}/upstream/emscripten/cmake/Modules/Platform/\
Emscripten.cmake" \
-S "{{.ROOT_DIR}}" \
Expand Down
15 changes: 12 additions & 3 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,22 @@
"scripts": {
"release": "git diff --exit-code && npm publish"
},
"type": "module",
"files": [
"./dist"
"dist"
],
"exports": {
".": {
"import": "./dist/ClpFfiJs.js",
"types": "./dist/ClpFfiJs.d.ts"
"import": "./dist/ClpFfiJs-worker.js",
"types": "./dist/ClpFfiJs-worker.d.ts"
},
"./node": {
"import": "./dist/ClpFfiJs-node.js",
"types": "./dist/ClpFfiJs-node.d.ts"
},
"./worker": {
"import": "./dist/ClpFfiJs-worker.js",
"types": "./dist/ClpFfiJs-worker.d.ts"
Comment on lines +24 to +33
Copy link

@coderabbitai coderabbitai bot Oct 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

💡 Codebase verification

🛠️ Refactor suggestion

Exported files are missing in the dist directory

The verification script indicates that the following exported files do not exist:

  • dist/ClpFfiJs-worker.js
  • dist/ClpFfiJs-worker.d.ts
  • dist/ClpFfiJs-node.js
  • dist/ClpFfiJs-node.d.ts

This suggests that the "exports" field in package.json references files that are missing, which may lead to runtime errors or broken imports.

Actions Needed:

  • Ensure that the build process generates the necessary files in the dist directory.
  • Verify the build configuration to include exports for both Node.js and Web Worker environments.
  • Update the "exports" field in package.json to reference existing files or adjust the build to produce the required files.
🔗 Analysis chain

Review the changes to export configurations

The modifications to the "exports" field indicate a shift towards supporting multiple environments (Node.js and Web Workers). However, there are some considerations:

  1. The main export (.) now points to the worker version by default. This might not be suitable for all use cases and could potentially break existing implementations.

  2. The addition of specific exports for node and worker environments provides more flexibility, which is good.

Consider the following suggestions:

  1. Make the main export more generic or provide a default that works in most environments. For example:
".": {
  "import": "./dist/ClpFfiJs.js",
  "types": "./dist/ClpFfiJs.d.ts"
}

  1. Ensure that the ClpFfiJs.js file can detect the environment and use the appropriate version (node or worker).

  2. Update the documentation to clearly explain the different export options and when to use each one.

Please run the following script to verify the existence of all referenced files:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the existence of exported files

files=(
  "dist/ClpFfiJs-worker.js"
  "dist/ClpFfiJs-worker.d.ts"
  "dist/ClpFfiJs-node.js"
  "dist/ClpFfiJs-node.d.ts"
)

for file in "${files[@]}"; do
  if [ ! -f "$file" ]; then
    echo "Warning: $file does not exist"
  else
    echo "$file exists"
  fi
done

Length of output: 553

Script:

#!/bin/bash
# Description: Verify the existence of exported files using fd

files=(
  "dist/ClpFfiJs-worker.js"
  "dist/ClpFfiJs-worker.d.ts"
  "dist/ClpFfiJs-node.js"
  "dist/ClpFfiJs-node.d.ts"
)

for file in "${files[@]}"; do
  if fd --type f --max-depth 2 "$(basename "$file")" | grep -q "$(basename "$file")"; then
    echo "$file exists"
  else
    echo "Warning: $file does not exist"
  fi
done

Length of output: 1237

Copy link
Member Author

Choose a reason for hiding this comment

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

The files are generated rather than directly checked in, so it's expected you don't see the files under /dist without any compilations.

Copy link

Choose a reason for hiding this comment

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

@junhaoliao, thank you for the clarification. I understand that the files under /dist are generated during the build process and are not checked into source control. I'll make a note of this for future reviews.

(_/)
(•_•)
(🧠) Curious rabbit learns

✏️ Learnings added 
Learnt from: junhaoliao
PR: y-scope/clp-ffi-js#11
File: package.json:24-33
Timestamp: 2024-10-05T19:27:36.977Z
Learning: In the `clp-ffi-js` project, the files in the `/dist` directory are generated during the build process and are not checked into source control, so their absence before compilation is expected.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

}
}
}
Loading