feat: introduce jank lib with static runtime #347
Conversation
Samy-33
commented
Jul 12, 2025
Samy-33
commented
- Also allows user to choose type of runtime while ahead of compiling their projects
- Also allows user to choose type of runtime while ahead of compiling their projects
jeaye
left a comment
jeaye
left a comment
There was a problem hiding this comment.
Nice! This came together well. I have some cleanup requests, since there are a few different forms of duplication throughout, but I think the overall approach will work well.
Assuming you've tried this with your AOT compilation, will you please provide the ldd (linux) or otool -L (mac) output for the resulting static runtime AOT binary in the PR description?
|
|
||
| add_library( | ||
| jank_lib STATIC | ||
| set(JANK_LIBRARY_COMMON_SOURCES |
There was a problem hiding this comment.
No need to uppercase all of this. All of our CMake variables are lowercase.
| # Native module sources. | ||
| src/cpp/clojure/core_native.cpp | ||
| src/cpp/clojure/string_native.cpp | ||
| src/cpp/jank/compiler_native.cpp |
There was a problem hiding this comment.
Why keep this one in the static sources? It requires codegen for the only function in there.
| "$<BUILD_INTERFACE:${PROJECT_SOURCE_DIR}/third-party/nanobench/include>" | ||
| "$<BUILD_INTERFACE:${PROJECT_SOURCE_DIR}/third-party/folly>" | ||
| "$<BUILD_INTERFACE:${PROJECT_SOURCE_DIR}/third-party/bpptree/include>" | ||
| "$<BUILD_INTERFACE:${PROJECT_SOURCE_DIR}/third-party/immer>" | ||
| "$<BUILD_INTERFACE:${PROJECT_SOURCE_DIR}/third-party/magic_enum/include/magic_enum>" | ||
| "$<BUILD_INTERFACE:${PROJECT_SOURCE_DIR}/third-party/cli11/include>" | ||
| "$<BUILD_INTERFACE:${PROJECT_SOURCE_DIR}/third-party/ftxui/include>" | ||
| "$<BUILD_INTERFACE:${PROJECT_SOURCE_DIR}/third-party/libzippp/src>" | ||
| "$<BUILD_INTERFACE:${PROJECT_SOURCE_DIR}/third-party/cpptrace/include>" | ||
| "$<BUILD_INTERFACE:${PROJECT_SOURCE_DIR}/third-party/boost-preprocessor/include>" | ||
| "$<BUILD_INTERFACE:${PROJECT_SOURCE_DIR}/third-party/boost-multiprecision/include>" |
There was a problem hiding this comment.
We need to not duplicate all of this between the two libs. Same goes for the linked libs and the common compile options.
| * of previous code. This is essential for REPL use. */ | ||
| /* TODO: This needs to be synchronized. */ | ||
| analyze::processor an_prc{ *this }; | ||
| #ifndef JANK_STATIC_RUNTIME |
There was a problem hiding this comment.
Let's def JANK_DYNAMIC_RUNTIME for dynamic builds and JANK_STATIC_RUNTIME for static builds. Checking for "not static" is both indirect and potentially too broad. For example, if we add a third runtime, this check would also match that.
| native_vector<jtl::immutable_string> define_macros; | ||
| native_vector<jtl::immutable_string> libs; | ||
|
|
||
| jtl::immutable_string runtime; |
There was a problem hiding this comment.
We want an enum for this, not a string. It only has two possible values, whereas the domain of a string is infinite.
| native_vector<runtime::obj::symbol_ref> | ||
| params) // NOLINT(performance-unnecessary-value-param): This is a dummy implementation for static runtime | ||
| { | ||
| throw std::runtime_error{ "Eval disabled in static runtime." }; |
There was a problem hiding this comment.
Let's define a constexpr C string at the top of the file with this message, instead of duplicating it 24 times.
| analyze::expression_ref const expr, | ||
| jtl::immutable_string const &name, | ||
| native_vector<runtime::obj::symbol_ref> | ||
| params) // NOLINT(performance-unnecessary-value-param): This is a dummy implementation for static runtime |
There was a problem hiding this comment.
Please put the comment before the arg, use NOLINTNEXTLINE, use /* */ style comments, and end the comment in a . since comments are documentation.
| #pragma clang diagnostic push | ||
| #pragma clang diagnostic ignored "-Wunused-parameter" |
There was a problem hiding this comment.
This is not needed if you just remove the param names.
| } | ||
| return ok(); | ||
| #else | ||
| return err("Load or eval not allowed in static runtime"); |
There was a problem hiding this comment.
Every error must be a complete sentence or paragraph, so please end this with a ..
|
|
||
| return ok(); | ||
| #else | ||
| return err("Can't load file in static runtime"); |
There was a problem hiding this comment.
Please use correct grammar and punctuation for error messages.
Dynamic file loading is disable in jank's static runtime.
Let's use this for load_o, load_jank, and load_cpp. Put it into a constexpr global so we're not repeating it. Making sure we write helpful error messages is important in every part of the system.
2 participants
