3rdpartyライブラリの更新 #283
3rdpartyライブラリの更新 #283
Conversation
Walkthroughサブモジュール Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as 開発者
participant CM as CMake (configure)
participant LP as libpng サブディレクトリ
participant PL as plateau ターゲット
participant CI as CI (upload-mobile-dlls)
Dev->>CM: プロジェクトをconfigure
alt Apple (macOS / iOS)
CM->>CM: `include_compat/fp.h` を生成
CM->>CM: C/C++FLAGS に `-isystem include_compat` を一時追加
end
CM->>LP: libpng をビルド（ターゲット: png_static）
CM->>CM: 一時フラグを復元
CM->>PL: plateau をリンク（依存: png_static）
CI->>CI: iOS/macOS ビルドで `-DPNG_ARM_NEON_OPT=0` をC_FLAGSに追加
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
CMakeLists.txt (2)
298-317: fp.h 互換ヘッダは「存在しない環境でのみ」適用に限定を現状は APPLE 全域で適用。macOS < 15 環境では本来の fp.h を不要に隠蔽しうるため、存在検出で条件分岐してください。C++言語はすでに有効なので
CheckIncludeFileCXXが使えます。include(CheckIncludeFileCXX) check_include_file_cxx("fp.h" HAVE_FP_H) if(APPLE AND NOT HAVE_FP_H) # ここに互換ヘッダ生成ロジック endif()ビルドログで
HAVE_FP_Hの判定結果をご確認ください。
310-317: include パスのリークと -isystem の二重指定を解消
include_directories(BEFORE SYSTEM ...)はディレクトリプロパティに残留し、他サブプロジェクトにも影響します。- さらに C/CXX フラグへ
-isystemを直接付与しており二重管理です。png のビルドターゲットにだけスコープするのが安全です。適用方針:
- ここでの
include_directories(...)と C/CXX フラグ書き換えを削除。add_subdirectory(3rdparty/libpng)の後で、png ターゲットにのみシステムインクルードを付与。- include_directories(BEFORE SYSTEM "${FP_H_COMPAT_DIR}") - - # libpngのビルド時のみCFLAGSを一時的に変更するため、現在の値を保存 - set(CMAKE_C_FLAGS_SAVE "${CMAKE_C_FLAGS}") - set(CMAKE_CXX_FLAGS_SAVE "${CMAKE_CXX_FLAGS}") - set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -isystem ${FP_H_COMPAT_DIR}") - set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -isystem ${FP_H_COMPAT_DIR}") + # インクルードは png ターゲットにだけ付与（add_subdirectory 後に設定）add_subdirectory の直後（行 319 の後）に以下を追加してください:
if(APPLE AND (NOT HAVE_FP_H)) # 互換ヘッダを png ターゲットに限定してシステムインクルード付与 target_include_directories(png_static SYSTEM BEFORE PRIVATE "${FP_H_COMPAT_DIR}") endif()C/CXX フラグの退避/復元（321-325行）も不要になります。
Also applies to: 321-325
src/CMakeLists.txt (1)
43-53: iOS/Android ブロックの "png_static" はターゲットとして参照を（クォート解除）文字列としてリンクすると使用要件（インクルード等）が伝播しません。実体ターゲットが存在するため、クォートを外してターゲットリンクにしてください。
- "png_static" + png_static他の内部ターゲットも可能なら非クォート化をご検討ください（citygml, GLTFSDK など）。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
3rdparty/libpng(1 hunks)3rdparty/zlib(1 hunks)CMakeLists.txt(1 hunks)src/CMakeLists.txt(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-and-test (ubuntu-24.04, x86_64)
- GitHub Check: build-and-test (windows-2022, x86_64)
- GitHub Check: build-and-test (macos-14, arm64)
🔇 Additional comments (3)
3rdparty/zlib (1)
1-1: サブモジュール更新を確認しました差分はコミット参照の更新のみで問題は見当たりませんでした。
src/CMakeLists.txt (2)
98-101: COMBINE_LIB_FILES の更新（png_static）OK静的結合対象の置換は妥当です。
117-118: COMBINE_LIB_DEPENDS の更新（png_static）OK依存関係の差し替えは妥当です。
| set(PNG_STATIC "defined" CACHE STRING "" FORCE) | ||
|
|
There was a problem hiding this comment.
PNG_STATIC の再代入（型不整合）を削除してください
PNG_STATIC ON の直後に set(PNG_STATIC "defined" ...) を文字列で再代入しており、オプションの意味を壊します。重複かつ誤用の可能性が高いです。後者の行を削除してください。Windows の消費側に PNG_STATIC マクロが必要なら、ターゲット側へ target_compile_definitions(... PUBLIC PNG_STATIC) を付けるのが正攻法です（別コメントで提案）。
-set(PNG_STATIC "defined" CACHE STRING "" FORCE)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| set(PNG_STATIC "defined" CACHE STRING "" FORCE) |
🤖 Prompt for AI Agents
In CMakeLists.txt around lines 290-291, remove the erroneous re-assignment
set(PNG_STATIC "defined" CACHE STRING "" FORCE) because it overwrites the
PNG_STATIC option with a string (type mismatch) and breaks the option; simply
delete that line and, if consumers on Windows actually need a PNG_STATIC
preprocessor macro, add target_compile_definitions(<target> PUBLIC PNG_STATIC)
on the appropriate targets instead of re-setting the cached variable.
| add_dependencies(plateau libjpeg-turbo png_static tiff hmm) | ||
| target_link_libraries(plateau PRIVATE ${JPEG_TURBO_LIBRARIES} png_static tiff hmm) | ||
| target_include_directories(plateau PRIVATE |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
❓ Verification inconclusive
png15_static → png_static 置換は妥当。Windows 向けに PNG_STATIC を公開定義してください
差し替えは正しいです。Windows で静的 libpng を使う場合、消費側が PNG_STATIC を定義していないと __declspec(dllimport) との不整合でリンク警告/失敗の恐れがあります。plateau に公開定義を追加してください。
add_dependencies(plateau libjpeg-turbo png_static tiff hmm)
target_link_libraries(plateau PRIVATE ${JPEG_TURBO_LIBRARIES} png_static tiff hmm)
+if(WIN32)
+ target_compile_definitions(plateau PUBLIC PNG_STATIC)
+endif()🏁 Script executed:
#!/bin/bash
# リポジトリ内に png15_static の残存参照がないか確認
rg -nP 'png15_static'Length of output: 0
Windowsビルド向けにPNG_STATIC定義を追加
静的リンクした libpng と __declspec(dllimport) の不整合によるリンク警告／失敗を防ぐため、CMakeLists.txt に以下を追加してください。
add_dependencies(plateau libjpeg-turbo png_static tiff hmm)
target_link_libraries(plateau PRIVATE ${JPEG_TURBO_LIBRARIES} png_static tiff hmm)
+if(WIN32)
+ target_compile_definitions(plateau PUBLIC PNG_STATIC)
+endif()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| add_dependencies(plateau libjpeg-turbo png_static tiff hmm) | |
| target_link_libraries(plateau PRIVATE ${JPEG_TURBO_LIBRARIES} png_static tiff hmm) | |
| target_include_directories(plateau PRIVATE | |
| add_dependencies(plateau libjpeg-turbo png_static tiff hmm) | |
| target_link_libraries(plateau PRIVATE ${JPEG_TURBO_LIBRARIES} png_static tiff hmm) | |
| if(WIN32) | |
| target_compile_definitions(plateau PUBLIC PNG_STATIC) | |
| endif() | |
| target_include_directories(plateau PRIVATE |
🤖 Prompt for AI Agents
In src/CMakeLists.txt around lines 34 to 36, the build can get link
warnings/failures on Windows due to using a statically linked libpng while
symbols expect __declspec(dllimport); add the PNG_STATIC preprocessor definition
for the plateau target when building on Windows. Modify the CMake file to set
the definition only for Windows (e.g., check WIN32 or MSVC) and apply it to the
plateau target via target_compile_definitions(... PRIVATE PNG_STATIC) so the
static libpng export/import mismatch is avoided.
|
libpngのmobileビルド時のエラー修正対応Done |
| -D CMAKE_OSX_ARCHITECTURES="arm64" | ||
| -D CMAKE_OSX_DEPLOYMENT_TARGET=13.0 | ||
| -D CMAKE_BUILD_TYPE=Release | ||
| -D CMAKE_C_FLAGS="-DPNG_ARM_NEON_OPT=0" |
There was a problem hiding this comment.
CMAKE_C_FLAGSを直接上書きしないでください
cmake -D CMAKE_C_FLAGS="..." でキャッシュ値を丸ごと置き換えると、Apple向けツールチェーンが自動で設定していた必須フラグ（-fembed-bitcode や -isysroot など）が消失し、最悪ビルド失敗や提出要件を満たせなくなる恐れがあります。PNGのNEON最適化だけを抑止したいのであれば、target_compile_definitions() 等で該当ターゲットに PNG_ARM_NEON_OPT=0 を付与するか、CMake側に専用オプションを生やして切り替えるようにしてください。
🤖 Prompt for AI Agents
.github/actions/upload-mobile-dlls/action.yml around line 91: this change
overwrites the entire cached CMAKE_C_FLAGS via -D CMAKE_C_FLAGS="...", which
removes Apple toolchain required flags (e.g. -fembed-bitcode, -isysroot) and can
break builds; instead, stop setting CMAKE_C_FLAGS directly and apply
PNG_ARM_NEON_OPT=0 to the specific build target(s) (use
target_compile_definitions() in the CMakeLists for the iOS-related targets) or
add a dedicated CMake option (e.g. -DPNG_ARM_NEON_OPT=0 used within CMake to set
a definition) so only the PNG_NEON macro is disabled without replacing global
compiler flags.
🔗 関連リンク
✅ レビュー前確認項目
✅ マージ前確認項目
🚀 実装内容
3rdpartyライブラリのバージョン上げる対応を実施しました
🌐 影響範囲
🛠️ 動作確認
Summary by CodeRabbit