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

Feature/lib install target part i #506

Merged
merged 42 commits into from
Jun 25, 2018

Conversation

halfflat
Copy link
Contributor

CMake and build refactoring

  • Use CUDA as first-class language (leading to CMake 3.9 minimum version requirement).

  • Use 'modern CMake' interface libraries for compiler options, include file and library dependency tracking. Interface library targets:

    • arbor-deps: compiler options and library requirements for the libarbor.a static library, as governed by configure-time options and environment.
    • arbor-private-headers: include path for non-installed headers, as required by unit tests and arbor itself.
    • arbor-aux: helper classes and utilities used across tests and examples.
    • ext-json, ext-tclap, ext-tbb, ext-benchmark, ext-sphinx_rtd_theme: externally maintained software that we include (directly or via submodule) in the ext/ subdirectory.
  • Single static library libarbor.a includes all built-in modules and CUDA objects.

  • Simply configuration options:

    • ARB_WITH_TRACE, ARB_AUTORUN_MODCC_ON_CHANGES ARB_SYSTEM_TYPE removed.
    • External modcc is provided by ARB_MODCC configuration option; if provided modcc is still buildable, but is not included in the default target.
    • ARB_PRIVATE_TBBLIB, defaulting to OFF, instructs the build to make TBB from the included submodule.
  • Extend ErrorTarget functionality to provide a dummy target or an error target based on a condition.

  • Generate header version defines and library version variables based on git status and project version, via new script include/git-source-id.

  • All generated binaries now placed in bin/ subdirectory at build.

  • Install targets installs: public headers (incomplete); static library; modcc tool; lmorpho executable; html documentation (examples, tests and validation data are currently not installed).

  • Executable targets have had the .exe suffix removed; unit tests are labelled unit (arbor unit tests), unit-modcc (modcc unit tests), unit-local (distributed tests with local context), unit-mpi (distributed tests with MPI context).

  • More graceful handling of configure-time detection of nrniv, Julia and required Julia modules for validation data generation.

  • Add cmake/FindJulia.cmake, cmake/FindTBB.cmake package finders, and adjust cmake/FindUnwind.cmake to use link library-style properties.

  • Adjust travis script to test unit-local and unit-mpi if appropriate.

  • Simply documentation conf.py.

Source relocation and reorganization

  • All external project sources and files moved to ext/.
  • Source code refactoring to decouple library-using code from the configure-time definitions that govern arbor behaviour: removes conditional code in public headers that depends upon ARB_WITH_X-type definitions at compile time. Affected code is is in the public interfaces for MPI, the threading implementation, and the profiler.
  • Remove util/debug.hpp; split out functionality for pretty-printing from assertion handling.
  • Make FVM cell non-physical voltage check a run-time cell-group parameter.
  • Move spike double buffer implementation to simulation.cpp.
  • Make timer utility wrap POSIX clock_gettime independent of threading configuration.
  • Make mpi_error derive from system_error and follow C++11 system_error semantics.
  • EXPECTS macro replaced by arb_assert macro.
  • JSON dependency removed from libarbor.a and header files: moved to auxiliary library.
  • Publicly visible macros garner an ARB_ prefix as required.
  • Move SWC test file to test/unit directory.
  • Work-in-progress splitting of public from private includes: as a convention not entirely adhered to as yet, private headers within arbor source are included with "", public headers with <>.

Modcc interface changes

  • Expose via --namespace option the functionality that sets the namespace in generated code.
  • Use --profile option to add profiler hooks to generated code; uses public function interface directly rather than PE/PL macros in order to avoid public PE and PL defines.

halfflat and others added 30 commits May 30, 2018 00:03
* Rejig validation cmake logic.
* renaming: tests->test; global_communication->unit-mpi; plus some
others.
* move modcc test data (test.mod) to test source directory.
* CMake stuff for modcc and unit test.
unit tests up, but weird invalid free() and/or address sanitizer blow up
on std::mutex ctor, so something is awry.
Fixes memory corruption in unit tests that create
an `arb::simulator`.
In progress:
* No requirement for ARB_HAVE_MPI defined in user code for headers to work.
* Use system_error interface to wrap MPI error codes/conditions.
* Move mpi::scoped_guard functionality out of lib.
* Distributed tests build against both mpi and local contexts.
* Remove test/test_util.hpp (nearly).
Note: missing warning on configure for missing e.g. neuron
also: address unsigned/signed comparison warning in test.
Need to fix broken clock/timing, but otherwise ok.
Copy link
Member

@bcumming bcumming left a comment

Choose a reason for hiding this comment

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

I have to say, I can't possibly review this properly.
447 files is just too much.
GitHubs tools start breaking too: the "jump to file" drop down doesn't work because there are too many files (just when you need it the most).

CMakeLists.txt Outdated
@@ -1,313 +1,248 @@
cmake_minimum_required(VERSION 3.0)
cmake_minimum_required(VERSION 3.9)
Copy link
Member

Choose a reason for hiding this comment

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

This is a very recent version of CMake to enforce, so you should have a comment along the lines of "because CUDA" to justify it.

CMakeLists.txt Outdated
set_property(CACHE ARB_THREADING_MODEL PROPERTY STRINGS cthread tbb serial )

if(ARB_THREADING_MODEL STREQUAL "tbb")
set(ARB_WITH_TBB TRUE)
Copy link
Member

Choose a reason for hiding this comment

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

missing indent?

Copy link
Member

Choose a reason for hiding this comment

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

Here ARB_WITH_TBB is set, yet in ext/CMakeLists.txt the variable ARB_REQUIRE_TBB is used without being set.
Neither variable is documented in the docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could explain the TBB travis failure, hmm!

CMakeLists.txt Outdated
-Xcudafe --diag_suppress=integer_sign_change
-Xcudafe --diag_suppress=unsigned_compare_with_zero)
# External libraries in `ext` sub-directory: json, tclap and
# (if ARB_REQUIRE_TBB is true) tbb.
Copy link
Member

Choose a reason for hiding this comment

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

this is phrased awkwardly, I would just have tbb on the end of the list.

CMakeLists.txt Outdated
set_property(CACHE ARB_THREADING_MODEL PROPERTY STRINGS cthread tbb serial )

if(ARB_THREADING_MODEL STREQUAL "tbb")
set(ARB_WITH_TBB TRUE)
Copy link
Member

Choose a reason for hiding this comment

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

Here ARB_WITH_TBB is set, yet in ext/CMakeLists.txt the variable ARB_REQUIRE_TBB is used without being set.
Neither variable is documented in the docs.

CMakeLists.txt Outdated
set_property(DIRECTORY APPEND_STRING PROPERTY COMPILE_OPTIONS "${MPI_C_COMPILE_FLAGS}")
endif()
target_compile_options(arbor-deps INTERFACE
$<$<COMPILE_LANGUAGE:CUDA>:--diag_suppress=integer_sign_change
Copy link
Member

Choose a reason for hiding this comment

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

I am no guru of CMake generator expressions (who is? what do they do for fun? why would they be?), but I think that the < need closing >.

Copy link
Contributor Author

@halfflat halfflat Jun 20, 2018

Choose a reason for hiding this comment

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

Yeah, I think this is a combination of sloppy testing and merge-editing. Will revise all the CUDA building again.

CMakeLists.txt Outdated
target_compile_options(arbor-deps INTERFACE
$<$<COMPILE_LANGUAGE:CUDA>:--diag_suppress=integer_sign_change
$<$<COMPILE_LANGUAGE:CUDA>:--diag_suppress=unsigned_compare_with_zero)
target_compile_definitions(arbor-deps ARB_HAVE_GPU)
Copy link
Member

Choose a reason for hiding this comment

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

Sometimes, like here, target_compile_definitions doesn't use INTERFACE, then in others like line 155 where ARB_HAVE_MPI is defined do.
Why?

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 I figured that one out. The question then becomes "if arbor-deps is an interface, why do I have to say INTERFACE every time I interact with it?".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a very good question, Ben! Does anyone else have a question they would like to ask the CMake developers?

if(ARB_VECTORIZE)
set(modcc_simd "-s")
list(APPEND modcc_flags "-s")
Copy link
Member

Choose a reason for hiding this comment

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

Is there a more verbose option like --simd? I would use that here, to make it clearer to someone parsing the command.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, good point, will amend.

)

# I can't believe I have to do this.
Copy link
Member

Choose a reason for hiding this comment

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

This will force the call to your add_custom_command above?
You would think that they had figured out how to build DAGs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, a more informative and less snarky comment is called for.

@@ -0,0 +1,40 @@
file(MAKE_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/arbor)
if(ARB_WITH_ASSERTIONS)
Copy link
Member

Choose a reason for hiding this comment

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

Indentation of if blocks.

endif()

add_library(arbor-public-headers INTERFACE)
target_include_directories(arbor-public-headers INTERFACE
Copy link
Member

Choose a reason for hiding this comment

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

What does this do?
Could all three of these $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}> and friends evaluate to true? Or just one?
I am afraid that the CMake docs are not very helfpful:

The BUILD_INTERFACE expression wraps requirements which are only used
when consumed from a target in the same buildsystem, or when consumed from
a target exported to the build directory using the export() command

Copy link
Member

Choose a reason for hiding this comment

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

"Modern CMake" is even harder to understand than "old skool CMake". Maybe a comment here to explain what decision is being made?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I'll try and come up with a comment that properly balances concision, informativeness, and profanity.

target_link_libraries(modcc LINK_PUBLIC compiler)
set_target_properties(modcc libmodcc PROPERTIES EXCLUDE_FROM_ALL ${ARB_WITH_EXTERNAL_MODCC})

install(TARGETS modcc RUNTIME DESTINATION ${CMAKE_INSTALL_LIBEXECDIR})
Copy link
Member

Choose a reason for hiding this comment

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

This is installed in CMAKE_INSTALL_LIBEXECDIR, which on my system is <prefix>/libexe.
lmorpho is installed in CMAKE_INSTALL_BINDIR, i.e. <prefix>/bin.
I would rather see modcc get installed in bin, what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Put it in libexec on the grounds that it was a tool used to build the library, rather than a user executable, but on reflection this was wrong thinking.

If modcc were only ever used by libarbor — say to build mechanisms from NMODL at run time — then it would go here. But it is in fact a user-tool, and should go in bin as you suggest.

@@ -0,0 +1,110 @@
#pragma once
Copy link
Member

Choose a reason for hiding this comment

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

This header is public, however it is only used in non-public code. And only in non-public cpp files at that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mpi_error should be able to be consumed though in user code, i.e. one should be able to write a catch (mpi_error& e) clause in an application.

* Work around CUDA vs FindThreads issue.
* Always build device memory wrappers, but fail at run-time if no CUDA support.
* Unify cuda runtime wrappers in memory utilities.
* Remove KNL memory wrappers.
* Add extra required synchronization for K80 to GPU stack.
* Move top level mechanism headers to public.
* Move fvm_types to public, splitting out threshold_crossing type.
* Remove arb::config.
* Pimplize thread_private_spike_store to remove threading model define header dependency.
* Add threading model independent query functions thread_count() and thread_implementation().
* Remove vestigial threading timer code.
* Resolve TBB library build and install issues (private TBB build will now install static tbb libs too).
* Add some defines to version.h that capture configure-time features.
* Address some CMakeLists.txt indenting and commenting issues.
* Resolve threading header define dependencies for unit tests, via version.h macro tests.
* Resolve duplicate test and name issues in combined CUDA and CPU unit test.
@halfflat
Copy link
Contributor Author

CMake error is something they addressed in a later version of CMake. I'll attempt a workaround tomorrow (or we can bump the minimum CMake version).

@bcumming
Copy link
Member

bcumming commented Jun 22, 2018

I would prefer that you finish this PR as is: i.e. get it passing tests and installing "something", even if the installed target is not very useful.
Then further pull requests to get the installed target into shape.
There are a lot of changes like wholesale renaming that are going on, that I would like to discuss, but I can't reasonably do so if are lost in the noise of 490 change files.

Copy link
Member

@bcumming bcumming left a comment

Choose a reason for hiding this comment

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

Some small gripes and comments.

This is already an omnibus bill PR.

Please don't make any more refactoriings beyond what is strictly required to get the tests passing and something installed.

Then we can have seperate PRs for (a) getting a usefull installation (b) refactorings that Sam is itching to make ;)

CMakeLists.txt Outdated
target_compile_definitions(arbor-deps ARB_HAVE_GPU)
add_compile_options(
"$<$<COMPILE_LANGUAGE:CUDA>:-Xcudafe=--diag_suppress=integer_sign_change>"
"$<$<COMPILE_LANGUAGE:CUDA>:-Xcudafe=--diag_suppress=unsigned_compare_with_zero>")
Copy link
Member

Choose a reason for hiding this comment

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

A question about CMake, clarifying what we have here. Tell me if what I say below is correct....

Compile options are set using add_compile_options. We are using two compilers here, say gcc for the C++ and nvcc for the CUDA, and there is the one add_compile_options interface for setting flags to pass to both of them.

So these $<COMPILE_LANGUAGE:CUDA> are not expanded immediately here with the call to add_compile_options, instead they are expanded later on each invocation of the compiler, making the choice of whether to add a flag at "dispatch time"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nearly correct: they get resolved when they make the Makefiles.

@@ -12,24 +12,38 @@ target_include_directories(ext-tclap INTERFACE tclap/include)
# Alias system TBB or build locally and export that, according
# to ARB_PRIVATE_TBBLIB setting.

if(ARB_REQUIRE_TBB)
find_package(TBB)
if(ARB_PRIVATE_TBBLIB OR NOT TBB_FOUND)
Copy link
Member

Choose a reason for hiding this comment

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

Will this compile TBB if the users has tried to provided their own version, and there was an error (and hence TBB_FOUND is false).

If so, shouldn't that be an error, because: (a) the user is expecting their TBB to be used; (b) there was an error with the information they provided?

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''m not really concerned about the exact semantics here: there is more than one 'right way'. If you'd prefer that users must explicitly set the ARB_PRIVATE_TBBLIB option to use the included TBB, then that's also cool.

Copy link
Member

Choose a reason for hiding this comment

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

I thought that the approach you were taking was to explicitly set ARB_PRIVATE_TBBLIB

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, the logic there kinda captures what I thought would be useful: if TBB is requested and we can't find it, or the user requests the in-source one, then we build the in-source one. I'm totally happy with the alternative, of it being an error if TBB isn't found and the explicit request is not made. Do you have a preference?

Copy link
Member

Choose a reason for hiding this comment

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

What you have is fine then! I thought that your preference was for the in-source TBB to be strictly opt-in.
I actually prefer this approach to making it strictly opt-in, because I like using the in-source version, and don't feel like explicitly asking for it!

add_library(ext-tbb ALIAS TBB::tbb)
endif()

if(ARB_WITH_TBB)
if(ARB_PRIVATE_TBBLIB)
Copy link
Member

Choose a reason for hiding this comment

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

ARB_PRIVATE_TBBLIB still isn't set as an option anywhere. It must be provided by the user on the command line.

For correctness, and self-documentation, it should be an explicit option that defaults to OFF>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It already is an explicit option that defaults to OFF :)

Copy link
Member

Choose a reason for hiding this comment

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

OK. That didn't show up when I did a search of the changeset.

@@ -218,7 +218,7 @@ std::string emit_cuda_cu_source(const Module& module_, const printer_options& op
"#include <" << arb_private_header_prefix() << "backends/event.hpp>\n"
"#include <" << arb_private_header_prefix() << "backends/multi_event_stream_state.hpp>\n"
"#include <" << arb_private_header_prefix() << "backends/gpu/cuda_common.hpp>\n"
"#include <" << arb_private_header_prefix() << "backends/gpu/math.hpp>\n"
"#include <" << arb_private_header_prefix() << "backends/gpu/math_cu.hpp>\n"
Copy link
Member

Choose a reason for hiding this comment

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

why the longer name math_cu.hpp. It is in the gpu path, which is sufficient, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It resolved some ambiguities in header paths.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also it's clearer: having multiple headers called "math.hpp" is confusing.

Copy link
Member

@bcumming bcumming Jun 22, 2018

Choose a reason for hiding this comment

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

It just fells like redundant renaming. I would also look for X/math.hpp instead of X/math_x.hpp where x is restating X.
Not a big deal though!

#include "util/index_into.hpp"
#include "util/meta.hpp"

// (Pending abstraction of threading interface)
Copy link
Member

Choose a reason for hiding this comment

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

How about indenting some of these includes? The pattern of "always align preprocessor to the left gutter" makes for ugly code in my opinion.

#include <arbor/version.hpp>
#if defined(ARB_TBB_ENABLED)
    #include "threading/tbb.hpp"
#elif defined(ARB_CTHREAD_ENABLED)
    #include "threading/cthread.hpp"
#else
    #include "threading/serial.hpp"
#endif

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 generally prefer not to indent directives unless it's really necessary for clarity, as the scoping rules for them are completely different from the scoping rules of the source code around them.

Copy link
Member

Choose a reason for hiding this comment

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

It isn't about scoping, it is about making it easier to read and understand.
The additional clarity overides any potential scoping confusion for a proficient C++ developer.
There is an argument to be made, particularly at the top of a file like this, that there is a sort of "preprocessor scoping".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Consistency is also desirable, and having one convention for when it is just directives, and another when it is a mix of directives and code, is also unappealing. In short sequences like this I can't see it being a big gain in readability to indent them. It matters little in this instance, but I don't want to concede that indenting these guys is necessarily desirable.

Copy link
Member

Choose a reason for hiding this comment

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

We discussed this with @vkarak 2 years ago when working on our coding standards.
After much bloodletting a discussion the decision was to indent preprocessor directives, though we never wrote it down.

Taking the original src/threading/threading.hpp file as a reference:

#if defined(ARB_HAVE_TBB)
    #include "tbb.hpp"
#elif defined(ARB_HAVE_CTHREAD)
    #include "cthread.hpp"
#else
    #define ARB_HAVE_SERIAL
    #include "serial.hpp"
#endif

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pop it in the coding standards.

Copy link
Member

Choose a reason for hiding this comment

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

We should copy those standards from the old cell_algorithms wiki over the the new wiki, where we might update them from time to time.

@@ -158,7 +159,7 @@ TEST(lif_cell_group, spikes) {
path_recipe recipe(2, 1000, 0.1);

hw::node_info nd;
nd.num_cpu_cores = threading::num_threads();
nd.num_cpu_cores = arb::thread_count();
Copy link
Member

Choose a reason for hiding this comment

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

To minimise the surface area of this PR, lI would avoid renamings likenum_threads to thread_count.
Also, num_threads is a better variable name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You know, I hate those num_names. But yeah, it doesn't matter what it is called. It's not replacing the underlying call to the thread implementation, but wrapping it.

Copy link
Member

Choose a reason for hiding this comment

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

I know you don't like them, but that is the name we have, and wrapping it with a different name is renaming it ;)

#include <mc_cell_group.hpp>
#include <arbor/common_types.hpp>

#include "backends.hpp"
Copy link
Member

Choose a reason for hiding this comment

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

Why are we using "backends.hpp" instead of <backends.hpp>. These files are not in the current path, so this is a bit misleading.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The convention I'm trying to adhere to is that public headers get the angle brackets, and private ones get the quotes, just to help keep things straight. We could e.g. rename the arbor source directory back to src, or maybe lib, and then qualify the private headers with that directory prefix. In terms of confusing header paths though, it was already a huge mess, and I think this is making it less so.

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 that the convention of using "" is for files relative to the current path, and <> for relative to paths in -I and -isystem. This is why it is confusing.

I don't think you have to rename the arbor path. I think that it is clear enough that #include <arbor/foo.hpp> and #include <foo.hpp> are referring to public and private headers respectively.

Copy link
Member

Choose a reason for hiding this comment

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

We have discussed this in the past, and moved to using <> in the unit tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we see <foo/bar.hpp> it's not at all clear that that is referring to a private foo/bar.hpp or a for a bar.hpp belonging to a system-installed package called foo. I am not saying that that is a terrible thing, only that it doesn't give the clarity that you desire.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also note that when the public/private split is complete, the number of private headers used by the tests will be very much reduced.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And (hopefully) lastly, this distinction between <> and "" for public/private is quite common.

Copy link
Member

Choose a reason for hiding this comment

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

How about a new macro include_private_header(foo.hpp)?

Joking aside, <src/arbor/foo.hpp> is a bit long winded, and we probably shouldn't change the directory structure of the source code just to accomodate the internal use of headers in our unit tests.

Thinking about it, the reason this makes me uncomfortable is that the quoting is being use as shorthand for "any header not part of the public API", when it is usually used as shorthand for "headers relative to this path".

Copy link
Member

Choose a reason for hiding this comment

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

We could have grouping rules for the unit tests, e.g.

#include <iostream>
#include <numeric>

#include <arbor/common_types.hpp>
#include <arbor/simulation.hpp>

#include <foo.hpp>
#include <internal.hpp>
...

There will still be a lot of inclusion of private headers after the refactoring, because we will have tests for all the private functionality, right?

Copy link
Member

Choose a reason for hiding this comment

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

OK, I have been doing some reading, and it would appear that using quotes for project files is a common practice.
e.g. https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes

Let's argue about this in front of a black board.

@halfflat
Copy link
Contributor Author

The refactorings are unfortunately necessary if we want to keep configuration-based code out of header files that are included by other code which does not (and should not) set the same configuration-based defines.

@bcumming
Copy link
Member

How about finishing this PR up with an imperfect setup. Then in subsequent PRs I will be able to follow the steps taken to keep configuration based code out of header files.
It doesn't have to be perfect at first.

@halfflat
Copy link
Contributor Author

As far as I can tell from this point, indenting and name wrangling aside, the only remaining issue is working around CMake brokenness in 3.9.

halfflat and others added 9 commits June 22, 2018 14:35
* Accumulate public and private dependencies for arbor at top level CMakeLists.txt based on configuration options. This means consumers (in CMake land) will inherit the same include path to MPI and TBB that arbor used.
* Use wrap_mpi.h where required.
* Add more random subdirectories to FindTBB library search.
…at/arbor into feature/lib-install-target-part-I
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants