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

Incorporate MICM into build and start chem process structure #114

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from

Conversation

zmoon
Copy link
Collaborator

@zmoon zmoon commented Jan 27, 2025

@@ -1,6 +1,6 @@
cmake_minimum_required(VERSION 3.5)

project(CATChem VERSION 0.1.0 LANGUAGES Fortran)
project(CATChem VERSION 0.1.0 LANGUAGES Fortran C CXX)
Copy link
Collaborator Author

@zmoon zmoon Jan 27, 2025

Choose a reason for hiding this comment

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

Just nothing that, without adding these, was getting this error:

CMake Error in src/process/chem/CMakeLists.txt:
  No known features for CXX compiler

  ""

  version .

Copy link
Collaborator Author

@zmoon zmoon Jan 27, 2025

Choose a reason for hiding this comment

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

Also note, it seems like really it's only necessary to add CXX to get a successful build

Copy link
Collaborator Author

@zmoon zmoon Jan 28, 2025

Choose a reason for hiding this comment

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

@K20shores do you know if there's a way to get MUSICA to build without changing this, by specifying something elsewhere instead? Just curious, as our project code is all Fortran really.

Choose a reason for hiding this comment

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

@zmoon I think this is required. If not, I don't think the linker will be properly configured. I could be wrong though

Choose a reason for hiding this comment

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

@zmoon I was able to build your fork

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks @K20shores , that's good to hear.

Copy link

@K20shores K20shores Feb 13, 2025

Choose a reason for hiding this comment

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

Ah, if you want to only enable these languages only when musica is built, somewhere you can do something like this

if (ENABLE_MUSICA)
    enable_language(C)
    enable_language(CXX)
endif()

@zmoon zmoon marked this pull request as ready for review January 27, 2025 22:41
tests/test_chem.f90 Outdated Show resolved Hide resolved
src/external/CMakeLists.txt Outdated Show resolved Hide resolved
@bbakernoaa
Copy link
Collaborator

@mbruckner-work Please take a look

@rschwant
Copy link
Collaborator

Very cool! Let's talk about this in our UFS-Chem meetings on Monday. I have a couple of questions, but this looks great!

Comment on lines +3 to +5
set(_srcs CCPr_Chem_Mod.F90)

add_library(${_lib} ${_srcs})
Copy link

@K20shores K20shores Feb 13, 2025

Choose a reason for hiding this comment

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

The only cmake related comment I can think of is that it's recommended to use target_sources in newer versions of cmake. I think if you use target_sources and generate a project for something like Xcode or Visual Studio, this helps Cmake create better integration with that IDE. This certainly isn't a requirement though

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks @K20shores , I can update the whole project to use that in a separate PR.

Copy link
Collaborator

@mattldawson mattldawson left a comment

Choose a reason for hiding this comment

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

looks great!

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.

6 participants