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

Switch to CMake #35

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

bradlaufenberg
Copy link
Contributor

So far I have successfully built the src directory using CMake. The next step is to do the same for the rest of the sub directories.

Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

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

Assuming that we'll use a series of different PRs, it's probably best to remove the empty CMakeLists.txt files that have been added here.

We also need to decide how we'll properly set the default data directory and VERSION using CMake.


FILE(GLOB h_files "${CMAKE_CURRENT_SOURCE_DIR}/*.h")
FILE(GLOB c_files "${CMAKE_CURRENT_SOURCE_DIR}/*.C")
SET(EXECUTABLES ${c_files})
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
SET(EXECUTABLES ${c_files})
SET(SOURCE_FILES ${c_files})

add_subdirectory(DataLib)

# Add all source files
add_executable(alara ${EXECUTABLES})
Copy link
Member

Choose a reason for hiding this comment

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

to match the change above

Suggested change
add_executable(alara ${EXECUTABLES})
add_executable(alara ${SOURCE_FILES})

src/input.C Outdated
@@ -1,7 +1,7 @@
/* $Id: input.C,v 1.14 2007-12-20 22:03:56 wilsonp Exp $ */
#include "alara.h"
#include "input_tokens.h"
#include "dflt_datadir.h"
//#include "dflt_datadir.h"
Copy link
Member

Choose a reason for hiding this comment

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

Have we successfully figured out how to generate this file with CMake as we were doing with autotools?

Copy link
Member

Choose a reason for hiding this comment

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

I think we have put this info in config.h instead (that is current generated from version.h.in). We can probably delete this line.

Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

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

This is looking pretty good. A few additional comments:

  1. I think we should rename version.h.in to config.h.in since it is used to generate config.h. Be sure to update the line in src/CMakeLists.txt as well
  2. We can delete version.h entirely as we no longer use it
  3. I don't think we need the symbolic link from src/DataLib/alara.h to src/alara.h. If something isn't working we probably need to fix it in CMake
    4 .We probably need to add CMake options to set xsdir and nonxsdir so that they can be set properly in config.h

include_directories(${CMAKE_CURRENT_SOURCE_DIR})
configure_file(version.h.in "${CMAKE_CURRENT_SOURCE_DIR}/config.h")

FILE(GLOB h_files "${CMAKE_CURRENT_SOURCE_DIR}/*.h")
Copy link
Member

Choose a reason for hiding this comment

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

You don't use this variable, so probably don't need this line.

src/input.C Outdated
@@ -1,7 +1,7 @@
/* $Id: input.C,v 1.14 2007-12-20 22:03:56 wilsonp Exp $ */
#include "alara.h"
#include "input_tokens.h"
#include "dflt_datadir.h"
//#include "dflt_datadir.h"
Copy link
Member

Choose a reason for hiding this comment

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

I think we have put this info in config.h instead (that is current generated from version.h.in). We can probably delete this line.

@gonuke gonuke changed the base branch from master to main June 23, 2020 16:16
@bradlaufenberg
Copy link
Contributor Author

@gonuke I believe everything is done but before I push just wanted to clarify two things from the latest comments you had made.

  1. Is the symbolic link between src/alara.h and src/DataLib/alara.h the target_link_libraries line in src/CMakeLists.txt ?

  2. Do I set the xsdir and nonxsdir in src/CmakeLists.txt ?

@gonuke
Copy link
Member

gonuke commented Jun 29, 2020

The symbolic link looks like an extra file that appears in the repository. It's actually not directly related to your changes, but I think that your changes make it less necessary.

You should be able to set the value of xsdir and nonxsdir in src/CMakeLists.txt (or equally valid may be CMakeLists.txt) based on the known installation location and relative paths from there.

Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

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

This is getting close but we need to discuss "DFLT_XSDIR" and "DFLT_DATADIR"

@@ -12,4 +11,7 @@ add_subdirectory(DataLib)

# Add all source files
add_executable(alara ${SOURCE_FILES})
target_link_libraries(alara DataLib)

# Set direcotry variable names
Copy link
Member

Choose a reason for hiding this comment

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

typo

Suggested change
# Set direcotry variable names
# Set directory variable names

target_link_libraries(alara DataLib)

# Set direcotry variable names
set(DFLT_XSDIR ${xsdir})
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this will do what it needs to do - we should discuss (see note in Slack)

@@ -0,0 +1,10 @@
# CMake file in the DataLib direcotry within the src directory

FILE(GLOB h_files "${CMAKE_CURRENT_SOURCE_DIR}/*.h")
Copy link
Member

Choose a reason for hiding this comment

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

Not sure that you need h_files defined.

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