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

Can't make recent ppx_inline_test work on Windows #17

Open
db4 opened this issue Jul 30, 2019 · 9 comments
Open

Can't make recent ppx_inline_test work on Windows #17

db4 opened this issue Jul 30, 2019 · 9 comments
Labels
forwarded-to-js-devs This report has been forwarded to Jane Street's internal review system.

Comments

@db4
Copy link

db4 commented Jul 30, 2019

I'm trying to make ppx_inline_test.v0.12.0 work on Windows. To enable tests it uses

CAMLprim CAMLweakdef value Base_am_testing()
{
  return Val_false;
}

that is overridden in ppx_inline_test. The problem is that CAMLweakdef is no-op under Windows and the linker always takes base variant (base.cma goes before ppx_inline_test_lib.cma) so tests are always disabled. Any idea how to work around this?

@hhugo
Copy link

hhugo commented Oct 25, 2019

@dra27, do you have any idea how to make this work on windows ?

@db4
Copy link
Author

db4 commented Oct 25, 2019

I worked around this using additional library https://github.com/db4/ppx_inline_test_enable
To enable inline tests one should link ppx_inline_test_enable before base

@dra27
Copy link

dra27 commented Jun 29, 2020

There are possibly hacks to achieve similar behaviour on MSVC, but why is it necessary to use weak linking at all? Does something prevent having Base_am_testing in base/src/am_testing.c being a normal symbol, but based on a static variable which can be updated by another primitive (e.g. Base_set_testing) which the runner ensures is invoked?

@ghost
Copy link

ghost commented Jun 29, 2020

The idea is that the status change if you link against a special library that is linked when linking a test runner. Some code might inspect this status before the toplevel of this library is evaluated, causing them to see the wrong value.

That said, this dance could be done at the OCaml level with some help from the build system.

@db4
Copy link
Author

db4 commented Jul 6, 2020

The idea is that the status change if you link against a special library that is linked when linking a test runner. Some code might inspect this status before the toplevel of this library is evaluated, causing them to see the wrong value.

That said, this dance could be done at the OCaml level with some help from the build system.

@jeremiedimino what's wrong with my solution (additional helper library to enable testing)?

@ghost
Copy link

ghost commented Jul 6, 2020

@db4 what ensures that ppx_inline_test_enable is linked before base?

@dra27
Copy link

dra27 commented Jul 6, 2020

@db4 - sorry, my comment wasn't intended to discount your solution entirely! It does indeed work. Its downside is that it requires the library to be linked in each use-case first. A possibly stronger version might be for the enabling to be done in a .obj/.o file, since that would always be specified before libraries and always takes precedence over symbols in .lib/.a files. Equally, that also requires assistance from the build system (at least for Dune), I think.

@db4
Copy link
Author

db4 commented Jul 6, 2020

@db4 what ensures that ppx_inline_test_enable is linked before base?

Well, in the case of omake I just specify findlib packages in the necessary order.

@ghost
Copy link

ghost commented Jul 6, 2020

I see. Well, your solution seems like a reasonable workaround to me. For a long term solution, it seems clear to me that the best way forward is to something similar at the OCaml level, at least it will work on all platforms and will all backends. We'll probably look at this once we have made more progress on the switch from Jenga to Dune inside Jane Street, to avoid duplicating the work between Jenga and Dune. @db4, I can notify if/when we do this, so that you can update your omake setup accordingly.

That being said, @dra27 if you have a way to make the current solution based on C weak definitions work on Windows right now, we are happy to integrate it straight away.

@github-iron github-iron added the forwarded-to-js-devs This report has been forwarded to Jane Street's internal review system. label Oct 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
forwarded-to-js-devs This report has been forwarded to Jane Street's internal review system.
Projects
None yet
Development

No branches or pull requests

4 participants