Navigation Menu

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

Include entire src tree in multiconfig projects: #2707

Closed
wants to merge 1 commit into from

Conversation

ximinez
Copy link
Collaborator

@ximinez ximinez commented Oct 8, 2018

  • For example Visual Studio, XCode. This will allow easily working with
    any file in the IDE.
  • Also ignore the file created by Visual Studio when using cmake
    integration.

Note: This change is based off of #2703 to increase the chances of the Jenkins build passing. Please ignore the "Add dependency for NuDB ExternalProject" commit.

Assignees:

@ripplelabs-jenkins
Copy link
Collaborator

ripplelabs-jenkins commented Oct 8, 2018

Jenkins Build Summary

Built from this commit

Built at 20181010 - 15:58:54

Test Results

Build Type Log Result Status
rpm logfile no test results, t: n/a FAIL 🔴
msvc.Debug logfile 1034 cases, 0 failed, t: 531s PASS ✅
msvc.Debug,
NINJA_BUILD=true
logfile 1034 cases, 0 failed, t: 551s PASS ✅
gcc.Debug
-Dcoverage=ON
logfile 1037 cases, 0 failed, t: 3m51s PASS ✅
docs logfile 1 cases, 0 failed, t: 0m1s PASS ✅
gcc.Release
-Dassert=ON,
MANUAL_TESTS=true
logfile 1101 cases, 0 failed, t: 6m7s PASS ✅
clang.Debug logfile 1037 cases, 0 failed, t: 3m12s PASS ✅
clang.Debug
-Dunity=OFF
logfile 1037 cases, 0 failed, t: 5m22s PASS ✅
gcc.Debug logfile 1037 cases, 0 failed, t: 3m22s PASS ✅
gcc.Debug
-Dunity=OFF
logfile 1037 cases, 0 failed, t: 6m23s PASS ✅
clang.Release
-Dassert=ON
logfile 1037 cases, 0 failed, t: 5m40s PASS ✅
msvc.Debug
-Dunity=OFF
logfile 1034 cases, 0 failed, t: 1169s PASS ✅
gcc.Debug
-Dstatic=OFF
logfile 1037 cases, 0 failed, t: 3m24s PASS ✅
gcc.Release
-Dassert=ON
logfile 1037 cases, 0 failed, t: 6m4s PASS ✅
msvc.Release logfile 1034 cases, 0 failed, t: 394s PASS ✅
gcc.Debug
-Dstatic=OFF -DBUILD_SHARED_LIBS=ON
logfile 1037 cases, 0 failed, t: 3m24s PASS ✅
gcc.Debug,
NINJA_BUILD=true
logfile 1037 cases, 0 failed, t: 3m2s PASS ✅
clang.Debug
-Dunity=OFF -Dsan=address,
PARALLEL_TESTS=false,
DEBUGGER=false
logfile 1037 cases, 0 failed, t: 2m55s PASS ✅
clang.Debug
-Dunity=OFF -Dsan=undefined,
PARALLEL_TESTS=false
logfile 1037 cases, 0 failed, t: 12m0s PASS ✅

CMakeLists.txt Outdated
@@ -2354,6 +2364,7 @@ if (is_multiconfig)
group_sources(src)
group_sources(docs)
group_sources(Builds)
group_sources(.nih_c)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you might want to group_sources(${nih_cache_path}) instead, which gives just the cache portion for the relevant compiler/generator combo. That said, on xcode this ends-up only including the files which were gotten via FetchContent on the first run (when this cache is clean). On subsequent runs, it's very likely this will contain the other ExternalProject files as well - just something to be aware of I think.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried this (and a couple of relative path variations). Because of the way that group_sources is implemented, it doesn't work as-is. Also, without it, if you've got multiple config types (eg MSVC and ninja) then the other files will still be at the top of the tree (the files stuck at the top of the tree is why I added this in the first place).

Now the bigger issue is that the only reason (as far as I can tell) that .nih_c is relevant is because we search for all .md files in the repo, and there are several under .nih_c. I think the better way to do this is to only do Builds/*.md docs/*.md src/*.md (which will still be relevant accounting for your suggestion below).

CMakeLists.txt Outdated
@@ -2337,7 +2338,16 @@ endif ()

if (is_multiconfig)
# Rippled headers. Only useful for IDEs.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this comment doesn't apply any more and needs updating - maybe something like "This code finds all source files in the src subdirectory for inclusion in the IDE file tree as non-compiled sources. Since this file list will have some overlap with files we have already added to our targets to be compiled, we explicitly remove any of these target source files from this list."

Copy link
Collaborator

@scottschurr scottschurr left a comment

Choose a reason for hiding this comment

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

👍 Looks like it correctly builds an Xcode project for me. Also debug and unity command line builds are working for me on mac OS.

@scottschurr
Copy link
Collaborator

👍 mac OS Xcode and command line builds are still working for me with the latest changes.

@ximinez ximinez added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Oct 10, 2018
* For example Visual Studio, XCode. This will allow easily working with
  any file in the IDE.
* Also ignore the file created by Visual Studio when using cmake
  integration.
* Use conditional for unity/nounity sources (h/t @mellery451)
@seelabs
Copy link
Collaborator

seelabs commented Oct 10, 2018

In 1.2.0-b3

@seelabs seelabs closed this Oct 10, 2018
@ximinez ximinez deleted the cmake-file-tree branch October 10, 2018 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants