Skip to content

Conversation

@MisterDA
Copy link
Contributor

No description provided.

@MisterDA MisterDA force-pushed the docker2 branch 3 times, most recently from 984772f to a2b9bea Compare October 19, 2022 17:34
@MisterDA MisterDA marked this pull request as draft November 2, 2022 14:37
@MisterDA MisterDA force-pushed the docker2 branch 2 times, most recently from af61709 to 1dcd93a Compare November 18, 2022 11:15
@MisterDA MisterDA force-pushed the docker2 branch 3 times, most recently from be3dcdd to 805244a Compare January 5, 2023 16:59
lib/docker.mli Outdated
of [head] and [cmd] to be given to Docker command, as Docker
[--entrypoint] takes only one argument. *)

(** Wrappers for various Docker client commands, exposing file descritors. *)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
(** Wrappers for various Docker client commands, exposing file descritors. *)
(** Wrappers for various Docker client commands, exposing file descriptors. *)

let module Builder = Obuilder.Builder(Store)(Native_sandbox)(Obuilder.Docker_extract) in
Native_sandbox.create ~state_dir:(Store.state_dir store / "sandbox") conf >|= fun sandbox ->
let builder = Builder.v ~store ~sandbox in
Builder ((module Builder), builder)
Copy link
Member

Choose a reason for hiding this comment

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

It would be useful to have a stress test for the obuilder on Docker setup.

(** [transform_files ~src_dir ~from_tar ~src_manifest ~dst ~user ~to_untar]
renames the _unique_ file found in [from_tar], a tar archive streamed in
input, to [dst], and writes the resulting tar-format stream to
[to_untar]. All files are listed as being owned by [user]. *)
Copy link
Member

Choose a reason for hiding this comment

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

Add tests for this?

end

(** Wrappers for various Docker client commands. *)
module type DOCKER_CMD = sig
Copy link
Member

Choose a reason for hiding this comment

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

Write documentation comments for this module.

let stderr = Option.value ~default:(`FD_move_safely Os.stderr) stderr in
Os.exec_result ~stdin ~stdout ~stderr ?is_success ~pp cmd

module Cmd = struct
Copy link
Member

Choose a reason for hiding this comment

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

Good to have integration tests for these commands. Run against docker linux, macos and windows.

Comment on lines +280 to +295
let run_result ?stdin ~log ?name ?rm docker_argv image argv =
with_log ~log (fun ~stdout ~stderr ->
Cmd.run_result ?stdin ~stdout ~stderr ?name ?rm docker_argv image argv)

let run_result' ?stdin ?stdout ~log ?name ?rm docker_argv image argv =
with_stderr_log ~log (fun ~stderr ->
Cmd.run_result' ?stdin ?stdout ~stderr ?name ?rm docker_argv image argv)
Copy link
Member

Choose a reason for hiding this comment

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

Why have these almost duplicate functions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want some invocations of Docker to log directly to the build log, but not all. Likewise, some of the logging has to be captured, and some can be ignored.
Granted, this could be a lot cleaner, but my hope is to be able to talk with the Docker daemon with its REST API rather than the command-line client.

include S.BUILDER with type context := Context.t

val v : store:Store.t -> sandbox:Docker_sandbox.t -> t
end
Copy link
Member

Choose a reason for hiding this comment

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

Question here, why is this necessary and not just have a Docker version of S.STORE and S.SANDBOX?

Copy link
Contributor Author

@MisterDA MisterDA Jan 9, 2023

Choose a reason for hiding this comment

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

The problem is that the concept of store and sandbox isn't as tightly decoupled with Docker. There are pieces of info that need sharing in that context that we don't need with runc and fs stores. It was a problem with the build IDs, and when to clean the images and the containers (Docker_sandbox.teardown), which makes the Builder code slightly different. Besides, you can't swap the Docker sandbox or store with a non-Docker sandbox or store while using the other Docker store or sandbox, so it makes some sense to integrate them fully.
Another option would be to write an even more generic Builder to encompass all use cases, but this was rejected when I started writing the backend on the account that it might break the non-Docker builds.

@MisterDA MisterDA force-pushed the docker2 branch 4 times, most recently from 27f2f8d to e6228e4 Compare January 13, 2023 11:25
@MisterDA MisterDA force-pushed the docker2 branch 2 times, most recently from 8d4e204 to 163904e Compare January 17, 2023 13:34
@MisterDA MisterDA force-pushed the docker2 branch 3 times, most recently from 8099bb6 to b04dbcf Compare February 2, 2023 10:13
@MisterDA MisterDA force-pushed the master branch 2 times, most recently from 89e84ac to ccd3f2f Compare February 3, 2023 12:39
@MisterDA MisterDA force-pushed the docker2 branch 2 times, most recently from 525cf43 to add2cde Compare February 13, 2023 16:56
@MisterDA MisterDA force-pushed the docker2 branch 4 times, most recently from 0281700 to 856bb3c Compare March 15, 2023 19:23
@MisterDA MisterDA marked this pull request as ready for review March 15, 2023 22:41
@MisterDA MisterDA force-pushed the docker2 branch 3 times, most recently from 7379de6 to 6038f2a Compare April 7, 2023 16:35
@MisterDA
Copy link
Contributor Author

MisterDA commented Apr 7, 2023

Alea jacta est

@MisterDA MisterDA merged commit 59c4b29 into ocurrent:master Apr 7, 2023
@MisterDA MisterDA deleted the docker2 branch April 7, 2023 17:09
benmandrew added a commit to benmandrew/opam-repository that referenced this pull request Jan 31, 2024
CHANGES:

- Add a Docker backend for Windows and Linux jobs.
  (@MisterDA ocurrent/obuilder#127 ocurrent/obuilder#75, reviewed by @talex5 and @tmcgilchrist)
- Add FreeBSD sandbox backend using jail(8)
  (@dustanddreams ocurrent/obuilder#156 ocurrent/obuilder#174, reviewed by @tmcgilchrist, @MisterDA, and @mtelvers)
- Add Macos ZFS sandbox (@mtelvers ocurrent/obuilder#164, reviewed by @tmcgilchrist)
- Support XFS store (@mtelvers ocurrent/obuilder#170, reviewed by @tmcgilchrist)

- Search for bash rather than assume it lies in /bin (@dustanddreams ocurrent/obuilder#159, reviewed by @tmcgilchrist)
- Prune builds one at a time up to the limit (@mtelvers ocurrent/obuilder#157)
- Specify upper bound on number of items in the store (@mtelvers ocurrent/obuilder#158, reviewed by @MisterDA)
- Fix case where BTRFS is not fully allocated (@mtelvers ocurrent/obuilder#162)
- Avoid pruning parent cache objects (@mtelvers ocurrent/obuilder#176, reviewed by @tmcgilchrist)
nberth pushed a commit to nberth/opam-repository that referenced this pull request Jun 18, 2024
CHANGES:

- Add a Docker backend for Windows and Linux jobs.
  (@MisterDA ocurrent/obuilder#127 ocurrent/obuilder#75, reviewed by @talex5 and @tmcgilchrist)
- Add FreeBSD sandbox backend using jail(8)
  (@dustanddreams ocurrent/obuilder#156 ocurrent/obuilder#174, reviewed by @tmcgilchrist, @MisterDA, and @mtelvers)
- Add Macos ZFS sandbox (@mtelvers ocurrent/obuilder#164, reviewed by @tmcgilchrist)
- Support XFS store (@mtelvers ocurrent/obuilder#170, reviewed by @tmcgilchrist)

- Search for bash rather than assume it lies in /bin (@dustanddreams ocurrent/obuilder#159, reviewed by @tmcgilchrist)
- Prune builds one at a time up to the limit (@mtelvers ocurrent/obuilder#157)
- Specify upper bound on number of items in the store (@mtelvers ocurrent/obuilder#158, reviewed by @MisterDA)
- Fix case where BTRFS is not fully allocated (@mtelvers ocurrent/obuilder#162)
- Avoid pruning parent cache objects (@mtelvers ocurrent/obuilder#176, reviewed by @tmcgilchrist)
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