Adding ocaml-bench/sandmark dependency packages #21

moyodiallo · 2022-04-11T17:42:13Z

This PR is about to fix #17 and It could be done in 2 stage:

the first is to be able to handle different config with the pipeline, like how to handle different group of opam-repositories. It could be possible to have a same repo url that we want to build differently in different config.
the second one is to have a current-plugin that extract dynamically all the dependency packages from:
- https://github.com/ocaml-bench/sandmark/blob/main/dependencies/template/dev.opam
- https://github.com/ocaml-bench/sandmark/tree/main/dependencies/packages

moyodiallo · 2022-04-27T14:30:28Z

Hi @shakthimaan would you like to change some dev-repo url in https://github.com/ocaml-bench/sandmark dependencies.

git://github.com/dbuenzli/nbcodec to https://github.com/dbuenzli/nbcodec.git
git://github.com/ocaml-ppx/ppx_tools.git to htttps://github.com/ocaml-ppx/ppx_tools.git

moyodiallo · 2022-05-17T13:24:00Z

To have some idea about the use case difference against the current Multicore-CI #23

moyodiallo · 2022-06-08T12:26:20Z

The approach has changed. In order to extract, to clone and build all dependencies package which we missed some config of Sandmark's dependencies opam-repository.

We only clone sandmark github repo and after extracting the packages(name,version), we use the necessary config (add the opam-repository in https://github.com/ocaml-bench/sandmark/tree/main/dependencies/packages) to build each extracted package by using "opam install".

Necessary for the packages that are patched. This approach simplifies a lot against the Sandmark's particularity.

I don't know how I missed that at some point 😠

In the old config, the pipeline could not handle different config of opam-repositories. With this fix, it's possible to have different groups of opam-repositories. The dependency packages of Sandmark repo are added manually, the next time will be adding those dynamically from Sandmark repo This is the first commit in fixing the issues #17

A plugin for extracting sandmark's dependencies packages: Fist it's about looking for all packages in sandmark specific opam-repository and get their dev-repo url which are important for cloning and building them. Second there's another pacakges in 'dev.opam' file. Those packages only have name and version which is use to get their dev-repo url from the sandmark's specific opam-repository if not found then look in to the the default opam-repository.

It's would be nice to build all extracted packages from Sandmark plugin in the pipeline. The building process is reused. Now the sandmark's dependencies packages are built dynamically no longer packages's url needed in the config file.

With the old version, the pipeline config file is little bit generalize, but build_mechanism wasn't generalize. Fix also repo url of sandmark's packages extractions One thing to notice, the build_mechanism in the config file won't to be taken when the analyze stage of the pipleline results on at least one Selection.

This plugin is used to get a hash(git rev-parse) of specific refs. This is usefull because the clone function from ocurrent package use already a prefix "origin/", the function fail when called with a refs that is not prefixed with "origin/" by the git repo. With that hash it's easly possible to fetch the right version.

Sandmark's repos dependencies with their specificied version can't be cloned directly. So we need to get the main branch repo url (url@branch) of each repos and then clone them. We need to search the right tag also, to avoid fail during the fectch of sandmark's repos.

Sandmark's package dependencies doesn't necessary need the all packages in a repo. The actual state of multicore-ci includes all packages of a given repo during the analysis. To avoid an unnecessary fail of the pipeline we skip the analysis part. Almost all those packages that sandmark use are old version and we fail sometime during the analysis because of ocamlformat file when 'version' is not specified.

Delete `Sand_build because it the same with `Opam. Build sandmark's packages by using "opam install" after skipping the analysis. During the build of a package we pin all packages in the same repo. Important when code is from a cloned repo because we could end up building from opam source.

All opam-repositories from the analysis are used now during the build time. This commit could solve #25

The previous is about extracting all packges by name, version and dev-repo from sandmark dev repository. Clone all those packages before building each of them by missing some conf in sandmark dev repo. This simplification is about cloning only sandmark dev repository. In sandmark repo there's an opam-repository directory, so we add this directory using "opam repo add <directory name>" command. Each build of extracted packages use "opam install -yv <name>" command. With this aproach we got some benefit. When there's is some patches in the opam-repository directory for some packages, they are used by opam during the building.

There's some packages which are not necessary to build. In contrast it's a filter that give flexiblity in the config. Clean some part of the code too, used for the old approach.

moyodiallo · 2022-06-10T21:16:09Z

I did a rebase the branch. Could be merged if there's no opposition.

tmcgilchrist · 2022-06-21T06:36:16Z

lib/analyse.ml

@@ -334,6 +341,18 @@ module Analysis = struct
      Current.Job.log job "@[<v2>Results:@,%a@]" Yojson.Safe.(pretty_print ~std:true) (to_yojson r);
      Lwt_result.return r
    )
+
+    let of_dir_sandmark ~solver ~job ~platforms ~opam_repository_commits ~package_name ?is_compiler ?compiler_commit _ =


Suggested change

let of_dir_sandmark ~solver ~job ~platforms ~opam_repository_commits ~package_name ?is_compiler ?compiler_commit _ =

let of_dir_sandmark ~job ~platforms ~opam_repository_commits ~package_name ?is_compiler ?compiler_commit () =

This isn't using ~solver just remove that argument along with any others you aren't using.

tmcgilchrist · 2022-06-21T06:42:58Z

lib/analyse.ml

-    { src; compiler_commit=None }
-    { Examine.Value.opam_repository_commits; platforms; is_compiler; compiler_commit=None }
+    { sandmark_package; src; compiler_commit=None }
+    { Examine.Value.opam_repository_commits; platforms; is_compiler; compiler_commit=None; sandmark_package=sandmark_package }


Suggested change

{ Examine.Value.opam_repository_commits; platforms; is_compiler; compiler_commit=None; sandmark_package=sandmark_package }

{ Examine.Value.opam_repository_commits; platforms; is_compiler; compiler_commit=None; sandmark_package }

tmcgilchrist · 2022-06-21T06:43:10Z

lib/analyse.ml

-    { Examine.Value.opam_repository_commits; platforms; is_compiler=false; compiler_commit=(Some compiler_commit) }
+    { sandmark_package; src; compiler_commit=(Some compiler_commit) }
+    { Examine.Value.opam_repository_commits; platforms; is_compiler=false; compiler_commit=(Some compiler_commit);
+    sandmark_package=sandmark_package }


Suggested change

sandmark_package=sandmark_package }

sandmark_package }

tmcgilchrist · 2022-06-21T06:55:36Z

lib/cluster_build.ml

@@ -28,16 +28,18 @@ module Op = struct
      repo : string;                            (* Used to choose a build cache *)
      test_repo : string option;                       (* The repo under test, if repo is a compiler *)
      label : string;                           (* A unique ID for this build within the commit *)
+      sandmark_package: string option


Please include a comment here detailing what the string represents.

tmcgilchrist · 2022-06-21T07:03:07Z

lib/opam_build.ml

+    if String.equal repo default then run "opam repo priority default 1 --set-default"
+    else run "opam repo add rep-%s %s --set-default" (String.sub commit 0 7) repo) commits_in_order @
+  [run "opam update -u"]


Suggested change

if String.equal repo default then run "opam repo priority default 1 --set-default"

else run "opam repo add rep-%s %s --set-default" (String.sub commit 0 7) repo) commits_in_order @

[run "opam update -u"]

if String.equal repo default

then run "opam repo priority default 1 --set-default"

else run "opam repo add rep-%s %s --set-default" (String.sub commit 0 7) repo) commits_in_order @

[run "opam update -u"]

Maybe split the if then line out.

tmcgilchrist · 2022-06-21T07:28:29Z

lib/selection.ml

@@ -2,19 +2,16 @@
 type t = {
  variant : Variant.t;                (** The variant image to build on. *)
  packages : string list;             (** The selected packages ("name.version"). *)
-  commit : string;                    (** A commit in opam-repository to use. *)
+  commits : (string * string) list;   (** The list of repo_url,commit) opam-repositories to use.*)


Suggested change

commits : (string * string) list; (** The list of repo_url,commit) opam-repositories to use.*)

commits : (string * string) list; (** The list of (repo_url,commit) opam-repositories to use.*)

tmcgilchrist · 2022-06-21T07:29:50Z

lib/selection.mli

@@ -2,7 +2,7 @@
 type t = {
  variant : Variant.t;                (** The variant image to build on. *)
  packages : string list;             (** The selected packages ("name.version"). *)
-  commit : string;                    (** A commit in opam-repository to use. *)
+  commits : (string * string) list;   (** The different opam-repository to use. *)


Can you provide more detail about why this has changed to a list?
The different opam-repository to use. doesn't explain what it is used for.

This is about using all different open-repository during the build, those were used in the analysis, but during the build only the default was used, and the build could end up with missing packages.

tmcgilchrist · 2022-06-21T07:30:55Z

service/conf.ml

@@ -85,9 +85,10 @@ let platforms =
      [v label tag ov]
  in
  let make_release ?arch ov =
-    let distro = DD.tag_of_distro master_distro in
+    let distro = DD.tag_of_distro (master_distro :> DD.t) in


Suggested change

let distro = DD.tag_of_distro (master_distro :> DD.t) in

let distro = DD.tag_of_distro master_distro in

This is not necessary.

tmcgilchrist · 2022-06-21T08:48:34Z

service/main.ml

+      match app with
+      | Some app -> Current_github.App.webhook_secret app
+      | None -> ""
+    in


This should stay as mandatory, I think the setup is there in the deployed en

The app is not used by the pipeline yet. This is also useful for debugging purpose, we don't have to config the app stuff, to start the service.

tmcgilchrist · 2022-06-21T08:48:54Z

service/main.ml

+let github_app_id =
+  Arg.required @@
+  Arg.opt Arg.(some string) None @@
+  Arg.info ["github-app-id"]


There should be a Cmdliner version of this in current_gitlab

I don't understand, but so far the pipeline doesn't use any gitlab repo.

shakthimaan · 2022-06-22T10:45:48Z

service/conf.ml

+    ; "rm -fr dependencies/packages/dune"
+	  ; "opam pin add -n --yes base.v0.14.3 https://github.com/janestreet/base.git#v0.14.3"
+	  ; "opam pin add -n --yes coq-core https://github.com/ejgallego/coq/archive/refs/tags/multicore-2021-09-29.tar.gz"
+	  ; "opam pin add -n --yes coq-stdlib https://github.com/ejgallego/coq/archive/refs/tags/multicore-2021-09-29.tar.gz"


Why do the above steps? If tomorrow, we have an updated version of base and coq building in Sandmark, these steps will be outdated.

I tried to match what you are using now. If you change those package in the future, it would be better to have a PR. For dune it's about getting the good version because of bigarray issue ocaml/dune#5526

You can skip the packages pinned in the Makefile, as we try to keep it to a minimum. If you look at Irmin replay benchmark for 5.0, the pinned list is growing:

https://github.com/ocaml-bench/sandmark/pull/343/files#diff-76ed074a9305c04054cdebb9e9aad2d818052b07091de1f20cad0bbac34ffb52R137

And these do not yet build for 5.1.0+trunk, so we expect them to change again. We are aware of these breaking changes and hence use the latest -alpha builds. For now, the packages from dev.opam and dependencies/packages constitute most of the Sandmark dependencies that are of interest for the CI. Thanks!

@tmcgilchrist

Refactoring the code and stop pinning sandmark packages review from @tmcgilchrist and @shakthimaan

moyodiallo mentioned this pull request Apr 27, 2022

Backport changes from Ewan's fork to ocurrent #19

Closed

2 tasks

moyodiallo added 16 commits June 10, 2022 12:08

Building extracted packages

d9abbe3

It's would be nice to build all extracted packages from Sandmark plugin in the pipeline. The building process is reused. Now the sandmark's dependencies packages are built dynamically no longer packages's url needed in the config file.

switching ocurrent/ocurrent to the master

bb08b39

upgrade ocurrent and submodules

7134a99

Remove current_incr, no longer supported in ocurrent/ocurrent

2bde898

ocluster was omitted

dc831de

Fix duplicated build of Sandmark's packages

6ec083d

All "opam repository" commits is now used

eb27988

All opam-repositories from the analysis are used now during the build time. This commit could solve #25

skip some Sandmark's dependencies package

416c4f2

There's some packages which are not necessary to build. In contrast it's a filter that give flexiblity in the config. Clean some part of the code too, used for the old approach.

moyodiallo requested a review from tmcgilchrist June 10, 2022 21:09

moyodiallo force-pushed the sandmark-dependencies branch from 837405b to 416c4f2 Compare June 10, 2022 21:12

better set for submodules

3836847

moyodiallo mentioned this pull request Jun 15, 2022

Fix graphsql-ci failure after web-service restart #27

Merged

tmcgilchrist requested a review from shakthimaan June 21, 2022 05:59

tmcgilchrist reviewed Jun 21, 2022

View reviewed changes

Merge the master

9ec8cfd

shakthimaan reviewed Jun 22, 2022

View reviewed changes

Refactoring the code

f1cfaad

Refactoring the code and stop pinning sandmark packages review from @tmcgilchrist and @shakthimaan

Using Fmt.failwith with a context

8e6593c

moyodiallo merged commit ce5ec41 into master Jun 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding ocaml-bench/sandmark dependency packages #21

Adding ocaml-bench/sandmark dependency packages #21

moyodiallo commented Apr 11, 2022

moyodiallo commented Apr 27, 2022

moyodiallo commented May 17, 2022

moyodiallo commented Jun 8, 2022

moyodiallo commented Jun 10, 2022

tmcgilchrist Jun 21, 2022

tmcgilchrist Jun 21, 2022

tmcgilchrist Jun 21, 2022

tmcgilchrist Jun 21, 2022

tmcgilchrist Jun 21, 2022

tmcgilchrist Jun 21, 2022

tmcgilchrist Jun 21, 2022

moyodiallo Jun 22, 2022

tmcgilchrist Jun 21, 2022

tmcgilchrist Jun 21, 2022

moyodiallo Jun 22, 2022

tmcgilchrist Jun 21, 2022

moyodiallo Jun 22, 2022

shakthimaan Jun 22, 2022

moyodiallo Jun 22, 2022

shakthimaan Jun 22, 2022

	let of_dir_sandmark ~solver ~job ~platforms ~opam_repository_commits ~package_name ?is_compiler ?compiler_commit _ =
	let of_dir_sandmark ~job ~platforms ~opam_repository_commits ~package_name ?is_compiler ?compiler_commit () =

	{ Examine.Value.opam_repository_commits; platforms; is_compiler; compiler_commit=None; sandmark_package=sandmark_package }
	{ Examine.Value.opam_repository_commits; platforms; is_compiler; compiler_commit=None; sandmark_package }

	commits : (string * string) list; (** The list of repo_url,commit) opam-repositories to use.*)
	commits : (string * string) list; (** The list of (repo_url,commit) opam-repositories to use.*)

	let distro = DD.tag_of_distro (master_distro :> DD.t) in
	let distro = DD.tag_of_distro master_distro in

Adding ocaml-bench/sandmark dependency packages #21

Adding ocaml-bench/sandmark dependency packages #21

Conversation

moyodiallo commented Apr 11, 2022

moyodiallo commented Apr 27, 2022

moyodiallo commented May 17, 2022

moyodiallo commented Jun 8, 2022

moyodiallo commented Jun 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment