Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

conan create remote WIP #15768

Closed
wants to merge 14 commits into from
Closed

Conversation

davidsanfal
Copy link
Contributor

Changelog: (Feature | Fix | Bugfix): Describe here your pull request
Docs: https://github.com/conan-io/docs/pull/XXXX

  • 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.

@CLAassistant
Copy link

CLAassistant commented Feb 28, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

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

Looks good, some minor comments
(I still need to understand better the integration)

Dockerfile Outdated Show resolved Hide resolved
conan/cli/commands/create.py Outdated Show resolved Hide resolved
conan/cli/commands/create.py Outdated Show resolved Hide resolved
conan/cli/commands/create.py Outdated Show resolved Hide resolved
conans/client/profile_loader.py Outdated Show resolved Hide resolved
conans/client/profile_loader.py Outdated Show resolved Hide resolved
@davidsanfal davidsanfal force-pushed the develop2 branch 7 times, most recently from 7cee9eb to f897344 Compare March 4, 2024 17:19
@davidsanfal davidsanfal force-pushed the develop2 branch 2 times, most recently from 6513c14 to dca0d5a Compare March 6, 2024 17:49
@davidsanfal davidsanfal force-pushed the develop2 branch 8 times, most recently from 0e9bba7 to d7fba39 Compare March 7, 2024 08:41
Copy link
Member

@RubenRBS RubenRBS left a comment

Choose a reason for hiding this comment

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

Nice! Some initial comments and questions/doubts, but overall looks great :)

except StopIteration:
pass
else:
return exec_run.output.decode('utf-8', errors='ignore').strip()
Copy link
Member

Choose a reason for hiding this comment

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

Does this already get printed if stream is set to false elsewhere? Or is the idea to have that case be silent?

conan/internal/runner/docker.py Show resolved Hide resolved
self.run_command('conan list --graph=create.json --graph-binaries=build --format=json > pkglist.json')
self.run_command('conan cache save --list=pkglist.json --file "'+self.abs_docker_path+'"/.conanrunner/docker_cache_save.tgz')
tgz_path = os.path.join(self.abs_runner_home_path, 'docker_cache_save.tgz')
docker_info(f'Restore host cache from: {tgz_path}')
Copy link
Member

Choose a reason for hiding this comment

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

Looks good 👍!

@davidsanfal davidsanfal force-pushed the develop2 branch 3 times, most recently from 7ab9bd1 to 65ba41d Compare March 11, 2024 10:57
Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

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

Creating another feature/docker_wrapper branch helps a bit to checkout and work on your branch

Already discussed with @davidsanfal some points, but submitting to share with others too

@@ -7,3 +7,4 @@ fasteners>=0.15
distro>=1.4.0, <=1.8.0; sys_platform == 'linux' or sys_platform == 'linux2'
Jinja2>=3.0, <4.0.0
python-dateutil>=2.8.0, <3
docker>=5.0.0, <=5.0.3
Copy link
Member

Choose a reason for hiding this comment

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

I am concerned about this: https://github.com/docker/docker-py/blob/main/requirements.txt, for example pinning the requests version to an exact one.

Adding extra dependencies to Conan has always been fragile, interacting with other user dependencies, etc.
I'd explore the possibility to make this opt-in

@@ -55,6 +58,12 @@ def create(conan_api, parser, *args):
args.build_require)

print_profiles(profile_host, profile_build)
if profile_build.runner and not os.environ.get("CONAN_RUNNER_ENVIRONMENT"):
Copy link
Member

Choose a reason for hiding this comment

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

This should be profile_host, not profile build. We want to apply this runner to the "host" packages, not to the "build" context packages (most likely we want to apply to both)

@davidsanfal
Copy link
Contributor Author

davidsanfal commented Mar 12, 2024

@memsharded Moved to #15856

@davidsanfal davidsanfal reopened this Mar 13, 2024
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.

None yet

4 participants