-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
MUSICA Tutorial Chapter 0 #173
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I just left a couple questions.
tutorial/CMakeLists.txt
Outdated
target_include_directories(demo_c PUBLIC ${MUSICA_INCLUDE_DIR}) | ||
target_link_directories(demo_c PUBLIC ${MUSICA_LIB_DIR}) | ||
|
||
target_include_directories(demo_f PUBLIC ${MUSICA_INCLUDE_DIR}) | ||
target_link_directories(demo_f PUBLIC ${MUSICA_LIB_DIR}) | ||
target_link_libraries(demo_f musica-fortran musica stdc++) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of using target_link_directories
, could you use target_link_libraries
as it is discouraged to use as stated in the docs? I also wonder whether you need them. When you just use target_link_librararies
to link musica, does it fail?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @mattldawson, @K20shores, do you think we should add these demo_c
and demo_f
to the musica test suite?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I would add any tutorial code to the test suite
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, @dwfncar, could you make demo_*
as a test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without the target_link_directories
I get the link errors /usr/bin/ld: cannot fine -lmusica-fortran
(and -lmusica
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added demo.f90 as a unit test in fortran/test/unit/CMakeLists.txt, will add demo.c when it has actual content.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm maybe instead oftutorial/
being the top level directory it might need to be a subdirectory offortran/
like micm
? Then it could link to musica-fortran
library. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I would recommend the tutorial tests being in micm/fortran/test/tutorial
and you should be able to just use the create_standard_fortran_test()
macro (I think). That macro links tests against the musica-fortran
library. Similar to this:
musica/fortran/test/unit/CMakeLists.txt
Lines 6 to 8 in 69bcb2e
create_standard_test_fortran(NAME micm_fortran_api SOURCES ../fetch_content_integration/test_micm_api.F90) | |
create_standard_test_fortran(NAME get_micm_version SOURCES ../fetch_content_integration/test_get_micm_version.F90) | |
create_standard_test_fortran(NAME micm_box_model SOURCES ../fetch_content_integration/test_micm_box_model.F90) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! Thank you for addressing the comments!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I missed target link directory
thing before I submitted the reveiw
target_include_directories(demo_f PUBLIC ${MUSICA_INCLUDE_DIR}) | ||
target_link_directories(demo_f PUBLIC ${MUSICA_LIB_DIR}) | ||
target_link_libraries(demo_f musica-fortran musica stdc++) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops sorry, I missed it, could you remove target_link_directories(..)
?
I also think you don't need to link to the musica library.
target_include_directories(demo_f PUBLIC ${MUSICA_INCLUDE_DIR}) | |
target_link_directories(demo_f PUBLIC ${MUSICA_LIB_DIR}) | |
target_link_libraries(demo_f musica-fortran musica stdc++) | |
target_include_directories(demo_f PUBLIC ${MUSICA_INCLUDE_DIR}) | |
target_link_libraries(demo_f PRIVATE musica::musica-fortran) |
Added Chapter 0 tutorial describing a CMakeLists.txt to link to the musica-fortran library.