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

Get dune build working on Windows #92

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jonahbeckford
Copy link

During capnp-ocaml development I want dune build to work. That compiles the whole project, including the benchmark code. dune build makes it easy to see if I'm breaking other code, even if I can't run the benchmarks (yet) on Windows.

Two changes:

  • CAPNP_INCLUDE environment variable can be set if the build machine does not have a standard location to store capnp/*.capnp schema file (ex. /usr/include on Unix)
  • Windows compiler flags and commands are detected and used in benchmarks

I also added in manual compile instructions for Windows.

The python benchmark tool was not modified. Running the benchmark on Unix is fine for now.

- `CAPNP_INCLUDE` environment variable can be set if the build machine does
  not have a standard location to store `capnp/*.capnp` schema file (ex.
  `/usr/include` on Unix)
- Windows compiler flags and commands are detected and used in benchmarks
Copy link
Collaborator

@talex5 talex5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit confused about why CAPNP_INCLUDE is needed. If we can find the capnp binary OK on Windows, can't that can find the includes relative to itself?

@@ -2,44 +2,64 @@
(name main)
(enabled_if (= %{architecture} amd64))
(libraries capnp capnp_unix fast_rand base)
(ocamlopt_flags :standard -O3 -inline 2000))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could just get rid of this. It was for flambda, but it's probably out of date by now anyway.

Comment on lines +12 to +16
(rule
(enabled_if (= %{os_type} "Win32"))
(target ln.lines)
(action (with-stdout-to %{target} (echo "cmd\n/c\ncopy"))))

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be easier to have main.ml just take the benchmark name as the first argument instead of using the program name. I'm not sure why it's using hard-links at the moment.

@jonahbeckford
Copy link
Author

jonahbeckford commented Nov 19, 2024

I'm a bit confused about why CAPNP_INCLUDE is needed. If we can find the capnp binary OK on Windows, can't that can find the includes relative to itself?

capnp is a binary not under our control. Are you asking for changes to the upstream capnp binary? Because that mechanism does not exist today as far as I can tell: https://capnproto.org/language.html#imports has the rules for the search path.

EDIT: Here is the capnp search path logic: https://github.com/capnproto/capnproto/blob/6e071e34d88a8fc489638535899cd9d02e55bf76/c%2B%2B/src/capnp/compiler/capnp.c%2B%2B#L350-L367.

@talex5
Copy link
Collaborator

talex5 commented Nov 19, 2024

Yes, it seems reasonable to get this in the upstream binary. Otherwise, we'll have to do this dune stuff in every project that uses capnp.

@jonahbeckford
Copy link
Author

Yes, it seems reasonable to get this in the upstream binary. Otherwise, we'll have to do this dune stuff in every project that uses capnp.

If I were the maintainer of capnp I would say no to that request. I would question why the requester's build system and/or the package system does not support functions, aliases, macros ... anything so that this logic could be centralized and not repeated. I would also question why the requester wasn't using the build system (CMake) that capnp uses, which supports all of that. I would also do a quick google search and see that there is already a proposal in Dune to make it much more friendly to the CMake and has a specific example for capnp: ocaml/dune#8707 (comment).

I'll let you ask for the upstream issue. In the meantime, we can keep this issue open. I'll update the other two minor and uncontroversial things you mentioned (-O3 and arg passing) when I get some free time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants