conan create remote WIP #15856

davidsanfal · 2024-03-12T08:46:19Z

Changelog: Feature: Added transparent support for running Conan within a Docker container.
Docs: conan-io/docs#3699

Refer to the issue that supports this Pull Request.
If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
I've read the Contributing guide.
I've followed the PEP8 style guides for Python code.
I've opened another PR in the Conan docs repo to the develop branch, documenting this one.

conan/internal/runner/__init__.py

jcar87 · 2024-03-13T13:44:09Z

conans/requirements_dev.txt

@@ -4,3 +4,4 @@ parameterized>=0.6.3
 mock>=1.3.0, <1.4.0
 WebTest>=2.0.18, <2.1.0
 bottle
+docker


the docker pypi package is pure python, but it's worth checking whether all the dependencies are also pure python and to estimate the likelihood of dependency conflicts in the future.

otherwise I wonder if we may want to consider making it an optional dependency?

pip install conan[docker]

jcar87

a few comments :D but this is amazing

jcar87 · 2024-03-13T13:44:35Z

conans/test/integration/command/dockerfiles/Dockerfile

@@ -0,0 +1,7 @@
+FROM ubuntu


if it's forst tests I would do FROM ubuntu:jammy or a specific LTS

jcar87 · 2024-03-13T13:47:46Z

conans/test/integration/command/dockerfiles/Dockerfile

+RUN apt update && apt upgrade -y
+RUN apt install -y build-essential
+RUN apt install -y python3-pip cmake git build-essential
+RUN pip install docker


each RUN line is cached independently, this Dockerfile is only guaranteed to build properly from a clean cache (or a --no-cache flag when building)

Remember that apt install does the dependency resolution against the cached outcome of apt update ) - we want to avoid running an apt install against an outdated cache (it will fail, since the apt remotes delete versions quite quickly)

Recommended way is something like:

RUN apt-get update \ && DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends \ build-essential \ cmake \ ninja-build \ python3 \ python3-venv # && rm -rf /var/lib/apt/lists/*

The && guarantees that apt-get update and apt-get install are part of the same RUN command, and the last line ensures that the apt cache is removed as part of the same command: it is outdated almost by definition.

jcar87 · 2024-03-13T13:48:12Z

conans/test/integration/command/dockerfiles/Dockerfile

+RUN apt install -y python3-pip cmake git build-essential
+RUN pip install docker
+COPY . /root/conan-io
+RUN cd /root/conan-io && pip install -e .


IIRC newer versions of Python in ubuntu will complain when pip is invoked at the system level (for the better) - I'd install it inside a virtual environment

jcar87 · 2024-03-13T13:57:47Z

conans/test/integration/command/runner_test.py

+    type=docker
+    dockerfile={dockerfile_path("Dockerfile_test")}
+    docker_build_path={conan_base_path()}
+    image=conan-runner-default-test


My intuition here... but having both dockerfile and image strike me as contradictory? I would expect to have one or the other:

My expectation:

if we have an image, assume it exists and launch a container from that image

if we are given a dockerfile - build the dockerfile and launch a container from the resulting image, I would not expect to pass an image - I would expect this to be internal to conan to decide what the image is called

If you only define the dockerfile, it will generate the image with the default name. If you have several profiles with dockerfiles, you can have a mechanism that doesn't overwrite the images and not need to rebuild them when changing profiles.
In addition, it assumes that the image exists if you only define the name of the image.

jcar87 · 2024-03-13T13:58:07Z

conans/test/integration/command/runner_test.py

+    docker_build_path={conan_base_path()}
+    image=conan-runner-default-test
+    cache=copy
+    remove=True


what's the meaning of remove?

remove the container at the end of the conan command execution

jcar87 · 2024-03-13T14:00:49Z

conans/test/integration/command/runner_test.py

+    [runner]
+    type=docker
+    dockerfile={dockerfile_path("Dockerfile_test")}
+    docker_build_path={conan_base_path()}


I would suggest to rename this to docker_build_context or similar - and document:

what the default behaviour is when this is not provided

what it is relative to, if a relative path is provided (if it's even possible)

(assuming that it will work as expected if a full path is provided, but that tends to be tricky when this is expressed in distributable config files)

czoido · 2024-03-13T16:49:52Z

conans/test/integration/command/runner_test.py

+    return os.path.dirname(os.path.dirname(conans.__file__))
+
+
+def dockerfile_path(name=None):


One minor comment regarding the test style. I think it would be better to declare the file as a string here (or import the string from a common place once many tests in different places use it) and create the file using client.save(), then you can access the path with client.current_folder...

this is a static path inside the conan test folder.
For example: conans/test/integration/command/dockerfiles/Dockerfile

conans/test/integration/command/runner_test.py

jcar87 · 2024-03-19T18:09:22Z

conan/internal/runner/docker.py

+        cwd = os.getcwd()
+        if profiles:
+            for profile in profiles:
+                profile_path = ProfileLoader.get_profile_path(os.path.join(ConfigAPI(self.conan_api).home(), 'profiles'), profile, cwd)


I could be wrong here - if we are using the profile name, rather than the profile already "composed" by the create command - this may result in an incomplete thing being copied if the profile has a include() referring to other files - we are only going to copy the referenced file, rather than anything else it might need.

if we propagate the resolved profile from the conan create command - we can just do a profile.dumps() and save it in the container - that should resolve includes(), but I suspect it will also render jinja templates and the likes

a reason this is important is that we may have a profile like this:

include(windows-msvc193) [runner] type=ssh host=my-remote-host

and then the file windows-msvc193 is a different profile file that doesn't have any [runner] related things. I expect devs to operate like this to avoid repetition

@memsharded may be able to provide more context here : D

This is correct, I initially discussed with @davidsanfal that we might want to transfer the files, because of the same reason, there would be files that would evaluate differently in different machines due to jinja rendering.

But it is totally true that the include() would be a limitation and fail unexpectedly. We need to think about it.

RubenRBS · 2024-04-25T19:52:58Z

Just noting down here that we had excellent used feeback on the addition of this feature on the Using cpp::std conference after we showed it in our talk! 🥳

memsharded · 2024-04-29T10:14:33Z

conan/internal/runner/docker.py

+        except:
+            raise ConanException("Docker Client failed to initialize."
+                                 "\n - Check if docker is installed and running"
+                                 "\n - Run 'pip install docker>=5.0.0, <=5.0.3'")


Better pip isntall ... [runners]?

memsharded · 2024-04-29T10:15:26Z

conan/cli/commands/create.py

+            'docker': DockerRunner,
+            'ssh': SSHRunner,
+            'wsl': WSLRunner,
+        }[profile_host.runner.get('type')](conan_api, 'create', profile_host, profile_build, args, raw_args).run()


A bit too much, I wouldn't mind something in 3 separate lines, and also something that raise a controlled ConanException if the runner.type is not defined or incorrect.

conan/internal/runner/docker.py

memsharded · 2024-04-29T10:20:25Z

conans/test/integration/command/runner_test.py

+    import os
+    from conan import ConanFile
+    from conan.tools.cmake import CMake, CMakeToolchain
+
+    class Library(ConanFile):
+        name = "hello"
+        version = "1.0"


Is there any reason not to use a standard conan new cmake_lib template? That would be better.
The Ninja generator can be defined in the profile if necessary.

memsharded · 2024-04-29T10:22:03Z

conans/test/integration/command/dockerfiles/Dockerfile_ninja

+FROM ubuntu:22.04
+RUN apt-get update \
+    && DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends \
+        build-essential \
+        cmake \
+        ninja-build \
+        python3 \
+        python3-pip \
+        python3-venv \


If the 3 dockerfiles are very similar, wouldn't it make sense to define a common base one? that would make tests that use these dockerfiles faster to build locally, isn't it? test speed is important to keep as fast as possible.

It could be done via multi stage feature: https://docs.docker.com/build/building/multi-stage/

memsharded · 2024-04-29T10:24:22Z

conans/model/profile.py

@@ -24,6 +24,7 @@ def __init__(self):
        self.conf = ConfDefinition()
        self.buildenv = ProfileEnvironment()
        self.runenv = ProfileEnvironment()
+        self.runner = {}


It seems the Profile might need to add serialize() and dumps() for full management of the runners information

memsharded · 2024-04-29T10:25:07Z

conan/internal/runner/wsl.py

+            self.args.profile_host[0]: self.host_profile.dumps(),
+            self.args.profile_build[0]: self.build_profile.dumps()


Profile dumps() doesn't include the runners information now, but it might.

RubenRBS

Looks great, I have some comments about errors I found while testing this locally

conan/cli/commands/create.py

conan/internal/runner/docker.py

conan/cli/commands/create.py

conan/internal/runner/docker.py

Co-authored-by: Rubén Rincón Blanco <rubenrb@jfrog.com>

RubenRBS

🥳

conan/internal/runner/__init__.py

czoido · 2024-05-06T13:41:47Z

conan/internal/runner/docker.py

+        self.image = host_profile.runner.get('image') or self.configfile.image
+        if not (self.dockerfile or self.image):
+            raise ConanException("'dockerfile' or docker image name is needed")
+        self.image = self.image or 'conan-runner-default'


Maybe not for this PR, but suggesting here that we make the default image name configurable with something like core.runners.docker.default_image_name

czoido · 2024-05-06T13:51:01Z

conan/internal/runner/docker.py

+        self.dockerfile = host_profile.runner.get('dockerfile') or self.configfile.build.dockerfile
+        self.docker_build_context = host_profile.runner.get('build_context') or self.configfile.build.build_context
+        self.image = host_profile.runner.get('image') or self.configfile.image


I find the preference order in selecting from the profile or the config file a bit unnatural, because you can set in the profile contradictory things, like for example using a configfile that points to a dockerfile and be pointing in the profile to another one, shouldn't the configfile and the profile settings that overlap be incompatible in some way?

davidsanfal requested review from czoido and memsharded March 12, 2024 08:46

davidsanfal mentioned this pull request Mar 12, 2024

conan create remote WIP #15768

Closed

5 tasks

czoido reviewed Mar 12, 2024

View reviewed changes

conan/internal/runner/__init__.py Outdated Show resolved Hide resolved

davidsanfal force-pushed the feature/docker_wrapper branch 2 times, most recently from 69b9720 to 9a2a05a Compare March 13, 2024 08:24

jcar87 reviewed Mar 13, 2024

View reviewed changes

czoido reviewed Mar 13, 2024

View reviewed changes

conans/test/integration/command/runner_test.py Show resolved Hide resolved

davidsanfal closed this Mar 13, 2024

davidsanfal reopened this Mar 13, 2024

RubenRBS mentioned this pull request Mar 19, 2024

[feature] Add command to query *why* a package is a dependency #15837

Closed

1 task

jcar87 reviewed Mar 19, 2024

View reviewed changes

memsharded self-assigned this Mar 20, 2024

memsharded added this to the 2.3.0 milestone Mar 20, 2024

davidsanfal force-pushed the feature/docker_wrapper branch 4 times, most recently from 2c50c14 to e31be33 Compare March 22, 2024 08:42

davidsanfal force-pushed the feature/docker_wrapper branch 2 times, most recently from db89115 to 84ef97d Compare April 19, 2024 08:45

davidsanfal added 8 commits April 19, 2024 11:38

conan create remote WIP

7073730

runners added

d852ca8

docker runner updated

acc000c

docker runner updated

284e62e

abs path added

9175aa6

linux slashes fixed

03d53ca

info updated

559428b

restore only new packages

62de035

davidsanfal force-pushed the feature/docker_wrapper branch from a81379f to 07232cd Compare April 23, 2024 08:14

davidsanfal marked this pull request as ready for review April 23, 2024 13:39

davidsanfal requested review from jcar87 and czoido April 23, 2024 13:39

memsharded reviewed Apr 29, 2024

View reviewed changes

davidsanfal added 2 commits April 29, 2024 18:28

configfile docker fixed

8056359

wip

6a44fbf

davidsanfal force-pushed the feature/docker_wrapper branch from 3803ac5 to 6a44fbf Compare April 30, 2024 14:58

davidsanfal added 3 commits May 3, 2024 11:29

docker runner test added

f151c3b

ssh and wsl disabled

a54ecca

print remove

6e26f78

memsharded approved these changes May 5, 2024

View reviewed changes

RubenRBS self-requested a review May 5, 2024 22:20

RubenRBS reviewed May 6, 2024

View reviewed changes

RubenRBS mentioned this pull request May 6, 2024

conan runner docs added conan-io/docs#3699

Merged

davidsanfal and others added 4 commits May 6, 2024 09:54

Update conan/cli/commands/create.py

a6e1799

Co-authored-by: Rubén Rincón Blanco <rubenrb@jfrog.com>

Update conan/cli/commands/create.py

fa8ab58

Co-authored-by: Rubén Rincón Blanco <rubenrb@jfrog.com>

Update conan/internal/runner/docker.py

2977c64

Co-authored-by: Rubén Rincón Blanco <rubenrb@jfrog.com>

docker runner tests with default profile added

fb6bab8

RubenRBS approved these changes May 6, 2024

View reviewed changes

franramirez688 reviewed May 6, 2024

View reviewed changes

conan/internal/runner/__init__.py Show resolved Hide resolved

czoido assigned czoido and unassigned memsharded May 6, 2024

czoido reviewed May 6, 2024

View reviewed changes

czoido approved these changes May 6, 2024

View reviewed changes

uilianries approved these changes May 6, 2024

View reviewed changes

czoido merged commit bd60587 into develop2 May 6, 2024
2 checks passed

czoido deleted the feature/docker_wrapper branch May 6, 2024 14:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

conan create remote WIP #15856

conan create remote WIP #15856

davidsanfal commented Mar 12, 2024 •

edited by czoido

jcar87 Mar 13, 2024

jcar87 left a comment

jcar87 Mar 13, 2024

jcar87 Mar 13, 2024

jcar87 Mar 13, 2024

jcar87 Mar 13, 2024

davidsanfal Mar 13, 2024

jcar87 Mar 13, 2024

davidsanfal Mar 13, 2024

jcar87 Mar 13, 2024

czoido Mar 13, 2024 •

edited

davidsanfal Mar 15, 2024

jcar87 Mar 19, 2024

memsharded Mar 19, 2024

RubenRBS commented Apr 25, 2024

memsharded Apr 29, 2024

memsharded Apr 29, 2024

memsharded Apr 29, 2024

memsharded Apr 29, 2024

uilianries May 6, 2024

memsharded Apr 29, 2024

memsharded Apr 29, 2024

RubenRBS left a comment

RubenRBS left a comment

czoido May 6, 2024

czoido May 6, 2024 •

edited

		return os.path.dirname(os.path.dirname(conans.__file__))


		def dockerfile_path(name=None):

		self.args.profile_host[0]: self.host_profile.dumps(),
		self.args.profile_build[0]: self.build_profile.dumps()

conan create remote WIP #15856

conan create remote WIP #15856

Conversation

davidsanfal commented Mar 12, 2024 • edited by czoido

Choose a reason for hiding this comment

jcar87 left a 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

czoido Mar 13, 2024 • edited

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

RubenRBS commented Apr 25, 2024

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

RubenRBS left a comment

Choose a reason for hiding this comment

RubenRBS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

czoido May 6, 2024 • edited

Choose a reason for hiding this comment

davidsanfal commented Mar 12, 2024 •

edited by czoido

czoido Mar 13, 2024 •

edited

czoido May 6, 2024 •

edited