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.

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

Link internal library as CMake object library #710

Merged
merged 4 commits into from
Nov 14, 2024

Conversation

sksat
Copy link
Collaborator

@sksat sksat commented Nov 12, 2024

Related issues

N/A

Description

s2e-core-internal libraries like COMPONENT have complex (and cyclic) dependencies with each other, since they are only for build S2E.

However, these libraries are built as static libraries.
This causes the final executable link to not succeed unless non-trivial link configuration is performed many times.

s2e-core/CMakeLists.txt

Lines 185 to 198 in 6ca93ed

target_link_libraries(COMPONENT DYNAMICS GLOBAL_ENVIRONMENT LOCAL_ENVIRONMENT MATH_PHYSICS SETTING_FILE_READER LOGGER UTILITIES)
target_link_libraries(DYNAMICS GLOBAL_ENVIRONMENT LOCAL_ENVIRONMENT SIMULATION MATH_PHYSICS)
target_link_libraries(DISTURBANCE DYNAMICS GLOBAL_ENVIRONMENT LOCAL_ENVIRONMENT MATH_PHYSICS)
target_link_libraries(SIMULATION DYNAMICS GLOBAL_ENVIRONMENT LOCAL_ENVIRONMENT DISTURBANCE MATH_PHYSICS)
target_link_libraries(GLOBAL_ENVIRONMENT ${CSPICE_LIB} MATH_PHYSICS)
target_link_libraries(LOCAL_ENVIRONMENT GLOBAL_ENVIRONMENT ${CSPICE_LIB} MATH_PHYSICS)
target_link_libraries(MATH_PHYSICS ${NRLMSISE00_LIB})
target_link_libraries(SETTING_FILE_READER INIH)
target_link_libraries(${PROJECT_NAME} DYNAMICS)
target_link_libraries(${PROJECT_NAME} DISTURBANCE)
target_link_libraries(${PROJECT_NAME} SIMULATION)
target_link_libraries(${PROJECT_NAME} GLOBAL_ENVIRONMENT LOCAL_ENVIRONMENT)
target_link_libraries(${PROJECT_NAME} COMPONENT)

This PR change these libraries to be built as object libraries feature provided by CMake.
It eliminates the need to specify dependencies mid-build by allowing circular dependencies to exist until the final executable link.

s2e-core/CMakeLists.txt

Lines 183 to 200 in 7f8ae7c

target_link_libraries(${PROJECT_NAME}
LOGGER
SETTING_FILE_READER
INIH
UTILITIES
COMPONENT
DISTURBANCE
DYNAMICS
GLOBAL_ENVIRONMENT
LOCAL_ENVIRONMENT
MATH_PHYSICS
SIMULATION
# ExtLibraries
${NRLMSISE00_LIB}
${CSPICE_LIB}
)

Test results

N/A

Impact

The build process is a bit changed; S2E users also need to make a similar modification to this PR in their CMakeLists.txt.

Supplementary information

This simplifies the build process and makes it easier to expand use cases, such as interoperation with Rust.

@sksat sksat added priority::medium priority medium tools labels Nov 12, 2024
@sksat sksat requested a review from 200km November 12, 2024 05:33
@sksat sksat self-assigned this Nov 12, 2024
@sksat sksat requested a review from a team as a code owner November 12, 2024 05:33
@sksat sksat requested review from ogoogo, Hiro-0110, seki-hiro, suzuki-toshihir0, t-hosonuma and conjikidow and removed request for a team November 12, 2024 05:33
@sksat
Copy link
Collaborator Author

sksat commented Nov 12, 2024

Win, 32bit, C2A=ON の時だけこの変更で落ちるのはあまりに変なので、Actions の minor update などを疑って #711 をやったが、違いそう

@sksat
Copy link
Collaborator Author

sksat commented Nov 12, 2024

rebase

@sksat sksat force-pushed the feature/link-internal-lib-with-cmake-object-lib branch 3 times, most recently from 46ac5cc to c1062c2 Compare November 14, 2024 08:41
@sksat
Copy link
Collaborator Author

sksat commented Nov 14, 2024

64bit で USE_C2A=ONの時、CI はパスしてるけどビルドがスキップされてる

@sksat
Copy link
Collaborator Author

sksat commented Nov 14, 2024

#409 で変更されてるな

@sksat
Copy link
Collaborator Author

sksat commented Nov 14, 2024

そもそも 64bit ビルドが追加されたのがここか

@sksat
Copy link
Collaborator Author

sksat commented Nov 14, 2024

単純に libC2A をリンクしようとした時にリンクエラーになる、のであればまだ make sense。しかし、SILS-S2E の際は 32bit ビルドを強制させたい、のであれば後出しジャンケン的に無かったことにするべきではない。

@200km
Copy link
Member

200km commented Nov 14, 2024

しかし、SILS-S2E の際は 32bit ビルドを強制させたい、のであれば後出しジャンケン的に無かったことにするべきではない。

そうですね。これはこれ以外のやり方がいまいちわからなかったので無理やりやったという感じです。
スマートな感じに修正していただけると嬉しいです!

@sksat
Copy link
Collaborator Author

sksat commented Nov 14, 2024

@200km #712 で、include という機能を使って存在する組み合わせをベタ書きするようにしてみました

@sksat
Copy link
Collaborator Author

sksat commented Nov 14, 2024

rebase

@sksat sksat force-pushed the feature/link-internal-lib-with-cmake-object-lib branch 2 times, most recently from a6a1a0c to 8f93dde Compare November 14, 2024 10:15
@sksat sksat force-pushed the feature/link-internal-lib-with-cmake-object-lib branch from 8f93dde to 159c9c9 Compare November 14, 2024 11:20
@sksat sksat force-pushed the feature/link-internal-lib-with-cmake-object-lib branch from 159c9c9 to 52c0199 Compare November 14, 2024 11:24
@sksat
Copy link
Collaborator Author

sksat commented Nov 14, 2024

Visual Studio のリンカ(link.exe)の挙動が微妙だったためやや dirty hack も入っているが、通った(ここらへんは native Windows のサポートを続けるかどうかも含めて考えたい)

@sksat
Copy link
Collaborator Author

sksat commented Nov 14, 2024

@200km いつもの摩訶不思議なことになってしまっている target_link_libraries() をシンプルにするパッチです。ビルドがシンプルになると AE 側の Rust の資産との接続が行いやすくなるといったメリットもあるため、マージできるとうれしいです。

@sksat
Copy link
Collaborator Author

sksat commented Nov 14, 2024

FYI: @suzuki-toshihir0 @tarotene

Copy link
Member

@200km 200km left a comment

Choose a reason for hiding this comment

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

修正提案ありがとうございます!

@sksat sksat merged commit 109f1f3 into develop Nov 14, 2024
10 checks passed
@sksat sksat deleted the feature/link-internal-lib-with-cmake-object-lib branch November 14, 2024 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: 💮 Done
Development

Successfully merging this pull request may close these issues.

2 participants