Externalize ReactiveStreams.h

[benjchristensen]

• Now have a dependency on https://github.com/ReactiveSocket/reactive-streams-cpp which can be installed into /usr/local/include
• SmartPointers etc stay here, now in /streams/ as they are an implementation details, not part of the interface definition
• We can eventually pull out the streams utilities if we create a concrete implementation library (such as ReactiveX-cpp/RxCpp) that has a type implementing Reactive-Streams

[benjchristensen]

Will figure out what's wrong a little later. Off to meetings right now.

[benjchristensen]

A permissions problem of some kind:

CMake Error at cmake_install.cmake:36 (file):
  file INSTALL cannot copy file
  "/home/travis/build/ReactiveSocket/reactivesocket-cpp/working/reactive-streams-cpp/include/ReactiveStreams.h"
  to "/usr/local/include/ReactiveStreams.h".

[ghost]

SmartPointers etc stay here, now in /streams/ as they are an implementation details, not part of the interface definition

I think this should change, per #11. Better now than later.

[ghost]

Sudo?

[benjchristensen]

Sudo?

Yes :-) I just had to go to another meeting so haven't fixed this yet.

[benjchristensen]

sudo fixed it.

[benjchristensen]

The commits in this should be squashed when merging. The messages of all but the first commit can be deleted when squashing.

[ghost]

That could lead to nasty conflicts. Can we add include paths in such a way that you still have to write reactive-streams-cpp? I.e. #include <reactive-streams-cpp/ReactiveStreams.h>.

[benjchristensen]

Conflicts with what? Another library installing /usr/local/include/ReactiveStreams.h? Putting it inside a folder doesn't change the ability to conflict.

[ghost]

Nope. Conflict with another library that has a header called ReactiveStreams.h anywhere on the include path (dozens locations, at best). Those are rather subtle bugs where the compiler picked up different header than the one you think it did. Obviously, if those two headers differed between parts of a build process (e.g. because one file included stuff using "" syntax and the other with <> syntax), everything could be fine until you run the binary... To make matters worse, you can't depend on the order of folders on include path, as the search order is not defined in the standard http://en.cppreference.com/w/cpp/preprocessor/include.

Look at how other libraries install headers -- usually (always?) in a subdirectory named after that library: folly, event2, wangle, boost, etc. Whatever you put in /usr/local/include/ will be picked up by all C++ builds on that machine...

[benjchristensen]

Yup, understand all of that. While querying this the answer I got was that if we only have a single header file, we don't need a subfolder, which is what many libs in /usr/local/include do.

A subfolder named "event2" does not prevent conflicts either when it is such a generic name if something else chooses to use "event2" and have "event2/event.h". What if some other project decided to have an "event2/event.h"? Well, it would collide.

Right now in my include folder I see this: zip.h, snappy.h (from https://github.com/google/snappy), event.h, event2/event.h, etc. All without folders, and very generic names.

Even glog which is in a folder, it has /glog/logging.h ... that doesn't prevent conflicts between versions of glog.

In other words, I don't see how adding a folder matters unless (a) the filename itself is too generic (such as event.h), or (b) multiple header files are contained within that folder and we want them packaged (such as with boost, wangle, and folly).

So why would any project be creating ReactiveStreams.h unless they were trying to implement ReactiveStreams? And if they are doing that, they should be using the authoritative one, which we are trying to create.

Now, we can put it in reactive-streams/ReactiveStreams.h, but I don't buy that it prevents collision, as there is no global namespacing, and anyone creating a fork or competing solution can easily choose these same names if emulating the Reactive-Streams JVM spec.

[ghost]

Sure one can always create a collision.

I hope you agree that when someone decides to create a fork, it would be much if one only had to change the project name instead of renaming all public headers (well, only one, for now).

If your point is that there is no rock-solid way of generating unique names without a single source of truth, then I do agree. There are heuristics that many C++ projects try to follow to make collisions less likely. <project name>/<header file> is the one I've seen most often. If project names of two different projects are the same then ¯_(ツ)_/¯.

[lehecka]

/reactive-streams/ReactiveStreams.h is very much FB style
/reactive-streams/reactive_streams.h
or
/reactive-streams/reactive_streams would be more standard library style

/ReactiveStreams/ReactiveStreams.h is like Microsoft style.

My preference is:

1. FB style because the rest of the code is written with FB style.
2. STD lib style as this also broadly used style for common libraries
3. MS style.

[benjchristensen]

reactive-streams-cpp lives outside the FB codebase so I don't think that is a strong reason to pick a convention if it is primarily an FB one.

[lehecka]

That is true. And I am fine if we pick std naming for common libraries. I was just trying to say that the classes, the methods, the members and everything else inside was written to conform FB style. If we pick STD lib naming (like STL, boost, ...) then we should do renaming of the classes, etc. to have a consistent style.

[benjchristensen]

Also, it kind of bothers me to mix two styles side-by-side :-)

[benjchristensen]

I'm okay with classes staying camel case. I'm only referring to the project name and header file name for this project.

But it's not a strong opinion.

[lehecka]

Are we mixing 2 styles already? Did I miss that? I thought we have been using FB style so far. And honestly the FB style is not bad at all. I think it is very similar to google style and other github libs. There are only very few things where we differ.

[benjchristensen]

I'll open a new pull request after updating the naming convention to /reactive-streams/ReactiveStreams.h

[benjchristensen]

New pull request: #50