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 5 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
105 changes: 56 additions & 49 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,59 +70,14 @@ 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.")

if(CMAKE_BUILD_TYPE MATCHES "Release")
set(CLP_FFI_JS_EXTRA_LINKER_FLAGS
-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 @@ -151,11 +106,63 @@ set(CLP_FFI_JS_SRC_ZSTD
src/submodules/zstd/lib/decompress/zstd_decompress.c
)

target_sources(
${CLP_FFI_JS_BIN_NAME}
PRIVATE
# Define supported environments
set(CLP_FFI_JS_ENVIRONMENTS
kirkrodrigues marked this conversation as resolved.
Show resolved Hide resolved
"node"
"worker"
)

foreach(env ${CLP_FFI_JS_ENVIRONMENTS})
set(CLP_FFI_JS_BIN_NAME "ClpFfiJs-${env}")
add_executable(${CLP_FFI_JS_BIN_NAME})
target_compile_features(${CLP_FFI_JS_BIN_NAME} PRIVATE cxx_std_20)

set(CLP_FFI_JS_LINKER_FLAGS
${CLP_FFI_JS_EXTRA_LINKER_FLAGS}
-fwasm-exceptions
-sALLOW_MEMORY_GROWTH
-sEXPORT_ES6
-sMODULARIZE
-sWASM_BIGINT
--emit-tsd=${CLP_FFI_JS_BIN_NAME}.d.ts
kirkrodrigues marked this conversation as resolved.
Show resolved Hide resolved
)
message(
"CLP_FFI_JS_BIN_NAME=\"${CLP_FFI_JS_BIN_NAME}\". \
CMAKE_BUILD_TYPE=\"${CMAKE_BUILD_TYPE}\". \
Linker flags: ${CLP_FFI_JS_LINKER_FLAGS}"
kirkrodrigues marked this conversation as resolved.
Show resolved Hide resolved
)
target_link_options(
${CLP_FFI_JS_BIN_NAME}
PRIVATE
${CLP_FFI_JS_LINKER_FLAGS}
)
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/)

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()
junhaoliao marked this conversation as resolved.
Show resolved Hide resolved
17 changes: 10 additions & 7 deletions Taskfile.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ 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_TARGET_NAMES: "ClpFfiJs-node ClpFfiJs-worker"
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 @@ -41,7 +41,7 @@ tasks:
- "src/**/*"
generates: ["{{.CHECKSUM_FILE}}"]
deps:
- "emsdk"
# - "emsdk"
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

emsdk Dependency Remains in Use

The "emsdk" dependency is still referenced in both CMakeLists.txt and Taskfile.yml. Commenting out the dependency in Taskfile.yml does not fully remove its usage and may impact the build process. Please ensure that all references to "emsdk" are appropriately handled.

🔗 Analysis chain

Please clarify the reason for commenting out the emsdk dependency.

The emsdk dependency has been commented out, which could potentially affect the build process. This change wasn't mentioned in the PR objectives or summary. Could you please explain the rationale behind this modification?

To ensure this change doesn't introduce issues, please run the following script:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if emsdk is still required in the build process

# Test: Search for emsdk usage in CMakeLists.txt and other relevant files
rg --type cmake 'emscripten|emsdk' CMakeLists.txt
rg --type yaml 'emscripten|emsdk' Taskfile.yml

Length of output: 1314

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, that was a typo. Reverted.

Copy link

Choose a reason for hiding this comment

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

@junhaoliao, thanks for correcting the typo!

(^_^)/

If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!

- task: "utils:validate-checksum"
vars:
CHECKSUM_FILE: "{{.CHECKSUM_FILE}}"
Expand All @@ -50,7 +50,7 @@ tasks:
- "mkdir -p '{{.OUTPUT_DIR}}'"
- |-
cmake -S "{{.ROOT_DIR}}" -B "{{.OUTPUT_DIR}}" -G "Unix Makefiles"
cmake --build "{{.OUTPUT_DIR}}" --parallel --target "{{.G_CLP_FFI_JS_TARGET_NAME}}"
cmake --build "{{.OUTPUT_DIR}}" --parallel --target {{.G_CLP_FFI_JS_TARGET_NAMES}}
# This command must be last
- task: "utils:compute-checksum"
vars:
Expand Down Expand Up @@ -114,10 +114,13 @@ 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_TARGET_NAMES"
split: " "
cmd: >-
rsync -a
"{{.G_CLP_FFI_JS_BUILD_DIR}}/{{.ITEM}}."{d.ts,js,wasm}
"{{.OUTPUT_DIR}}/"
- "npm pack"
# This command must be last
- task: "utils:compute-checksum"
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"
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.

⚠️ Potential issue

Correct the "files" field path

The change from "./dist" to "/dist" in the "files" field is problematic. The leading slash in "/dist" typically refers to the root of the file system, which is likely not the intended behaviour.

Please change this back to "./dist" or simply "dist" to correctly reference the distribution directory relative to the package root. For example:

"files": [
  "dist"
]

This ensures that the correct directory is included when the package is published.

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe the / refers to the project root. Correct me if I'm wrong.

Copy link

Choose a reason for hiding this comment

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

@junhaoliao, in the package.json files field, the leading slash / refers to the root of the filesystem, not the project root. This means that specifying "/dist" may not include the intended directory when publishing your package. Paths in the files field should be relative to the package root, so it's recommended to use "./dist" or simply "dist" to correctly reference the dist directory.

(_/)
(•_•)
( > 🧐

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 slash was needed to mitigate an issue when I was simulated the package publish / installation locally with Yalc on a Mac computer (previously I don't recall observing the issue on a Linux computer). With the "./", the "dist" folder does not seem to be correctly included in the installations.
Still, it seems with no "./" or "/" added the installation and the config works fine locally with Yalc. Will stick with simply "dist" then.

Copy link

Choose a reason for hiding this comment

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

@junhaoliao, glad to hear that using "dist" works correctly with Yalc on Mac as well. Let me know if you have any other questions.

(_/)
(•ᴗ•)
( ♥

✏️ Learnings added 
Learnt from: junhaoliao
PR: y-scope/clp-ffi-js#11
File: package.json:0-0
Timestamp: 2024-10-05T19:26:30.186Z
Learning: Using `"dist"` without any leading `"./"` or `"/"` in the `"files"` field of `package.json` ensures proper inclusion of the `dist` folder during package installation, especially when using tools like Yalc on Mac computers.

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.

If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!

],
"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.

}
}
}