add remotes in constructor#359
Conversation
|
Alternative fix to #331. Instead of delaying loading of package recipe (done in this pull request), we eagerly add all remotes in packager's constructor. BTW, I don't know how to test the fact that |
grisumbras
force-pushed
the
add-remotes-early
branch
from
e1b8c75 to
cb01a20
Compare
|
@lasote any thoughts? |
|
Could this fix the issue? https://github.com/conan-io/conan-package-tools/pull/379/files |
|
Tried the branch locally, and it appears that it does not. |
| self._exclude_vcvars_precommand = exclude_vcvars_precommand | ||
| self._build_policy = build_policy | ||
| self._runner = PrintRunner(runner or os.system, self.printer) | ||
| self._uploader.remote_manager.add_remotes_to_conan() |
There was a problem hiding this comment.
I think this is going to break something when running docker. Have you tried?
There was a problem hiding this comment.
No, I haven't. Also, currently I am basically in the middle of nowhere, so I can't test it now.
There was a problem hiding this comment.
Indeed, it will. I see these ways to fix this:
- Add remotes both in packager's and in runner's constructors. This will print a message about remote already existing for each remote if not running jobs in Docker.
- Same as 1, but add a flag to
RemotesManager, which is set and checked inadd_remotes_to_conan. - Add remotes in
RemotesManager's constructor. - Add remotes in packager's constructor and in
cpt.run_in_docker:run.
What is the most desired approach? Personally, I like approach 2 the most.
| reference="lib/1.0@lasote/mychannel", | ||
| ci_manager=self.ci_manager) | ||
|
|
||
| builder.add({}, {}, {}, {}) |
There was a problem hiding this comment.
This tests if remotes are added. Currently, remotes are added when the jobs are being run. With this change remotes are added before that, so there's no point in adding any jobs.
lasote
left a comment
lasote
left a comment
There was a problem hiding this comment.
Sorry for the delay, I've been taking a look and maybe we are mostly OK as it is this PR. Because for docker, there is no further need to load the conanfile class before running the build.
|
Many thanks by the way!!! |
|
Thanks for merging. |
2 participants
Changelog: Bugfix: Fixes the usage of python requires.