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

Binary registry #3726

Merged
merged 19 commits into from Oct 18, 2018

Conversation

Projects
None yet
3 participants
@lasote
Copy link
Contributor

commented Oct 11, 2018

Changelog: Fix: The remote used to download a binary package is now stored, so any update for the specific binary will come from the right remote.

Changelog: Feature: New conan remote list_pref/add_pref/remove_pref/update_pref commands added to manage the new Registry entries for binary packages.

Docs: conan-io/docs#902

@lasote lasote added this to the 1.9 milestone Oct 11, 2018

@lasote lasote requested review from jgsogo and memsharded Oct 11, 2018

@ghost ghost assigned lasote Oct 11, 2018

@ghost ghost added the stage: review label Oct 11, 2018

@jgsogo
Copy link
Member

left a comment

This is more me asking than a review, but I have some doubts and I need more information before having a deeper look into the execution flow.

Thanks!

self._registry.refs.set(conan_ref, upload_remote.name)

if (not cur_recipe_remote or not remote_name) and policy != UPLOAD_POLICY_SKIP:
self._registry.refs.set(conan_ref, recipe_remote.name)

This comment has been minimized.

Copy link
@jgsogo

jgsogo Oct 11, 2018

Member

I would think that the registry should be updated just after uploading the recipe (line 103)

And also, I don't know why it is conditioned on not cur_recipe_remote or not remote_name (same comment as the one regarding packages).

This comment has been minimized.

Copy link
@lasote

lasote Oct 16, 2018

Author Contributor

Agree about moving the registry update.
I think the condition can be simplified to not cur_recipe_remote, the same for packages. Let me check. The idea is: If something has a registry entry, then an upload won't change it.

policy, p_remote)

if (not cur_package_remote or not remote_name) and policy != UPLOAD_POLICY_SKIP:
self._registry.prefs.set(pref, p_remote.name)

This comment has been minimized.

Copy link
@jgsogo

jgsogo Oct 11, 2018

Member

I would want to know why it is conditioned to not cur_package_remote or not remote_name... At first, I expected the registry to be updated unconditionally if the package was uploaded.

This comment has been minimized.

Copy link
@lasote

lasote Oct 16, 2018

Author Contributor

This is an internal decision (probably arbitrary), that if you are installing from R1 but uploading to R2, you want the updates still from R1, otherwise you need to change the reference manually.

Show resolved Hide resolved conans/client/command.py
@@ -827,6 +827,32 @@ def remote_update_ref(self, reference, remote_name):
reference = ConanFileReference.loads(str(reference))
return self._registry.refs.update(reference, remote_name)

@api_method
def remote_list_pref(self, reference):
reference = ConanFileReference.loads(str(reference))

This comment has been minimized.

Copy link
@jgsogo

jgsogo Oct 11, 2018

Member

Add validation, it is an user input: ConanFileReference.loads(str(reference), validate=True).... in the future that argument should be False by default.

if conan_reference in refs:
raise ConanException("%s already exists. Use update" % conan_reference)
if ref in refs:
raise ConanException("%s already exists. Use update" % ref)
if remote_name not in remotes:
raise ConanException("%s not in remotes" % remote_name)

This comment has been minimized.

Copy link
@jgsogo

jgsogo Oct 11, 2018

Member

This check should be done outside the check_exists condition, as it is done in the update method too.

@@ -176,9 +198,8 @@ def exists_function(remotes):
self._add_update(remote_name, url, verify_ssl, exists_function, insert)

def rename(self, remote_name, new_remote_name):

This comment has been minimized.

Copy link
@jgsogo

jgsogo Oct 11, 2018

Member

(Not related to this PR, but) what if new_remote_name already exists?

This comment has been minimized.

Copy link
@lasote

lasote Oct 16, 2018

Author Contributor

I'll add a check

Show resolved Hide resolved conans/client/command.py
@@ -36,6 +36,7 @@ def _get_recipe(self, reference, check_updates, update, remote_name, recorder):
if not os.path.exists(conanfile_path):
remote, new_ref = self._download_recipe(reference, output, remote_name, recorder)
status = RECIPE_DOWNLOADED
self._registry.refs.set(reference, remote.name)

This comment has been minimized.

Copy link
@memsharded

memsharded Oct 15, 2018

Contributor

This is done already in self._download_recipe. Remove from that function?

@@ -83,68 +85,88 @@ def __init__(self, filename, lockfile, output):
self._filename = filename
self._lockfile = lockfile
self._output = output
self._remotes = None
# Cache json contents
self._contents = ""

This comment has been minimized.

Copy link
@memsharded

memsharded Oct 15, 2018

Contributor

Dangerous. The contents can be changed by a concurrent process.

This comment has been minimized.

Copy link
@lasote

lasote Oct 16, 2018

Author Contributor

Ok, I'll remove the cache

def get(self, conan_reference):
assert(isinstance(conan_reference, ConanFileReference))
def get(self, ref):
assert(isinstance(ref, ConanFileReference) or isinstance(ref, PackageReference))

This comment has been minimized.

Copy link
@memsharded

memsharded Oct 15, 2018

Contributor

isinstance(ref, (ConanFileReference, PackageRefererence))

if str(conan_reference) not in refs:
raise ConanException("%s does not exist. Use add" % str(conan_reference))
remotes, rrefs, prefs = self._load()
refs = self._select_refs(rrefs, prefs)

This comment has been minimized.

Copy link
@memsharded

memsharded Oct 15, 2018

Contributor

This _select_refs reads a bit tricky to me. I think I would have 2 different update(), one in each child class, even if there is some duplications.

This comment has been minimized.

Copy link
@lasote

lasote Oct 16, 2018

Author Contributor

Is not only the update but all the methods. I would try to think a different approach, if I move all to different subclasses the duplicated code will be the 99%


conanfile_path = self._client_cache.conanfile(conan_ref)
# FIXME: I think it makes no sense to specify a remote to "pre_upload"
# FIXME: because the recipe can have one and the package a different one

This comment has been minimized.

Copy link
@memsharded

memsharded Oct 15, 2018

Contributor

It depends on what will be the final model. If one upload of a package can only upload to a given remote, or is able to upload different binaries to different remotes, in the same command. To discuss.

@@ -111,18 +111,32 @@ def _upload(self, conan_file, conan_ref, packages_ids, retry, retry_wait,
"no packages can be uploaded")
total = len(packages_ids)
for index, package_id in enumerate(packages_ids):
ret_upload_package = self._upload_package(PackageReference(conan_ref, package_id),
pref = PackageReference(conan_ref, package_id)
cur_package_remote = self._registry.prefs.get(pref)

This comment has been minimized.

Copy link
@memsharded

memsharded Oct 15, 2018

Contributor

Not sure about this, this is changing current behavior, and it will be again changed in conan 2.0. Probably remove this.

This comment has been minimized.

Copy link
@lasote

lasote Oct 16, 2018

Author Contributor

Why is changing in Conan 2.0?

This comment has been minimized.

Copy link
@lasote

lasote Oct 16, 2018

Author Contributor

Anyway, I will change it to choose the specified remote or the default one. Ok?

@lasote

This comment has been minimized.

Copy link
Contributor Author

commented Oct 16, 2018

Reviewed with your comments. Now the pref is only used for the install --update (for the upload it will follow the used recipe remote). Added more tests to verify the behavior.

lasote added some commits Oct 18, 2018

lf

@memsharded memsharded merged commit 34ac878 into conan-io:develop Oct 18, 2018

2 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
license/cla Contributor License Agreement is signed.
Details

@ghost ghost removed the stage: review label Oct 18, 2018

grisumbras pushed a commit to grisumbras/conan that referenced this pull request Dec 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.