sociomantic-tsunami / ocean

General purpose, platform-dependent, high-performance library for D
Other
61 stars 56 forks source link

Add DUB unittest configuration to specify library dependencies #792

Closed joseph-wakeling-frequenz closed 4 years ago

joseph-wakeling-frequenz commented 4 years ago

This should help avoid linker errors when running dub test. Note that libssl (and hence libcrypto) have to be set to v1.0.0 instead of the default v1.1.0, as ocean requires symbols that are not available via the latter, specifically:

Placing these settings inside the unittest configuration should ensure that downstreams can avoid linking against library dependencies they do not need: however, it comes at the cost that downstreams have to specify their library dependencies manually.

Some libraries specified in Build.mak appear to be unnecessary and are therefore not included in the DUB config:

The ocean.core.UnitTestRunner module is excluded from unittest builds, as its main would clash with the DUB auto-created main. A different option would be to explicitly use the ocean unittest runner, but this is left for a later decision.

Finally, note that this setup will only run unittests, not integration tests: addressing this is left for future work.

As written, it is possible to run dub test successfully with upstream DMD. Using --compiler=dmd-transitional fails with an error message:

Error: module typetuple is in file 'std/typetuple.d' which cannot be read

while using --compiler=ldc2 or --compiler=ldmd2 fails because of https://github.com/sociomantic-tsunami/ocean/issues/730.

Geod24 commented 4 years ago

Note that libssl (and hence libcrypto) have to be set to v1.0.0 instead of the default v1.1.0, as ocean requires symbols that are not available via the latter, [..]

I am wondering if those usages could be replaced. I would be highly surprised if there was not a common subset that works for both libraries. Perhaps look into how Vibe.d did it ?

codecov[bot] commented 4 years ago

Codecov Report

Merging #792 into v5.x.x will decrease coverage by 0.07%. The diff coverage is n/a.

joseph-wakeling-frequenz commented 4 years ago

I am wondering if those usages could be replaced. I would be highly surprised if there was not a common subset that works for both libraries. Perhaps look into how Vibe.d did it ?

I'm sure that's possible, but this was the simplest and smallest change required to get dub test working -- I'm not sure how much time I have to go chasing into the depths of how ocean uses libssl ;-)

The risk is that fixing it would be a breaking change.

ben-palmer-sociomantic commented 4 years ago

I don't see anything too wrong with this approach however my dub knowledge is pretty much 0. Maybe @don-clugston-sociomantic or @stefan-koch-sociomantic could take a look?

don-clugston-sociomantic commented 4 years ago

I am wondering if those usages could be replaced. I would be highly surprised if there was not a common subset that works for both libraries. Perhaps look into how Vibe.d did it ?

You may be surprised. libssl is far worse than you would imagine. From v1.0 to v1.1 they completely changed the way it is initialised. A rational person would have changed the old functions to be no-ops, but instead they made a breaking change. Of course it would be better to just abandon v1.0 but at the time there was a reason we needed to support it. That might not be true any more.

ben-palmer-sociomantic commented 4 years ago

Of course it would be better to just abandon v1.0 but at the time there was a reason we needed to support it. That might not be true any more.

I think for this PR we can just merge these changes and look at upgrading SSL in a later PR.

joseph-wakeling-frequenz commented 4 years ago

Does that mean that we can take this in? :-P

don-clugston-sociomantic commented 4 years ago

Yeah, I'll take that as an LGTM. I also approve.