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

Package Revisions #3055

Merged
merged 223 commits into from Nov 23, 2018

Conversation

4 participants
@lasote
Copy link
Contributor

commented Jun 14, 2018

  • ApiV2 in one step downloads-uploads.
  • Revisions server/client implemented
  • CI for v2 blocked/apiv2 with revisions compatibility mode/full revisions

By default v2 is still blocked.

  • Pending to implement the revisions endpoint (to show revisions & timestamps) but I think it could be done later, as it a "nice to have" only.

@ghost ghost assigned lasote Jun 14, 2018

@ghost ghost added the stage: review label Jun 14, 2018

@lasote

This comment has been minimized.

Copy link
Contributor Author

commented Nov 7, 2018

CI is green. Team, review again, approve or disapprove ;)

Show resolved Hide resolved conans/server/migrations.py Outdated

lasote added some commits Nov 8, 2018

Show resolved Hide resolved conans/client/client_cache.py
Show resolved Hide resolved conans/client/cmd/uploader.py
@@ -88,6 +88,14 @@ def __cmp__(self, other):

if self.conan_ref == other.conan_ref:
return 0

This comment has been minimized.

Copy link
@jgsogo

jgsogo Nov 8, 2018

Member

https://portingguide.readthedocs.io/en/latest/comparisons.html#rich-comparisons

The __cmp__() special method is no longer honored in Python 3.

It seems we are not ordering Node instances, or it should be failing in Py3, so... why did you need to add this here? 😕

This comment has been minimized.

Copy link
@lasote

lasote Nov 8, 2018

Author Contributor
def print_graph(deps_graph, out):
    requires = OrderedDict()
    build_requires = OrderedDict()
    python_requires = set()
    for node in sorted(deps_graph.nodes):

This comment has been minimized.

Copy link
@lasote

lasote Nov 8, 2018

Author Contributor

And actually it failed in Python3 (CI) without these changes.... so WTF?

This comment has been minimized.

Copy link
@jgsogo

jgsogo Nov 20, 2018

Member

Any news on this? Just curious...

This comment has been minimized.

Copy link
@lasote

lasote Nov 21, 2018

Author Contributor

nope.... it is working and used with py3

Show resolved Hide resolved conans/client/graph/graph_builder.py
Show resolved Hide resolved conans/client/recorder/action_recorder.py
@memsharded
Copy link
Contributor

left a comment

Partial review (up to graph_builder.py). Prefer to submit it partially, so you have it soon.
Will keep reviewing tomorrow.

Show resolved Hide resolved conans/client/client_cache.py Outdated
Show resolved Hide resolved conans/client/cmd/export.py Outdated
Show resolved Hide resolved conans/client/cmd/uploader.py Outdated
Show resolved Hide resolved conans/client/cmd/uploader.py Outdated
Show resolved Hide resolved conans/client/conan_api.py Outdated
Show resolved Hide resolved conans/client/graph/graph_binaries.py Outdated

lasote added some commits Nov 21, 2018

@jgsogo
Copy link
Member

left a comment

Sending some comments, will continue later on

Show resolved Hide resolved conans/test/utils/tools.py Outdated
dumped = rev.dumps()
loaded = RevisionList.loads(dumped)
self.assertEquals(rev, loaded)
self.assertEquals(loaded.latest_revision().revision, "rev2")

This comment has been minimized.

Copy link
@jgsogo

jgsogo Nov 21, 2018

Member

The latest revision is about the lexicographic order or which one was added before or after? Please change the test so it is not the same order.

This comment has been minimized.

Copy link
@lasote

lasote Nov 22, 2018

Author Contributor

I don't know what you mean. I'm testing removing the latest element in the list and checking the "new latest". I've added another test removing not the latest but I don't understand your comment.

This comment has been minimized.

Copy link
@jgsogo

jgsogo Nov 23, 2018

Member

I mean, if the latest one is rev2 because it was added to the list after, or it is the last one because rev2 > rev1 in the lexicographic order.

Show resolved Hide resolved conans/test/remote/auth_bearer_test.py Outdated
Show resolved Hide resolved conans/test/model/ref_test.py
Show resolved Hide resolved conans/test/model/package_metadata_test.py
Show resolved Hide resolved conans/test/model/package_metadata_test.py
self.client.save({"conanfile.py": conanfile})
self.client.run("export . %s " % str(self.ref))
rev = self.client.get_revision(self.ref)
self.assertNotEqual(rev, "fixed_revision")

This comment has been minimized.

Copy link
@jgsogo

jgsogo Nov 21, 2018

Member

After reading this test I need some clarification.

  1. scm["revision"] will be the revision of the recipe?
  2. Why is changing the recipe revision so self.assertNotEqual(rev, "fixed_revision")? Because SCM is changing url and revision if one of them is set to auto? (from my point of view, this behavior is more a bug than a feature).

This comment has been minimized.

Copy link
@lasote

lasote Nov 22, 2018

Author Contributor
  1. No, because it is not "auto"
  2. Not a bug, very intended, because if you have a fixed commit, for example when using the scm feature to clone a third party of a third party (valid use case) instead of using the source() method, the commit is not from your recipe but from the source code. So only when auto the revision is taken from the scm.
@memsharded
Copy link
Contributor

left a comment

Reviewed implementation, but not tests yet.
Looking good, we are getting closer!


@revision.setter
def revision(self, r):
self._revision = DEFAULT_REVISION_V1 if r is None else r

This comment has been minimized.

Copy link
@memsharded

memsharded Nov 21, 2018

Contributor

Won't this be a bit confusing?

metadata.recipe.revision = None  
print metadata.recipe.revision #will be 0

This comment has been minimized.

Copy link
@lasote

lasote Nov 22, 2018

Author Contributor

Do you prefer to store a None and read a 0? I don't.

if not latest:
raise NotFoundException("Recipe not found: '%s'" % reference.full_repr())

return reference.copy_with_rev(latest.revision)

This comment has been minimized.

Copy link
@memsharded

memsharded Nov 21, 2018

Contributor

This copy I don't understand. Why not directly return latest?

This comment has been minimized.

Copy link
@lasote

lasote Nov 22, 2018

Author Contributor

latest is a RevisionEntry object, not a reference.

def _conflicting_references(ref1, ref2):
if ref1.copy_clear_rev() != ref2.copy_clear_rev():
return 1
if ref1.revision and ref2.revision and ref1.revision != ref2.revision:

This comment has been minimized.

Copy link
@memsharded

memsharded Nov 21, 2018

Contributor

Is it possible that one ref has revision and the other does not? Isn't it a conflict? Why the other won't have a revision?

This comment has been minimized.

Copy link
@lasote

lasote Nov 22, 2018

Author Contributor

Actually ref1.revision shouldn't be None because that is the previous computed node. I'm changing some var names, introducing an assert and changing the comparison.
The idea is: The new revision (ref2.revision) can't be different from the previous one, but could be None without a conflict, when you write an override for example, ref2 is the override and is not processed yet, and it shouldn't conflict with the previously resolved ref1.revision.

This comment has been minimized.

Copy link
@lasote

lasote Nov 22, 2018

Author Contributor

Now is never None because it comes also from v1 with "0".
This is how it looks now:

    @staticmethod
    def _conflicting_references(previous_ref, new_ref):
        if previous_ref.copy_clear_rev() != new_ref.copy_clear_rev():
            return REFERENCE_CONFLICT
        # Computed node, has to have a revision, at least 0
        assert(previous_ref.revision is not None)
        # If new_ref.revision is None we cannot assume any conflict, the user hasn't specified
        # a revision, so it's ok any previous_ref
        if new_ref.revision and previous_ref.revision != new_ref.revision:
            return REVISION_CONFLICT
        return False
def _recurse(self, closure, new_reqs, new_options):
""" For a given closure, if some requirements or options coming from downstream
is incompatible with the current closure, then it is necessary to recurse
then, incompatibilities will be raised as usually"""
for req in new_reqs.values():
n = closure.get(req.conan_reference.name)
if n and n.conan_ref != req.conan_reference:
if n and n.conan_ref.copy_clear_rev() != req.conan_reference.copy_clear_rev():

This comment has been minimized.

Copy link
@memsharded

memsharded Nov 21, 2018

Contributor

Here, the comparison should use the full Revision. Cause it needs to conflict upstream if it is different too.

This comment has been minimized.

Copy link
@lasote

lasote Nov 22, 2018

Author Contributor

I changed it with the same conflict comparission. review please


# Check if we have a revision different from the requested one
revisions_enabled = get_env("CONAN_CLIENT_REVISIONS_ENABLED", False)
if revisions_enabled and reference.revision and cur_revision != reference.revision:

This comment has been minimized.

Copy link
@memsharded

memsharded Nov 21, 2018

Contributor

I am not sure of this behavior. So if another revision rather than the installed one is requested, it will only use the remote from which the previous one was retrieved? I understand that is the current behavior, right? Maybe just something to think about for 2.0

This comment has been minimized.

Copy link
@lasote

lasote Nov 22, 2018

Author Contributor

we can decide in this case if we want to change it. it is only in case of revisions, so no breaking changes.

metadata.recipe.time = rev_time

revisions_enabled = get_env("CONAN_CLIENT_REVISIONS_ENABLED", False)
if revisions_enabled and policy in (UPLOAD_POLICY_NO_OVERWRITE,

This comment has been minimized.

Copy link
@memsharded

memsharded Nov 21, 2018

Contributor

Print it before the real upload?

This comment has been minimized.

Copy link
@lasote

lasote Nov 22, 2018

Author Contributor

Actually, I'm removing the warning. The check is not correct. The conan_reference is not the returned by the remote but the used in the upload. I don't want to introduce more source code here, so we can introduce again the warning message in the future if it is needed/requested.


# Update package revision with the rev_time (Created locally but with rev_time None)
with self._client_cache.update_metadata(new_pref.conan) as metadata:
metadata.packages[new_pref.package_id].revision = new_pref.revision

This comment has been minimized.

Copy link
@memsharded

memsharded Nov 21, 2018

Contributor

This is a bit confusing. If we uploaded the package, then we defined the revision. Why should we update the revision in the metadata?

This comment has been minimized.

Copy link
@lasote

lasote Nov 22, 2018

Author Contributor

not anymore. removed.

return url

def _remove_conanfile_files(self, conan_reference, files):
# V2 === revisions, do not remove files, it will create a new revision if the files changed

This comment has been minimized.

Copy link
@memsharded

memsharded Nov 21, 2018

Contributor

A bit weird, empty functionality being called. It seems that intent would be more clear if the caller didn't call it, it already knows that a new revision will be created?

Can be left as-is, though, if anything lets review later.

This comment has been minimized.

Copy link
@lasote

lasote Nov 22, 2018

Author Contributor

Yes, a bit weird, but the caller (common rest api) doesn't know (but could be done) which API versions is going to call.

lasote added some commits Nov 22, 2018

@lasote lasote merged commit 5a131ad into conan-io:develop Nov 23, 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 Nov 23, 2018

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

Package Revisions (conan-io#3055)
* apiv2 server

* wip

* Server v2 almost ready but nothing tested

* Most tests passing

* WIP adding binary package revisions

* revisions in references

* Refactor storage, search ok

* WIP

* Failing less

* Fixed tests

* Clean traceback

* Apiv2 tests passing (without revisions activated)

* jenkins multiapi

* Fixed jenkins

* Unique folders

* Git commits as revisions

* PEnding server better route

* Fixed routes

* changes revisions, many pending

* Pending work, pending issues

* Fixed some test

* AsserRaiseRegex

* AsserRaiseRegex

* Fixed actions

* First revision tests

* Upload also updates local registry

* Review

* Revision

* Info fixed

* Server search for server

* Pending managing references better

* Some fixes

* Fixed sources tgz

* Fixed test

* Check earlier the txz

* fixed some tests

* Mimetype

* Remote order

* revisions test

* Debug mac issues

* Prints debug

* More debug

* More debug

* More debug

* All tests

* and more debug

* and more debug

* Fixed env

* Remove prints

* All configs

* pull image

* Fixing tests

* Some more tests

* Fixed rev test

* Removed server storage adapter custom

* More test, controlled package uploading

* More test, controlled package uploading

* Fixed tests

* Fix skip remote

* new revisions

* Tagged test not revisions compatible, jenkinsfile progress

* Fix workspace var

* Fix workspace var

* Fix workspace var

* Fix workspace var

* Jenkinsfile

* Jenkinsfile

* Jenkinsfile

* Jenkinsfile

* Jenkinsfile

* Jenkinsfile

* Jenkinsfile

* Jenkinsfile

* Jenkinsfile

* Switch complete

* Switch complete

* Switch complete

* Excluded some test fixed others

* Fixes

* fix double locking

* Clear revision in registry when export

* Missing test

* Try to fix test

* Try to fix test

* Try to fix test

* Sleep 1 after upload

* Sleep 1 sec

* New revisions test and fixed rest api tests

* New tests

* Revisions search + delete in progress

* New tests search + remove

* Not delete package, let update if update or keep the bad package

* Fixed test, better storage of references and simplified code

* Added server

* init

* server test launcher

* Ref model

* fixed server launcher

* Fixed service test

* Server tests

* Review

* Refactor server storages

* Fix errors

* Fixed search service return

* Fixed storage issues with v2 without revisions

* Fixed ref with v2 without revisions, removed py34

* Fix test recipe corrupted

* Failing hard

* Fixed v2 without revisions

* Runner equal

* typo

* typo

* Fixing test apiv2

* Fix test

* Merged with develop

* fixed jenkins conf

* Renamed method

* dead code

* Prepared to work easier with revision and speculative metadata for files introduced in apiv2

* Fix flow without revisions in apiv2

* Fixing managing dict snapshot

* Removed comment

* Jenkinsfile for revisions

* Replaced urls with a /revisions/ partial endopoint instead of #

* WIP

* Working to update with binary registry revisions

* WIP, almost everything green but wrong

* Everything broken

* Almost green, pending introduce sequential search in server

* Server v2 and searching latest pid

* Cleaned test tags and fixed test

* Fixed last test

* refresh

* Simpler Jenkins and fixed test

* Fixed jenkinsfile

* Fixed jenkinsfile

* Fixed rest api and recovered excluded tags

* Full tests and fixed non-revisions test suite

* More simplified jenkins

* 3 flavors

* Fixed jenkinsfile

* fixed runner

* Fixed config replace when activating revisions

* Fixing tests

* Improving compatibility with revision deactivated in client

* all passing

* failing test

* Self review

* Fixed comparisson

* Renamed function

* Renamed function

* Server migration

* Fixed migrations

* protect migration

* Model for package metadata

* New package metadata revisions approach

* Revisions with metadata ok

* self revision

* Fixing diff delete

* Rev0 scales as any other

* Not necessary upload URLs in v2

* Unused include

* WIP remove endpoint changes

* Remove controllers implemented

* Separated search controller

* Removed all revs in search v1

* Middle revision

* Finishing review

* Nigthly full

* Fix comparisson < > of a ref with revision and another one without revision

* Fixed graph test

* Try if macos doesnt hang anymore

* to be released in 1.10

* Fixing tests and new remote iteration for revisions

* Review

* Recovered export dir calculation

* Testing more

* WIP

* Some more tests

* Not needed a new flag for v1, we have the capabilities

* Fixed test corner case

* wip

* finished review

* 0 revs in graph

* review
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.