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

Update for cmake use of neural-fortran #192

Merged

Conversation

mathomp4
Copy link
Contributor

I recently tried building neural-fortran and then wanted to try and use it with an external code (which were essentially the examples herein).

In doing so, I couldn't quite get the current Findneural-fortran.cmake file from #178 (via #166) to work for me.

So, I did some hacking around and came up with this PR. The changes are:

  1. Simplify the Findneural-fortran.cmake file. The original was a bit above me so I made something a bit easier to use. For this one, a user needs to set neural-fortran_ROOT_DIR such that neural-fortran_ROOT_DIR/lib/libneural-fortran.a exists. It also looks for the .mod files as those are also needed for external use.
  2. Install the .mod files to CMAKE_INSTALL_PREFIX/include so that they can be found by the find_package
  3. Install the Findneural-fortran.cmake in CMAKE_INSTALL_PREFIX/include/cmake/neural-fortran...which might not be the best place. I just wanted to put it somewhere in the install
  4. Update the README with a bit more info

In my testing, this all seems to work both for external and internal use. For example:

cmake -B build -S . --install-prefix=$(pwd)/install -Dneural-fortran_BUILD_TESTING=YES -Dneural-fortran_BUILD_EXAMPLES=YES
cmake --build build
cmake --install build
ctest --test-dir build

all worked and the examples seem to as well.

I'm going to keep this draft (for now) until @milancurcic can test. I'd also like to make sure I haven't broken the workflow of @jvdp1 who added the first Findneural-fortran.cmake file. And I guess I'll ping @aminiussi who is the filer of #166

@mathomp4
Copy link
Contributor Author

Huh. I didn't seem to trigger the CI...

@mathomp4
Copy link
Contributor Author

Also: I don't know how to use fpm so I hope I didn't break that!

@milancurcic milancurcic marked this pull request as ready for review September 11, 2024 18:37
@milancurcic
Copy link
Member

I think the CI is configured to only trigger CI on source changes. We should fix that!

Thanks for this, will test. fpm doesn't care about this part.

@mathomp4
Copy link
Contributor Author

I think the CI is configured to only trigger CI on source changes. We should fix that!

Thanks for this, will test. fpm doesn't care about this part.

Ahh. I could update that in my PR? We can add CMakeLists.txt to the "if changed" list.

@mathomp4
Copy link
Contributor Author

Okay, CMakeLists.txt added and I fiddled a bit with the CI. Looks like you have to approve the CI run 😄

Copy link
Member

@milancurcic milancurcic left a comment

Choose a reason for hiding this comment

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

Thank you!

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@milancurcic
Copy link
Member

Thanks @mathomp4, I'm not merging in case you're still working on it, let me know when good to merge.

@mathomp4
Copy link
Contributor Author

Thanks @mathomp4, I'm not merging in case you're still working on it, let me know when good to merge.

@milancurcic I think I'm good. Once it's in, I'll give it a test but this works for me!

@milancurcic milancurcic merged commit d516437 into modern-fortran:main Sep 13, 2024
4 checks passed
@mathomp4 mathomp4 deleted the feature/mathomp4/update-cmake branch September 13, 2024 14:33
@mathomp4
Copy link
Contributor Author

Thanks for merging! I'll work with main until next release :)

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