Skip to content

Add CMakeLists.txt#69

Open
qr243vbi wants to merge 1 commit into
G-Yong:mainfrom
qr243vbi:upstream
Open

Add CMakeLists.txt#69
qr243vbi wants to merge 1 commit into
G-Yong:mainfrom
qr243vbi:upstream

Conversation

@qr243vbi
Copy link
Copy Markdown

Add CMakeLists to use with cmake

@G-Yong
Copy link
Copy Markdown
Owner

G-Yong commented May 24, 2026

Thanks for adding CMake support. I found a couple of issues that should be addressed before this can reliably build on downstream projects, especially on Windows/MSVC.

  1. src/QScriptEngine/CMakeLists.txt, around add_library(${PROJECT_NAME} STATIC SHARED ...):

CMake only creates one target here; in practice this generates a shared library with the VS generator. The project does not define export macros or enable CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS, so consumers such as QQScriptDemo are likely to hit unresolved external symbols on MSVC. Please choose one library type explicitly, or make this controlled by BUILD_SHARED_LIBS and add the required Windows export handling if shared builds are intended.

  1. src/QScriptEngine/CMakeLists.txt, around target_include_directories(...) / target_link_libraries(${PROJECT_NAME} PRIVATE qjs):

The public headers (QScriptEngine.h, QScriptValue.h, etc.) include quickjs.h and expose QuickJS types such as JSContext, JSRuntime, and JSValue. However, the CMake target does not publish QuickJS's include directory/usage requirements to consumers, because qjs is linked privately and the public include dirs only contain src/QScriptEngine and scriptEngine/include. A downstream target that links QuickQtScript and includes these headers can fail with quickjs.h not found. Please make the QuickJS usage requirement public, for example by linking qjs as PUBLIC when using the bundled target, or otherwise adding the appropriate QuickJS include directory to the public interface.

@G-Yong
Copy link
Copy Markdown
Owner

G-Yong commented May 24, 2026

Also, I see that your CMakeLists.txt file is hard-coded to use Qt6. You could actually follow the convention used in projects automatically generated by Qt Creator—Qt${QT_VERSION_MAJOR}—to automatically adapt to the version, thereby making it compatible with both Qt5 and Qt6.

find_package(QT NAMES Qt6 Qt5 REQUIRED COMPONENTS Core)
find_package(Qt${QT_VERSION_MAJOR} REQUIRED COMPONENTS Core)

set(CMAKE_CXX_STANDARD_REQUIRED ON)
set(CMAKE_AUTOMOC ON)

find_package(Qt6 REQUIRED COMPONENTS
Copy link
Copy Markdown
Owner

@G-Yong G-Yong May 24, 2026

Choose a reason for hiding this comment

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

Suggested change
find_package(Qt6 REQUIRED COMPONENTS
find_package(QT NAMES Qt6 Qt5 REQUIRED COMPONENTS Core)
find_package(Qt${QT_VERSION_MAJOR} REQUIRED COMPONENTS

Adaptive Qt version

else()
message(STATUS "Using BUNDLED QuickJS")

add_subdirectory(quickjs)
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

It doesn’t seem necessary to include the entire QuickJS library, as we only need a few of its files. If we include the whole thing, it looks as though it will compile things like qjs by default, which we don’t need.

Qt6::Core
)

target_compile_options(${PROJECT_NAME} PRIVATE -fpermissive -Wno-narrowing)
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Suggested change
target_compile_options(${PROJECT_NAME} PRIVATE -fpermissive -Wno-narrowing)
if(CMAKE_CXX_COMPILER_ID MATCHES "GNU|Clang")
target_compile_options(${PROJECT_NAME} PRIVATE -fpermissive -Wno-narrowing)
endif()

-fpermissive and -Wno-narrowing are GCC/Clang-specific flags; they will cause errors in MSVC. A compiler check is required.

Comment thread .gitignore
Comment on lines +166 to +169

src/QScriptEngine/build

src/QQScriptDemo/build
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Suggested change
src/QScriptEngine/build
src/QQScriptDemo/build

The .gitignore file already contains *build*/, so the two new entries are actually redundant (as they are already covered by the wildcard).

Comment on lines +12 to +18
# -------------------------------------------------
# Includes
# -------------------------------------------------
include_directories(
${CMAKE_CURRENT_SOURCE_DIR}
${CMAKE_CURRENT_SOURCE_DIR}/scriptEngine/include
)
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Suggested change
# -------------------------------------------------
# Includes
# -------------------------------------------------
include_directories(
${CMAKE_CURRENT_SOURCE_DIR}
${CMAKE_CURRENT_SOURCE_DIR}/scriptEngine/include
)

Since target_include_directories(...) already handles the same path, the global include_directories variable will pollute the parent project’s scope and should be removed.

# -------------------------------------------------
# Library target
# -------------------------------------------------
add_library(${PROJECT_NAME} STATIC SHARED
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

STATIC SHARED is an invalid parameter; CMake will return an error immediately. You should either specify a single type or omit the type entirely (allowing the user to control this via BUILD_SHARED_LIBS).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants