Support build on Windows with MSVC 15.7 (#85) #93

mdouaihy · 2018-06-11T21:15:05Z

Export Symbols
Add Runtime targets to the install steps. On windows, static libraries are very tricky. shared library is needed.
Add some MSVC compile options (_SCL_SECURE_NO_WARNINGS). This warning warns about unsafe methods or usage of the stl.
Add appveyor build file. Credits for @hypnoce
Add output directory and ignore build folder in git

* Export Symbols * Add Runtime targets to the install steps * Fix failing unit test under Windows * Add some MSVC compile options * Add appveyor build file * add output directory and ignore build folder in git

rnburn · 2018-06-11T21:16:26Z

Thanks @mdouaihy. I'll get to reviewing this soon.

rnburn · 2018-06-12T15:20:20Z

include/opentracing/dynamic_load.h

+OPEN_TRACING_EXPORT int OPEN_TRACING_ATT
+    OpenTracingMakeTracerFactory(
+    const char* opentracing_version, const void** error_category,
+    void** tracer_factory);


OPEN_TRACING_ATT doesn't expand to anything on windows, right?

Suppose I have two tracing libraries libTracerA, libTracerB, and then I link them both into my app with the intention of switching between them based on a config -- would that give a multiple definition error on windows because both would define OpenTracingMakeTracerFactory ?

The weak attribute was there to support that scenario. Could we maybe use __declspec(selectany) for this?

Indeed, OPEN_TRACING_ATT does not expand to anything on Windows.

if we take your example, why would I link directly with the two tracers? I'd rather use the dynamic loading mechanism to load the implementation, no?

I would prefer people use the dynamic loading interface for that as well. But there are a few reasons why they might want to link directly, and it should be a scenario that we support if possible:

You might want all your dependencies linked into the same file so that it's easier to deploy.

You may want to use tracer-specific options that aren't available through the JSON configuration. For example, customized logging or transports where you need to pass in a callback in the options.

Envoy, for example, is an application that links in multiple tracers like this and uses some customizations that wouldn't be available through a JSON configuration interface.

ok I understand now your point.

Unfortunately, weak symbols are not exposed by MSVC compiler. selectany works only on data items and not on methods.
https://docs.microsoft.com/en-us/cpp/cpp/selectany

However, we can use the /FORCE:MULTIPLE link flag to force the link of the binary even if multiple definitions of a symbol exists
https://docs.microsoft.com/en-us/cpp/build/reference/force-force-file-output

Also, on windows, using dynamic linking, having the same symbols is not a problem.
This last point is important. If we take again the example, linking TracerA and TracerB statically with OT will cause duplication of the symbols of OT, among them the Global Tracer, which is not what we really want.

What do you think?

Could you use selectany with a data variable that's a function pointer?

If we take again the example, linking TracerA and TracerB statically with OT will cause duplication of the symbols of OT, among them the Global Tracer, which is not what we really want.

Static libraries are usually built as thin archives (i.e. without the dependent libraries linked in), so there wouldn't be duplication of the OT symbols since OT would be linked in by the app along with the tracing libraries.

rnburn · 2018-06-12T15:24:04Z

include/opentracing/symbols.h

+
+#ifdef _MSC_VER
+
+#define OPEN_TRACING_EXPORT __declspec(dllexport)


Could we s/OPEN_TRACING/OPENTRACING/ to keep the naming convention consistent?

rnburn · 2018-06-12T15:25:10Z

src/dynamic_load_windows.cpp

+      reinterpret_cast<decltype(OpenTracingMakeTracerFactory)*>(
+          GetProcAddress(handle, "OpenTracingMakeTracerFactory"));
+  if (make_tracer_factory == nullptr) {
+    error_message = "An error occurred whike looking up for OpenTracingMakeTracerFactory : " + GetLastError();


An error occurred whike

typo

mdouaihy · 2018-06-14T15:16:51Z

@rnburn, changes done as per our discussion.

rnburn · 2018-06-14T17:25:49Z

include/opentracing/symbols.h

+#endif // OPENTRACING_EXPORTS
+#endif  // OPENTRACING_STATIC
+
+#define OPENTRACING_ATT 


The OPENTRACING_ATT macro isn't used anywhere now, right? Can we remove?

yes. removed in the latest commit.

rnburn · 2018-06-14T17:30:11Z

src/dynamic_load_unix.cpp

          dlsym(handle, "OpenTracingMakeTracerFactory"));
-  if (make_tracer_factory == nullptr) {
+  if (make_tracer_factory == nullptr || *make_tracer_factory == nullptr) {
    error_message = dlerror();


If *make_tracer_factory == nullptr, won't this need a different error message since dlerror wouldn't return anything?

indeed. you're totally right.

…plementations.

rnburn · 2018-06-14T17:38:15Z

src/dynamic_load_windows.cpp

+          GetProcAddress(handle, "OpenTracingMakeTracerFactory"));
+
+  if (make_tracer_factory == nullptr) {
+    error_message = "An error occurred while looking up for OpenTracingMakeTracerFactory : " + GetLastError();


This StackOverflow question says that we need to use FormatMessage along with GetLastError to get a useful message.

Good idea. Change done. an example

Failed to load tracer library. An error occurred: The specified module could not be found.

rnburn · 2018-06-14T18:23:05Z

Could we add some brief instructions to README.md for how to install on windows? The current instructions, I think, are unix/linux-specific.

mdouaihy · 2018-06-14T18:28:52Z

sure. Very good idea.

rnburn · 2018-06-14T18:29:47Z

@mdouaihy - Why'd you close?

mdouaihy · 2018-06-14T18:31:01Z

I didnt do that on purpose. I might have pressed on the wrong button

mdouaihy · 2018-06-14T18:50:09Z

Instructions to generate the Visual Studio solution, to run the build and the tests are added now. Both Release and Debug target are covered.

rnburn · 2018-06-14T18:57:03Z

include/opentracing/dynamic_load.h

+                                                                            \
+  int __attribute((weak))                                                   \
+      X(const char* opentracing_version, const void** error_category,       \
+        void** tracer_factory);                                             \


I don't think we need these declarations:

+__declspec(dllexport) int X(const char* opentracing_version, \ + const void** error_category, \ + void** tracer_factory);

+ int __attribute((weak)) \ + X(const char* opentracing_version, const void** error_category, \ + void** tracer_factory);

All that needs to be exposed is the variable OpenTracingMakeTracerFactory. X can be a static function in the translation unit.

This declaration is useful so that

OPENTRACING_DECLARE_IMPL_FACTORY(OpenTracingMakeTracerFactoryFct);

can be declared anywhere in the translation unit.

change done.

rnburn · 2018-06-14T20:43:47Z

mocktracer/src/dynamic_load.cpp

@@ -4,7 +4,7 @@
 #include <cstring>
 #include <exception>

-int OpenTracingMakeTracerFactory(const char* opentracing_version,
+int OpenTracingMakeTracerFactoryFct(const char* opentracing_version,


Can we make OpenTracingMakeTracerFactoryFct static?

…ic loading.

rnburn · 2018-06-14T21:04:25Z

@mdouaihy - This looks good. Was there anything else you wanted to add or should I go ahead and merge in?

mdouaihy · 2018-06-14T21:14:53Z

@rnburn, nothing to add from my side for the moment. Once you merge, please create a version that we update on Hunter to finalize the Jaeger windows build.

Thank you for your time.

rnburn · 2018-06-14T21:21:17Z

@mdouaihy - I have another PR (#91) that also makes some changes to the dynamic loading API. I'll update it with the changes here, but I'd like to merge that before making the next release.

Support build on Windows with MSVC 15.7 (#85)

1f18c88

* Export Symbols * Add Runtime targets to the install steps * Fix failing unit test under Windows * Add some MSVC compile options * Add appveyor build file * add output directory and ignore build folder in git

Exclude dynamic_load_windows.cpp from bazel build.

b762fa4

rnburn reviewed Jun 12, 2018

View reviewed changes

rnburn reviewed Jun 14, 2018

View reviewed changes

Handlle multiple symbols problem when linking directly with tracer im…

51fcc6d

…plementations.

rnburn reviewed Jun 14, 2018

View reviewed changes

Format the error message when doing Windows calls.

71bccd9

mdouaihy closed this Jun 14, 2018

mdouaihy reopened this Jun 14, 2018

Enrich the README with instructions to build and test under Windows.

4a1c918

rnburn reviewed Jun 14, 2018

View reviewed changes

Dont declare the Fontions when registering them for OpenTracing dynam…

f4bcbc2

…ic loading.

rnburn approved these changes Jun 14, 2018

View reviewed changes

rnburn merged commit e677ceb into opentracing:master Jun 14, 2018

rnburn mentioned this pull request Jun 21, 2018

Update to OpenTracing 1.5.0. lightstep/lightstep-tracer-cpp#105

Merged

mdouaihy deleted the windowsbuild branch June 23, 2018 13:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support build on Windows with MSVC 15.7 (#85) #93

Support build on Windows with MSVC 15.7 (#85) #93

mdouaihy commented Jun 11, 2018

rnburn commented Jun 11, 2018

rnburn Jun 12, 2018

mdouaihy Jun 12, 2018

rnburn Jun 12, 2018

mdouaihy Jun 12, 2018

rnburn Jun 12, 2018

mdouaihy Jun 13, 2018

rnburn Jun 12, 2018

rnburn Jun 12, 2018

mdouaihy commented Jun 14, 2018

rnburn Jun 14, 2018

mdouaihy Jun 14, 2018

rnburn Jun 14, 2018

mdouaihy Jun 14, 2018

rnburn Jun 14, 2018

mdouaihy Jun 14, 2018

rnburn commented Jun 14, 2018

mdouaihy commented Jun 14, 2018

rnburn commented Jun 14, 2018

mdouaihy commented Jun 14, 2018

mdouaihy commented Jun 14, 2018

rnburn Jun 14, 2018

mdouaihy Jun 14, 2018 •

edited

Loading

rnburn Jun 14, 2018

mdouaihy Jun 14, 2018

rnburn commented Jun 14, 2018

mdouaihy commented Jun 14, 2018 •

edited

Loading

rnburn commented Jun 14, 2018


		#ifdef _MSC_VER

		#define OPEN_TRACING_EXPORT __declspec(dllexport)

Support build on Windows with MSVC 15.7 (#85) #93

Support build on Windows with MSVC 15.7 (#85) #93

Conversation

mdouaihy commented Jun 11, 2018

rnburn commented Jun 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdouaihy commented Jun 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnburn commented Jun 14, 2018

mdouaihy commented Jun 14, 2018

rnburn commented Jun 14, 2018

mdouaihy commented Jun 14, 2018

mdouaihy commented Jun 14, 2018

Choose a reason for hiding this comment

mdouaihy Jun 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnburn commented Jun 14, 2018

mdouaihy commented Jun 14, 2018 • edited Loading

rnburn commented Jun 14, 2018

mdouaihy Jun 14, 2018 •

edited

Loading

mdouaihy commented Jun 14, 2018 •

edited

Loading