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

Add BUILD_EXE as cmake option #717

Merged
merged 10 commits into from
Feb 15, 2021
Merged

Add BUILD_EXE as cmake option #717

merged 10 commits into from
Feb 15, 2021

Conversation

bam241
Copy link
Member

@bam241 bam241 commented Dec 21, 2020

This should allow to build dagmc libs (shared and.or static) without the exe.

endmacro ()

# Install a unit test
macro (dagmc_install_test test_name ext)
message(STATUS "Building unit tests: ${test_name}")
if (NOT BUILD_EXEC)
Copy link
Member

Choose a reason for hiding this comment

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

This should be BUILD_EXE not BUILD_EXEC

@bam241
Copy link
Member Author

bam241 commented Dec 21, 2020

I am now missing the includes :( in the install folder....

@ljacobson64
Copy link
Member

I think that's a problem in the dagmc_install_library macro, not something you did. The shared library section starts like this:

  if (BUILD_SHARED_LIBS)
    add_library(${lib_name}-shared SHARED ${SRC_FILES})
      set_target_properties(${lib_name}-shared
        PROPERTIES OUTPUT_NAME ${lib_name}
        PUBLIC_HEADER "${PUB_HEADERS}")

while the static library section starts like this:

  if (BUILD_STATIC_LIBS)
    add_library(${lib_name}-static STATIC ${SRC_FILES})
    set_target_properties(${lib_name}-static
      PROPERTIES OUTPUT_NAME ${lib_name})

The static library section doesn't set the public headers as part of the target properties.

@bam241
Copy link
Member Author

bam241 commented Dec 21, 2020

included the static header install fix in this PR....

Copy link
Member

@pshriwise pshriwise left a comment

Choose a reason for hiding this comment

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

Just a few small items from me here.

@@ -66,6 +66,7 @@ macro (dagmc_setup_options)
option(BUILD_SHARED_LIBS "Build shared libraries" ON)
option(BUILD_STATIC_LIBS "Build static libraries" ON)

option(BUILD_EXE "Build DAGMC executables" ON)
Copy link
Member

Choose a reason for hiding this comment

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

Let's indent the comment string to match the other options.

cmake/DAGMC_macros.cmake Outdated Show resolved Hide resolved
news/PR-0717.rst Outdated Show resolved Hide resolved
Co-authored-by: Patrick Shriwise <pshriwise@gmail.com>
@bam241
Copy link
Member Author

bam241 commented Jan 20, 2021

@pshriwise is there some still some undressed comments in here ?

@ljacobson64
Copy link
Member

@bam241 can you remind me what the use case is for building without executables?

@bam241
Copy link
Member Author

bam241 commented Jan 21, 2021

@bam241 can you remind me what the use case is for building without executables?

The plugin.

Copy link
Member

@pshriwise pshriwise left a comment

Choose a reason for hiding this comment

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

All my comments were addressed. Looks good @bam241!

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.

LGTM - thanks @bam241

@gonuke gonuke merged commit 80c2a0e into svalinn:develop Feb 15, 2021
@bam241 bam241 deleted the build_exe branch November 28, 2024 14:40
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.

4 participants