From de42043ff20946b75593add0f10e998d3ac6f133 Mon Sep 17 00:00:00 2001 From: Erik Martin-Dorel Date: Mon, 12 Feb 2024 10:39:28 +0100 Subject: [PATCH 1/3] feat: Add CLI option `learn-ocaml build --build-dir=[./learn-ocaml-build]` where exercises are precompiled Motivation: - in current master, the exercises were always precompiled in-place; - this conflicted with the documented workflow (option "ro"): https://ocaml-sf.org/learn-ocaml/howto-deploy-a-learn-ocaml-instance.html https://ocaml-sf.org/learn-ocaml/howto-setup-exercise-development-environment.html - this new option makes it possible to copy and precompile exercises apart by default (using the learn-ocaml binary, or the official Docker image). Examples: - learn-ocaml build --repo=demo-repository - learn-ocaml build --repo=demo-repository serve - learn-ocaml build --repo=demo-repository serve --replace # or - learn-ocaml build --repo=demo-repository --build-dir=demo-repository # to retrieve the previous behavior. --- Dockerfile | 4 ++- Dockerfile.test-server | 4 ++- src/main/learnocaml_main.ml | 59 +++++++++++++++++++++++++++++-------- 3 files changed, 53 insertions(+), 14 deletions(-) diff --git a/Dockerfile b/Dockerfile index 126d0b2c1..8922e7d7c 100644 --- a/Dockerfile +++ b/Dockerfile @@ -55,6 +55,8 @@ RUN apk update \ VOLUME ["/repository"] RUN mkdir -p /sync && chown learn-ocaml:learn-ocaml /sync VOLUME ["/sync"] +RUN mkdir -p /build && chown learn-ocaml:learn-ocaml /build +VOLUME ["/build"] EXPOSE 8080 EXPOSE 8443 @@ -79,5 +81,5 @@ ENV OCAMLPATH="/usr/lib" RUN ln -sf "$opam_switch/lib/vg" "/usr/lib" RUN ln -sf "$opam_switch/lib/gg" "/usr/lib" -ENTRYPOINT ["dumb-init","/usr/bin/learn-ocaml","--sync-dir=/sync","--repo=/repository"] +ENTRYPOINT ["dumb-init","/usr/bin/learn-ocaml","--sync-dir=/sync","--repo=/repository","--build-dir=/build"] CMD ["build","serve"] diff --git a/Dockerfile.test-server b/Dockerfile.test-server index 43b0fe50e..cd81ab52f 100644 --- a/Dockerfile.test-server +++ b/Dockerfile.test-server @@ -61,6 +61,8 @@ RUN apk update \ VOLUME ["/repository"] RUN mkdir -p /sync && chown learn-ocaml:learn-ocaml /sync VOLUME ["/sync"] +RUN mkdir -p /build && chown learn-ocaml:learn-ocaml /build +VOLUME ["/build"] EXPOSE 8080 EXPOSE 8443 @@ -73,5 +75,5 @@ COPY --from=compilation /home/opam/install-prefix /usr COPY --from=compilation "$opam_switch/bin"/ocaml* "$opam_switch/bin/" COPY --from=compilation "$opam_switch/lib/ocaml" "$opam_switch/lib/ocaml/" -ENTRYPOINT ["dumb-init","/usr/bin/learn-ocaml","--sync-dir=/sync","--repo=/repository"] +ENTRYPOINT ["dumb-init","/usr/bin/learn-ocaml","--sync-dir=/sync","--repo=/repository","--build-dir=/build"] CMD ["build","serve"] diff --git a/src/main/learnocaml_main.ml b/src/main/learnocaml_main.ml index c09091920..bdba67c9d 100644 --- a/src/main/learnocaml_main.ml +++ b/src/main/learnocaml_main.ml @@ -24,6 +24,12 @@ let readlink f = try Sys.chdir cwd; f with Sys_error _ -> Sys.chdir (Filename.get_temp_dir_name ()); f +let absolute_filename path = + (* Note: symlinks are not taken into account *) + if Filename.is_relative path + then Filename.concat (Sys.getcwd ()) path + else path + module Args = struct open Arg @@ -44,6 +50,11 @@ module Args = struct "The path to the repository containing the exercises, lessons and \ tutorials." + let build_dir = + value & opt dir "./_learn-ocaml-build" & info ["build-dir"] ~docs ~docv:"DIR" ~doc: + "Directory where the repository exercises should be copied and precompiled. \ + Giving the same path as `--repo` is a valid value for `--build-dir`." + let app_dir = value & opt string "./www" & info ["app-dir"; "o"] ~docs ~docv:"DIR" ~doc: "Directory where the app should be generated for the $(i,build) command, \ @@ -215,9 +226,10 @@ module Args = struct Term.(const apply $contents_dir $try_ocaml $lessons $exercises $playground $toplevel $base_url) let repo_conf = - let apply repo_dir exercises_filtered jobs = + let apply repo_dir build_dir exercises_filtered jobs = Learnocaml_process_exercise_repository.exercises_dir := - repo_dir/"exercises"; + (* not repo_dir/"exercises" here - since we need write permissions *) + build_dir/"exercises"; Learnocaml_process_exercise_repository.exercises_filtered := Learnocaml_data.SSet.of_list (List.flatten exercises_filtered); Learnocaml_process_tutorial_repository.tutorials_dir := @@ -227,7 +239,7 @@ module Args = struct Learnocaml_process_exercise_repository.n_processes := jobs; () in - Term.(const apply $repo_dir $exercises_filtered $jobs) + Term.(const apply $repo_dir $build_dir $exercises_filtered $jobs) let term = let apply conf () = conf in @@ -243,16 +255,17 @@ module Args = struct commands: command list; app_dir: string; repo_dir: string; + build_dir: string; grader: Grader.t; builder: Builder.t; server: Server.t; } let term = - let apply commands app_dir repo_dir grader builder server = - { commands; app_dir; repo_dir; grader; builder; server } + let apply commands app_dir repo_dir build_dir grader builder server = + { commands; app_dir; repo_dir; build_dir; grader; builder; server } in - Term.(const apply $commands $app_dir $repo_dir + Term.(const apply $commands $app_dir $repo_dir $build_dir $Grader.term $Builder.term $Server.term app_dir base_url) end @@ -328,6 +341,30 @@ let main o = >|= fun i -> Some i) else Lwt.return_none in + let copy_build_exercises o = + (* NOTE: if `--build` = `--repo`, then no copy is needed. + Before checking path equality, we need to get canonical paths *) + let repo_dir = readlink o.repo_dir / "exercises" in + let build_dir = readlink o.build_dir / "exercises" in + if repo_dir <> build_dir then begin + Printf.printf "Populating %s\n%!" (o.build_dir / "exercises"); + (* NOTE: we choose to reuse Lwt_utils.copy_tree, + even if we could use "rsync" (upside: "--delete-delay", + but downside: would require the availability of rsync). *) + Lwt.catch + (fun () -> Lwt_utils.copy_tree repo_dir build_dir) + (function + | Failure _ -> + Lwt.fail_with @@ Printf.sprintf + "Failed to copy repo exercises to %s" + (build_dir) + | e -> Lwt.fail e) + (* NOTE: no chown is needed, + but we may want to run "chmod -R u+w exercises" + if the source repository has bad permissions... *) + end + else Lwt.return_unit + in let generate o = if List.mem Build o.commands then (let get_app_dir o = @@ -412,7 +449,9 @@ let main o = (fun _ -> Learnocaml_process_playground_repository.main (o.app_dir)) >>= fun playground_ret -> if_enabled o.builder.Builder.exercises (o.repo_dir/"exercises") - (fun _ -> Learnocaml_process_exercise_repository.main (o.app_dir)) + (fun _ -> + copy_build_exercises o >>= fun () -> + Learnocaml_process_exercise_repository.main (o.app_dir)) >>= fun exercises_ret -> Lwt_io.with_file ~mode:Lwt_io.Output (o.app_dir/"js"/"learnocaml-config.js") (fun oc -> @@ -442,11 +481,7 @@ let main o = let running = Learnocaml_server.check_running () in Option.iter Learnocaml_server.kill_running running; let temp = temp_app_dir o in - let app_dir = - if Filename.is_relative o.app_dir - then Filename.concat (Sys.getcwd ()) o.app_dir - else o.app_dir - in + let app_dir = absolute_filename o.app_dir in let bak = let f = Filename.temp_file From d8e9f1bceef94af83d4b1ff6d9ec0361ad539b7b Mon Sep 17 00:00:00 2001 From: Erik Martin-Dorel Date: Mon, 12 Feb 2024 18:04:12 +0100 Subject: [PATCH 2/3] fix: Remove VOLUME ["/build"], Use default build-dir in Docker packaging - as externalizing the build-dir in a volume looks unneeded: if the build-dir is shared, but the container is deleted, the www is deleted as well, and the exercices are rebuilt anyway - and as an extra volume would add more noise (mandating "-v args" tweaks) --- Dockerfile | 4 +--- Dockerfile.test-server | 4 +--- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/Dockerfile b/Dockerfile index 8922e7d7c..126d0b2c1 100644 --- a/Dockerfile +++ b/Dockerfile @@ -55,8 +55,6 @@ RUN apk update \ VOLUME ["/repository"] RUN mkdir -p /sync && chown learn-ocaml:learn-ocaml /sync VOLUME ["/sync"] -RUN mkdir -p /build && chown learn-ocaml:learn-ocaml /build -VOLUME ["/build"] EXPOSE 8080 EXPOSE 8443 @@ -81,5 +79,5 @@ ENV OCAMLPATH="/usr/lib" RUN ln -sf "$opam_switch/lib/vg" "/usr/lib" RUN ln -sf "$opam_switch/lib/gg" "/usr/lib" -ENTRYPOINT ["dumb-init","/usr/bin/learn-ocaml","--sync-dir=/sync","--repo=/repository","--build-dir=/build"] +ENTRYPOINT ["dumb-init","/usr/bin/learn-ocaml","--sync-dir=/sync","--repo=/repository"] CMD ["build","serve"] diff --git a/Dockerfile.test-server b/Dockerfile.test-server index cd81ab52f..43b0fe50e 100644 --- a/Dockerfile.test-server +++ b/Dockerfile.test-server @@ -61,8 +61,6 @@ RUN apk update \ VOLUME ["/repository"] RUN mkdir -p /sync && chown learn-ocaml:learn-ocaml /sync VOLUME ["/sync"] -RUN mkdir -p /build && chown learn-ocaml:learn-ocaml /build -VOLUME ["/build"] EXPOSE 8080 EXPOSE 8443 @@ -75,5 +73,5 @@ COPY --from=compilation /home/opam/install-prefix /usr COPY --from=compilation "$opam_switch/bin"/ocaml* "$opam_switch/bin/" COPY --from=compilation "$opam_switch/lib/ocaml" "$opam_switch/lib/ocaml/" -ENTRYPOINT ["dumb-init","/usr/bin/learn-ocaml","--sync-dir=/sync","--repo=/repository","--build-dir=/build"] +ENTRYPOINT ["dumb-init","/usr/bin/learn-ocaml","--sync-dir=/sync","--repo=/repository"] CMD ["build","serve"] From 67ba372f14bf63b2833c8aca73a347f67f86ee08 Mon Sep 17 00:00:00 2001 From: Erik Martin-Dorel Date: Mon, 12 Feb 2024 19:03:44 +0100 Subject: [PATCH 3/3] feat: Run "rm -fr ./_learn-ocaml-build/exercises" before the build Update CLI doc accordingly --- src/main/learnocaml_main.ml | 44 +++++++++++++++++++++++++++++-------- 1 file changed, 35 insertions(+), 9 deletions(-) diff --git a/src/main/learnocaml_main.ml b/src/main/learnocaml_main.ml index bdba67c9d..f42237215 100644 --- a/src/main/learnocaml_main.ml +++ b/src/main/learnocaml_main.ml @@ -30,6 +30,8 @@ let absolute_filename path = then Filename.concat (Sys.getcwd ()) path else path +let dflt_build_dir = "_learn-ocaml-build" + module Args = struct open Arg @@ -51,9 +53,14 @@ module Args = struct tutorials." let build_dir = - value & opt dir "./_learn-ocaml-build" & info ["build-dir"] ~docs ~docv:"DIR" ~doc: - "Directory where the repository exercises should be copied and precompiled. \ - Giving the same path as `--repo` is a valid value for `--build-dir`." + value & opt dir ("./" ^ dflt_build_dir) & info ["build-dir"] ~docs ~docv:"DIR" ~doc: + (Printf.sprintf + "Directory where the repo exercises are copied and precompiled. \ + When $(docv) takes its default value (e.g. when it is omitted in CLI), \ + '$(b,learn-ocaml build)' first erases the '$(docv)/exercises' subfolder. \ + Note that the default value for $(docv), './%s', is generally a sensible choice. \ + But passing the same argument as the one for $(i,--repo) is also a valid value for $(docv)." + dflt_build_dir) let app_dir = value & opt string "./www" & info ["app-dir"; "o"] ~docs ~docv:"DIR" ~doc: @@ -344,20 +351,39 @@ let main o = let copy_build_exercises o = (* NOTE: if `--build` = `--repo`, then no copy is needed. Before checking path equality, we need to get canonical paths *) - let repo_dir = readlink o.repo_dir / "exercises" in - let build_dir = readlink o.build_dir / "exercises" in - if repo_dir <> build_dir then begin - Printf.printf "Populating %s\n%!" (o.build_dir / "exercises"); + let repo_exos_dir = readlink o.repo_dir / "exercises" in + let build_exos_dir = readlink o.build_dir / "exercises" in + if repo_exos_dir <> build_exos_dir then begin + (* NOTE: if the CLI arg is "./_learn-ocaml-build" or "_learn-ocaml-build" + then the exercises subdirectory is erased beforehand *) + begin + if (o.build_dir = dflt_build_dir || o.build_dir = "./" ^ dflt_build_dir) + && Sys.file_exists build_exos_dir then + Lwt.catch (fun () -> + Lwt_process.exec ("rm",[|"rm";"-rf"; build_exos_dir|]) >>= fun r -> + if r <> Unix.WEXITED 0 then + Lwt.fail_with "Remove command failed" + else Lwt.return_unit) + (fun ex -> + Printf.eprintf + "Error: while removing previous build-dir \ + %s:\n %s\n%!" + build_exos_dir (Printexc.to_string ex); + exit 1) + else + Lwt.return_unit + end >>= fun () -> + Printf.printf "Building %s\n%!" (o.build_dir / "exercises"); (* NOTE: we choose to reuse Lwt_utils.copy_tree, even if we could use "rsync" (upside: "--delete-delay", but downside: would require the availability of rsync). *) Lwt.catch - (fun () -> Lwt_utils.copy_tree repo_dir build_dir) + (fun () -> Lwt_utils.copy_tree repo_exos_dir build_exos_dir) (function | Failure _ -> Lwt.fail_with @@ Printf.sprintf "Failed to copy repo exercises to %s" - (build_dir) + (build_exos_dir) | e -> Lwt.fail e) (* NOTE: no chown is needed, but we may want to run "chmod -R u+w exercises"