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

In source build failure warning #3082

Merged
merged 3 commits into from
Jun 19, 2020

Conversation

hippo91
Copy link
Contributor

@hippo91 hippo91 commented Jun 6, 2020

This PR is an answer to #3081
It just adds a FATAL_ERROR in message with explanation in case of in-source build.

@dalg24-jenkins
Copy link
Collaborator

Can one of the admins verify this patch?

@codecov-commenter
Copy link

codecov-commenter commented Jun 6, 2020

Codecov Report

Merging #3082 into develop will decrease coverage by 0.0%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           develop   #3082     +/-   ##
=========================================
- Coverage     85.6%   85.6%   -0.1%     
=========================================
  Files          122     122             
  Lines        10391   10391             
=========================================
- Hits          8905    8904      -1     
- Misses        1486    1487      +1     
Flag Coverage Δ
#clang 76.1% <ø> (-0.1%) ⬇️
#gcc 86.4% <ø> (-0.1%) ⬇️
Impacted Files Coverage Δ
core/src/impl/Kokkos_HostThreadTeam.cpp 96.0% <0.0%> (-0.8%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 605ab03...481a32a. Read the comment docs.

@dalg24
Copy link
Member

dalg24 commented Jun 8, 2020

OK to test

@dalg24 dalg24 requested a review from jjwilke June 8, 2020 04:57
Copy link

@jjwilke jjwilke left a comment

Choose a reason for hiding this comment

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

Is it redundant to say "FATAL

CMakeLists.txt Outdated
@@ -1,4 +1,10 @@

# Disable in-source builds to prevent source tree corruption.
if( "${CMAKE_SOURCE_DIR}" STREQUAL "${CMAKE_BINARY_DIR}" )
message( FATAL_ERROR "FATAL: In-source builds are not allowed.
Copy link

Choose a reason for hiding this comment

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

I would do this as a single-line string and let CMake do the wrapping. This prints weirdly on my terminal.
Maybe redundant to say "FATAL"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On my terminal, FATAL appears only once. Thus in my case, i don't see any redundancy:

CMake Error at CMakeLists.txt:4 (message):
  FATAL: In-source builds are not allowed.  You should create a separate
  directory for build files.


-- Configuring incomplete, errors occurred!

if( "${CMAKE_SOURCE_DIR}" STREQUAL "${CMAKE_BINARY_DIR}" )
message( FATAL_ERROR "FATAL: In-source builds are not allowed.
You should create a separate directory for build files." )
endif()
Copy link

Choose a reason for hiding this comment

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

I actually don't care about case anymore, but in the past we had wanted to follow Trilinos use of upper case. @dalg24

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't find any reference to Trilinos cmake coding style. Could you explain a little further?

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 this is fine. Trilinos generally makes all function calls to cmake all upper case. But I don't really care either. Actually I think that it would be nicer to have all function calls lower case. Makes it easier to see most of our variables since they are mostly upper case.

@crtrott crtrott merged commit d69d1b6 into kokkos:develop Jun 19, 2020
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.

None yet

6 participants