Resolve #112 Build Under Windows

[mdouaihy]

Which problem is this PR solving?

• Resolves Make Jaeger Client Cpp build and run on Windows #112

Short description of the changes

• Move to OpenTracing 1.5.0 that builds under Windows
• Change link of OpenTracing to dynamic. With Static linking of OpenTracing, we end up with multi definition of OT Symbols. This is not acceptable especially for the OT Global Tracer
• Fix OS specificities
  ** Networking API
  ** Windows headers
• Fix some tests with random behavior that failed on Windows
• Fix some MSVC specific compilation
• Add appveyor build file.

Signed-off-by: Mehrez Douaihy [email protected]

[isaachier]

I see a number of good changes but not sure if this is the right approach. I don't know if this is worthwhile seeing as I plan on deprecating most of this code soon in favor of a C client.

[isaachier]

@mdouaihy do you plan on updating this or are you busy?

[mdouaihy]

Hello @isaachier, sorry for the delay to answer but I have been sick for the past weeks.

I will take your comments into consideration and update the PR soon.

[isaachier]

Sorry to hear you were sick @mdouaihy. I based some of the code changes in #120 on some changes in dynamic loading you have here, so I appreciate the PR. Whenever you have a chance to update this, I'll be glad to help review it.

[mdouaihy]

@rnburn, @isaachier, @ruslo
Sorry for the late PR update. I took me some time but I was able finally to allocate time for it.

[isaachier]

This is all great. I understand you probably had to redo your changes entirely for this second review. From now on, if possible, please commit on top of the older changes and do not squash so we can see updates more easily.

[FredrikAppelros]

Is work on this progressing? Do you need any help? Would love to see this merged ASAP.

[mdouaihy]

Hi @FredrikAppelros, some tests are failing on Windows because if the bug described in issue #127.

I believe this is the last issue to solve in order to have this PR pushed.

[isaachier]

Hi @FredrikAppelros, some tests are failing on Windows because if the bug described in issue #127.

I believe this is the last issue to solve in order to have this PR pushed.

Hey I'll try to look this over tonight. If that is the only issue, I will allow a quick hack seeing as I have not come up with an easy fix for the bug.

[solomonlu]

Hello everyone, can this PR continue? We are eager to use jaeger on Windows.

[isaachier]

I'll try to see if this still merges and looks good.

[isaachier]

I see I have a bunch of unresolved comments here. Please fix them and resubmit.

[FredrikAppelros]

@isaachier: Are you referring to the style changes you requested or something more substantial when you mention unresolved comments?

As I offered before, I can help if I were given permission to push commits to this PR from @mdouaihy. The style changes should be easy enough, and I also have some fixes on top of this locally that I would like to see here.

One of the issues I had prevented me from getting this to compile since it could not pull in some dependency properly and the other makes sure that we can target the Windows 8.1 SDK.

I have not looked into the failing test case you guys mention though. Would be great if you could work out a solution for that.

[isaachier]

@FredrikAppelros I have no control over the PR source, but you can always fork the windowsbuild branch from @mdouaihy and make the changes there. If you do that, we can open a PR based off of your version of windowsbuild and potentially land this without @mdouaihy's involvement. If @mdouaihy is free to help, then there is a better way to do this: make a PR onto @mdouaihy's windowsbuild branch and it will automatically update here when it lands there.

[yurishkuro]

Or make smaller incremental PRs.

[mdouaihy]

hello guys, I will recheck the changes requested by @isaachier. However, i still have the problem described in issue #127. That blocked me from going further.

[isaachier]

@mdouaihy I was/am ready to accept this PR despite #127, especially because there is no easy fix there. You can use the local fix you had previously used. Just try to fix other comments and style suggestions.

[efficks]

Hi, Is anybody have any news on this PR. Is someone needs help to make it merged? I offer my help if needed.

[isaachier]

Hi, Is anybody have any news on this PR. Is someone needs help to make it merged? I offer my help if needed.

Hey @efficks thanks for the offer. Last I heard, @mdouaihy was going to look over the changes. If you'd like to take over this PR, try pulling @mdouaihy's branch and writing a PR against @mdouaihy's fork. Then, if @mdouaihy accepts it on this branch, it will automatically update the PR.

EDIT: Alternatively, use this branch or a new branch to implement Windows compatibility and create a new PR.

[mdouaihy]

Hello,
I am working again on the PR. However, due to update of msvc to 15.9, some dependencies dont compile anymore. I am following/working on them.

[mdouaihy]

Hi @yurishkuro, I configured a build for my fork at https://ci.appveyor.com/project/mdouaihy/jaeger-client-cpp/builds with appveyor configuration. I am thinking of keeping only Debug or Release instead of having them both (leading to doubled build time). What do you think?

[yurishkuro]

@mdouaihy what do you mean by "keeping debug or release only"? keeping where?

we seem to have an official appvoyer account for this repo, but it has no builds since last year.

https://ci.appveyor.com/project/jaegertracing/jaeger-client-cpp/settings

Would you like to configure it to build for this PR? I sent you an invitation to that project.

[mdouaihy]

Hi @yurishkuro, my question was about the types of builds to run under windows: Debug and/or Release. I ended up keeping only the Debug build since the release one does offer any added value.

As for the integration with Appveyor, I dont have permission on the github project. Can you check that the Github Appveyor app is well configured to run on PR?

[yurishkuro]

I reinstalled AppVeyor GitHubApp.

I see this in the Events

But in the build history I only see older builds.

[mdouaihy]

I think that this is because it's configured to run only on the master and not for the PR, no?

[yurishkuro]

I don't think so.

[mdouaihy]

I pushed a small change to the PR. Appveyor was triggered.

[AppVeyorBot]

✅ Build jaeger-client-cpp 12 completed (commit 9d2e736a34 by @mdouaihy)

[yurishkuro]

I am ready to merge this, just a couple of asks for comments.

Any idea why AppVeyor only posted a comment, instead of adding the status to the build check like Travis does?

[AppVeyorBot]

✅ Build jaeger-client-cpp 13 completed (commit 53357be935 by @mdouaihy)

[mdouaihy]

it seems that Appveyor does both: update the check and submit a message.

[yurishkuro]

Hm, I don't get it, now the Travis status is gone. I was hoping to get both, testing on different OSes

[mdouaihy]

hi @yurishkuro, maybe we need to reinstall travis ci?

[mdouaihy]

hi @yurishkuro, any update?

[yurishkuro]

I disabled and re-enabled the repo in Travis, and restarted the last build (which was for commit d08082c, so it missed several latest commits).

[yurishkuro]

Can you push another commit to this PR?

[AppVeyorBot]

✅ Build jaeger-client-cpp 14 completed (commit 40996c2abe by @mdouaihy)

[mdouaihy]

Thank you @yurishkuro !

[yurishkuro]

thank YOU for your contribution.

[FredrikAppelros]

Excellent! Will we see this in a release soon? :)

[yurishkuro]

According to https://github.com/jaegertracing/jaeger-client-cpp/blob/master/RELEASE.md, releasing is just a tag? If someone can create the pre-release PR (changelog changes, etc), then I can do a release.

[mdouaihy]

PR #169 sent for that purpose.