remove opam dependancy #18

RyanGibb · 2022-09-23T13:51:49Z

if the opam config subst call fails, substitute the jobs variable with the number of physical cores in the machine

TheLortex

Looks good. The context here is that when used in a nix build, the opam command is not available (or available but with no switch bound to it), so we need a default configuration for the number of jobs used in the build.

src/dune

hannesm · 2022-09-25T21:27:18Z

I'm sorry to be picky, but /proc/cpuinfo is only available on GNU/Linux, not on any other Unix. If you want to find out how many CPUs are part of the hardware, please consult opam, or OCaml (e.g. ocaml/ocaml#11309).

Or, in case of opam not being available, why not simply use 1?

TheLortex · 2022-09-26T05:19:11Z

Yes we figured the portability issue. Another solution is to use sysctl to obtain CPU settings.

GMP is quite long to build so parallel jobs are very useful, hence the choice to use more than one job when possible. The reason why opam is not available is that @RyanGibb is working on building unikernels using Nix.

RyanGibb · 2022-09-26T10:37:50Z

Yes, I tried to build an opam switch in a nix derivation but there doesn't seem to be a nice way to install the compiler without network access.

Or, in case of opam not being available, why not simply use 1?

I've just pushed a change that defaults to 1 if /proc/cpuinfo isn't available.

Another solution is to use sysctl to obtain CPU settings.

It seems like the variable for CPU cores in sysctl differs by platform (sysctl hw.ncpu seems to work on BSD derivatives but not Linux).

This shouldn't be an issue outside though. Where opam is available outside nix derivations this will continue working as normal. On Linux the number of physical cores will be used, and otherwise it will fall back on 1.

dinosaure · 2022-09-26T10:38:47Z

The reason why opam is not available is that @RyanGibb is working on building unikernels using Nix.

It seems rather counter-intuitive to me when the mirage workflow requires opam in anyway.

hannesm · 2022-09-26T10:49:39Z

I'm all in for removing the opam dependency from the build of ocaml-gmp. The background is that I also compile MirageOS unikernels without an opam binary. (see robur-coop/orb@ed60f97 for the specific hack I had to implement just for ocaml-gmp).

Now, on the downside I'm against adding complexity (read: conditionals) to the build systems. So, if it is desired to use multiple CPU cores while building ocaml-gmp, why not resort to the opam package processor (https://github.com/haesbaert/ocaml-processor) that provides a function cpu_count and is available for all OCaml versions and platforms ocaml-gmp is supposed to work with.

hannesm · 2022-09-26T10:52:53Z

It seems rather counter-intuitive to me when the mirage workflow requires opam in anyway.

It actually does not really anymore -- I mean truly there's the opam-repository, and opam monorepo etc. -- but with MirageOS4, e.g. the opam config var calls to invoke pkg-config are no longer necessary. And I'd be keen on removing the opam binary as a dependency from a unikernel build. At least some versions of dune executed opam --version, but I think that has been revised as well.

Anyways, I gathered the parts of opam that are needed (or used to be needed?) in a shell script https://github.com/roburio/orb/blob/next/opam.sh (NB: mirage3 and mirage4 workflows) (which works fine to build many unikernels. see the https://builds.robur.coop overview).

RyanGibb · 2022-09-26T10:55:24Z

It seems rather counter-intuitive to me when the mirage workflow requires opam in anyway.

With this workflow we actually only use opam to resolve dependency versions. Nix provides binary or source (for the monorepo workflow) derivations for all the dependencies.

RyanGibb · 2022-09-26T10:59:53Z

So, if it is desired to use multiple CPU cores while building ocaml-gmp, why not resort to the opam package processor (https://github.com/haesbaert/ocaml-processor) that provides a function cpu_count and is available for all OCaml versions and platforms ocaml-gmp is supposed to work with.

Correct me if I'm wrong, but this would require adding OCaml as a dependency to the project?

hannesm · 2022-09-26T11:06:32Z

@RyanGibb yes, this will add "ocaml" as a dependency to ocaml-gmp. But turns out, it already depends on dune ocaml and conf-m4, so I hardly see any issue. From my understanding, the whole idea of this project is to deliver gmp as something built by dune.

RyanGibb · 2022-09-26T15:59:20Z

@RyanGibb yes, this will add "ocaml" as a dependency to ocaml-gmp. But turns out, it already depends on dune ocaml and conf-m4, so I hardly see any issue. From my understanding, the whole idea of this project is to deliver gmp as something built by dune.

Great, this sounds like it would be a more portable solution then :)

RyanGibb · 2022-09-26T18:54:22Z

I can confirm this is building mirage-www!

dinosaure · 2022-09-26T18:56:19Z

Can you run a dune build @fmt --auto on your PR? LGTM otherwise.

hannesm · 2022-09-26T18:58:08Z

uhm, this still uses /proc/cpuinfo (thus, Linux-only) and conditionals...

RyanGibb · 2022-09-26T19:20:35Z

uhm, this still uses /proc/cpuinfo

I've replaced this with a call to ocaml-processor-dump which we grep and cut.

and conditionals...

And removed the opam conditional.

Let me know what you think of this!

hannesm · 2022-09-26T19:37:27Z

thanks, looks good to me

RyanGibb · 2022-09-26T19:48:00Z

Hmm, this doesn't seem to work in a monorepo:

www-dev> File "duniverse/ocaml-processor/lib/dune", line 1, characters 0-451:
www-dev>  1 | (library
www-dev>  2 |  (name processor)
www-dev>  3 |  (public_name processor)
www-dev> ....
www-dev> 12 |  (private_modules ioreg)
www-dev> 13 |  (foreign_stubs (language c) (names processor_stubs))
www-dev> 14 |  (c_library_flags (-lpthread)))
www-dev> Error: Some modules don't have an implementation.
www-dev> You need to add the following field to this stanza:
www-dev>   (modules_without_implementation query topology)

I could try and patch/submit a PR for this but I'm not familiar with the project. Do you know what's going on here (why these modules don't have an implementation) @hannesm?

dinosaure · 2022-09-26T20:14:26Z

/cc @haesbaert

haesbaert · 2022-09-26T20:31:44Z

Hmm, this doesn't seem to work in a monorepo:

www-dev> File "duniverse/ocaml-processor/lib/dune", line 1, characters 0-451:
www-dev>  1 | (library
www-dev>  2 |  (name processor)
www-dev>  3 |  (public_name processor)
www-dev> ....
www-dev> 12 |  (private_modules ioreg)
www-dev> 13 |  (foreign_stubs (language c) (names processor_stubs))
www-dev> 14 |  (c_library_flags (-lpthread)))
www-dev> Error: Some modules don't have an implementation.
www-dev> You need to add the following field to this stanza:
www-dev>   (modules_without_implementation query topology)

I could try and patch/submit a PR for this but I'm not familiar with the project. Do you know what's going on here (why these modules don't have an implementation) @hannesm?

Basically we try to do some discovery on how to best build the topology and query modules, somehow you're not falling into any of the cases from dune:

(rule
 (target query.ml)
 (enabled_if (= %{system} "macosx"))
 (action (copy query_apple.ml query.ml)))
(rule
 (target query.ml)
 (enabled_if (or
              (= %{system} "linux")
              (= %{system} "freebsd")
              (= %{system} "netbsd")
              (= %{system} "openbsd")))
 (action (copy query_from_topology.ml query.ml)))

(rule
 (target topology.ml)
 (enabled_if (and (= %{architecture} "amd64")
              (or (= %{system} "linux") (= %{system} "freebsd"))))
 (action (copy topology_amd64.ml topology.ml)))
(rule
 (target topology.ml)
 (enabled_if (and (= %{system} "macosx") (= %{architecture} "amd64")))
 (action (copy topology_apple_amd64.ml topology.ml)))
(rule
 (target topology.ml)
 (enabled_if (and (= %{system} "macosx") (= %{architecture} "arm64")))
 (action (copy topology_apple_arm64.ml topology.ml)))
(rule
 (target topology.ml)
 (enabled_if (or
              (= %{system} "netbsd")
              (= %{system} "openbsd")
              (= %{system} "dragonflybsd")
              (and (= %{system} "linux") (not (= %{architecture} "amd64")))))
 (action (copy topology_fake.ml topology.ml)))

Do the dune variables somehow evaluate to something else in the monorepo ? as in is system not linux and architecture not amd64 ?

haesbaert · 2022-09-26T20:44:05Z

Btw: you can also:

$ for host in sam robur naiad mactina;do ssh $host 'uname -a && (which nproc > /dev/null 2>&1 && nproc) || sysctl -n hw.ncpu';done
Linux sam 5.18.16-arch1-1 #1 SMP PREEMPT_DYNAMIC Wed, 03 Aug 2022 11:25:04 +0000 x86_64 GNU/Linux
8
FreeBSD shell.robur.coop 12.2-RELEASE-p9 FreeBSD 12.2-RELEASE-p9 2430d82b40ea(releng/12.2) VM  amd64
8
OpenBSD naiad.spacehopper.org 7.2 GENERIC.MP#8 amd64
12
Darwin feanor.midearth 20.6.0 Darwin Kernel Version 20.6.0: Tue Jun 21 20:50:28 PDT 2022; root:xnu-7195.141.32~1/RELEASE_X86_64 x86_64
4

hannesm · 2022-09-26T20:49:58Z

May be related to / an instance of ocaml/dune#3917

haesbaert · 2022-09-26T20:54:11Z

May be related to / an instance of ocaml/dune#3917

hmm could be, maybe @TheLortex can cheap in ? I never used monorepo, but to my knowledge those variables are obtained in the runtime (it's what comes from ocamlc -config)

RyanGibb · 2022-09-27T14:37:06Z

Basically we try to do some discovery on how to best build the topology and query modules

Thanks, that makes sense.

Do the dune variables somehow evaluate to something else in the monorepo ? as in is system not linux and architecture not amd64 ?

Hmm, this is running on x86-64 Linux. Possibly nix sandboxing is to blame. I'm trying to figure out a way to debug dune variables (e.g. print them to stdout).

(which nproc > /dev/null 2>&1 && nproc) || sysctl -n hw.ncpu

The only issue with this is that nproc prints the number of virtual cores rather than physical cores which may be sub-optimal for CPU bound processes (which I think building gmp is).

haesbaert · 2022-09-27T15:57:07Z

Not sure I agree, threads helping or not is not much related to it being CPU bound, it's more related to instructions per cycles and therefore cache-misses, building stuff is very cache non friendly, so threading should help.

IMHO: this isn't something you/we should ponder much, this is hard to know in advance and depends on a bunch of stuff, the sane thing to do is just build on every logical CPU available (you can test if you want, but I doubt you find a case where threading doesn't help).

I used to build many things on a Niagra T2 (8 threads per core) on an SO where the kernel was completely giant locked, and using all threads were still almost always beneficial.

RyanGibb · 2022-09-27T16:19:58Z

That makes complete sense. It sounds like to avoid the complexity of building ocaml-processor from source with opam monorepo we should use your portable script to get the number of logical CPUs.

I believe sysctl -n hw.ncpu should work on BSD-based unixes like FreeBSD, OpenBSD, and Darwin @hannesm.

hannesm · 2022-09-27T16:41:10Z

yes, well, at least on FreeBSD that's fine. :)

RyanGibb · 2022-09-27T16:55:59Z

Great :) I've also added a default value of 1 if sysctl fails, and squashed the commits together. Thanks for the help again @haesbaert !

haesbaert · 2022-09-28T09:00:58Z

Great :) I've also added a default value of 1 if sysctl fails, and squashed the commits together. Thanks for the help again @haesbaert !

Good call !
You're most welcome 👯‍♂️

dinosaure · 2022-09-28T10:33:50Z

Ok, so if everyone is happy with this PR, we can merge it. @TheLortex do you want to release it?

TheLortex · 2022-09-28T11:28:40Z

Yes I can take care of it today.

@RyanGibb

CHANGES: - Build system: when used in a monorepo, do not invoke `opam` to figure out the level of parallelism. Instead, use `sysctl` to obtain the number of cores and provide `1` as a default value. (@RyanGibb, mirage/ocaml-gmp#18)

RyanGibb mentioned this pull request Sep 23, 2022

Unikernels depending on zarith won't build for non-unix targets RyanGibb/hillingar#1

Closed

TheLortex requested changes Sep 23, 2022

View reviewed changes

src/dune Outdated Show resolved Hide resolved

RyanGibb force-pushed the master branch from f74ea53 to 7df69c6 Compare September 23, 2022 14:14

RyanGibb force-pushed the master branch from 7df69c6 to b4c4d64 Compare September 26, 2022 10:31

RyanGibb force-pushed the master branch from b4c4d64 to a4a4639 Compare September 26, 2022 18:36

RyanGibb force-pushed the master branch 2 times, most recently from 886b28f to 04704d2 Compare September 27, 2022 16:54

remove opam dependancy

8e8a665

RyanGibb force-pushed the master branch from 04704d2 to 8e8a665 Compare September 27, 2022 17:05

dinosaure merged commit dbe5097 into mirage:master Sep 28, 2022

TheLortex mentioned this pull request Sep 28, 2022

[new release] gmp (6.2.1-4) ocaml/opam-repository#22180

Merged

balsoft mentioned this pull request Oct 10, 2022

fake-opam is overriding actual opam tweag/opam-nix#23

Closed

dinosaure mentioned this pull request Oct 21, 2022

Don't suppose getconf is in PATH at runtime mirage/ocaml-git#599

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove opam dependancy #18

remove opam dependancy #18

RyanGibb commented Sep 23, 2022

TheLortex left a comment

hannesm commented Sep 25, 2022

TheLortex commented Sep 26, 2022

RyanGibb commented Sep 26, 2022

dinosaure commented Sep 26, 2022 •

edited

Loading

hannesm commented Sep 26, 2022

hannesm commented Sep 26, 2022 •

edited

Loading

RyanGibb commented Sep 26, 2022

RyanGibb commented Sep 26, 2022 •

edited

Loading

hannesm commented Sep 26, 2022

RyanGibb commented Sep 26, 2022

RyanGibb commented Sep 26, 2022 •

edited

Loading

dinosaure commented Sep 26, 2022

hannesm commented Sep 26, 2022

RyanGibb commented Sep 26, 2022

hannesm commented Sep 26, 2022

RyanGibb commented Sep 26, 2022 •

edited

Loading

dinosaure commented Sep 26, 2022

haesbaert commented Sep 26, 2022

haesbaert commented Sep 26, 2022 •

edited

Loading

hannesm commented Sep 26, 2022

haesbaert commented Sep 26, 2022

RyanGibb commented Sep 27, 2022

haesbaert commented Sep 27, 2022 •

edited

Loading

RyanGibb commented Sep 27, 2022 •

edited

Loading

hannesm commented Sep 27, 2022

RyanGibb commented Sep 27, 2022 •

edited

Loading

haesbaert commented Sep 28, 2022

dinosaure commented Sep 28, 2022

TheLortex commented Sep 28, 2022

remove opam dependancy #18

remove opam dependancy #18

Conversation

RyanGibb commented Sep 23, 2022

TheLortex left a comment

Choose a reason for hiding this comment

hannesm commented Sep 25, 2022

TheLortex commented Sep 26, 2022

RyanGibb commented Sep 26, 2022

dinosaure commented Sep 26, 2022 • edited Loading

hannesm commented Sep 26, 2022

hannesm commented Sep 26, 2022 • edited Loading

RyanGibb commented Sep 26, 2022

RyanGibb commented Sep 26, 2022 • edited Loading

hannesm commented Sep 26, 2022

RyanGibb commented Sep 26, 2022

RyanGibb commented Sep 26, 2022 • edited Loading

dinosaure commented Sep 26, 2022

hannesm commented Sep 26, 2022

RyanGibb commented Sep 26, 2022

hannesm commented Sep 26, 2022

RyanGibb commented Sep 26, 2022 • edited Loading

dinosaure commented Sep 26, 2022

haesbaert commented Sep 26, 2022

haesbaert commented Sep 26, 2022 • edited Loading

hannesm commented Sep 26, 2022

haesbaert commented Sep 26, 2022

RyanGibb commented Sep 27, 2022

haesbaert commented Sep 27, 2022 • edited Loading

RyanGibb commented Sep 27, 2022 • edited Loading

hannesm commented Sep 27, 2022

RyanGibb commented Sep 27, 2022 • edited Loading

haesbaert commented Sep 28, 2022

dinosaure commented Sep 28, 2022

TheLortex commented Sep 28, 2022

dinosaure commented Sep 26, 2022 •

edited

Loading

hannesm commented Sep 26, 2022 •

edited

Loading

RyanGibb commented Sep 26, 2022 •

edited

Loading

RyanGibb commented Sep 26, 2022 •

edited

Loading

RyanGibb commented Sep 26, 2022 •

edited

Loading

haesbaert commented Sep 26, 2022 •

edited

Loading

haesbaert commented Sep 27, 2022 •

edited

Loading

RyanGibb commented Sep 27, 2022 •

edited

Loading

RyanGibb commented Sep 27, 2022 •

edited

Loading