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

Refactor/graph lock #6237

Merged
merged 20 commits into from Jan 17, 2020
Merged

Refactor/graph lock #6237

merged 20 commits into from Jan 17, 2020

Conversation

memsharded
Copy link
Member

@memsharded memsharded commented Dec 16, 2019

Changelog: Feature: Some improvements in the internal of lockfiles. Better ordering of nodes indexes. Separation of requires and build-requires. Better status field, with explicit exported, built values.
Docs: Omit

In preparation for https://github.com/conan-io/conan/pull/6217/files

Close #5673

#tags: slow
#revisions: 1

@memsharded memsharded requested a review from jgsogo Dec 16, 2019
@memsharded memsharded added this to the 1.22 milestone Dec 16, 2019
@memsharded memsharded marked this pull request as ready for review Dec 16, 2019
Copy link
Member

@jgsogo jgsogo left a comment

I'm starting to get familiarized with these changes, just writing down a few comments before I actually try the PR.

This looks like more than a simple refactor and, at least, it is going to break existing behavior related to the build_info:

  • I like having a status field, but removing the modified one will break existing conan_build_info --v2 (since Conan v1.20).
  • Splitting requires/build_requires should probably break the build_info too

@czoido,

We should leverage on the field lockfile["version"]/LOCKFILE_VERSION to know how to parse the JSON file, I would vote for (remember that there is always an outdated machine in the CI):

  • 1.20.x: assert lockfile["version"] == 0.20, better fail than parsing something wrong
  • 1.21.x: assert lockfile["version"] == 0.20, better fail than parsing something wrong
  • 1.22: we need more testing in the conan_build_info --v2, some tests should fail given the changes in this PR. Depending on the lockfile version it should use a different parser to read the JSON file.

conans/client/graph/graph_binaries.py Outdated Show resolved Hide resolved
conans/model/graph_lock.py Show resolved Hide resolved
conans/model/graph_lock.py Outdated Show resolved Hide resolved
conans/model/graph_lock.py Outdated Show resolved Hide resolved
conans/model/graph_lock.py Outdated Show resolved Hide resolved
@memsharded
Copy link
Member Author

@memsharded memsharded commented Dec 17, 2019

This looks like more than a simple refactor and, at least, it is going to break existing behavior related to the build_info:

I like having a status field, but removing the modified one will break existing conan_build_info --v2 (since Conan v1.20).

The conan_build_info is updated as well to process these changes. It is installed and versioning with Conan, so if you installed Conan to generate version 0.3 of the lockfile, the conan_build_info is already capable of handling it.

I am fine with adding version checks in the conan_build_info, and to fail with a nice message if the version is old, but not to add and maintain conditional parsers based on the version.

@jgsogo
Copy link
Member

@jgsogo jgsogo commented Dec 17, 2019

The graphlock might be stored, might be stashed and unstashed in different nodes... those nodes will have different Conan versions, CI will fail (or not, but it will create a wrong BuildInfo).

@memsharded
Copy link
Member Author

@memsharded memsharded commented Dec 17, 2019

The graphlock might be stored, might be stashed and unstashed in different nodes... those nodes will have different Conan versions, CI will fail (or not, but it will create a wrong BuildInfo).

Yes, sure, lets add a version check for a clean error message, but not adding multiple parsers, they should update the nodes to a consistent version.

@jgsogo
Copy link
Member

@jgsogo jgsogo commented Dec 17, 2019

Yes, sure, lets add a version check for a clean error message, but not adding multiple parsers, they should update the nodes to a consistent version.

The check is the bare minimum, but one important use-case for CI+graphlock is to store the graphlock of a project and use it to test if an incoming change to a library breaks the project or not. The graphlock used for the project is the one stored from a previous run (the last one that worked), it was generated with an older Conan, with a different graphlock version... if we are not able to parse it with the current Conan then the users won't update Conan in the CI or they will need to migrate the graphlock by hand.

I know it is burden, and right now there are few people using the graphlock in their CIs, but, if we support this feature, these early adopters will update Conan more often in their CI and will keep testing and giving feedback about these features.

@memsharded
Copy link
Member Author

@memsharded memsharded commented Dec 17, 2019

It is not only the minimum, it is the only sane possibility. You cannot recover necessary information from older versions lockfiles, for example what is a build require and what not. And we are not going to implement version comparisons not only in the parsing but in different places in the codebase. We need to keep things simple and maintainable. This is exactly the reason this feature is experimental, and that we have decided not to store the lockfiles in packages yet.

@memsharded memsharded assigned memsharded and unassigned lasote Dec 17, 2019
@memsharded memsharded removed their assignment Dec 18, 2019
Copy link
Member

@jgsogo jgsogo left a comment

What is the status of this PR? Is there anything else you want to modify/add/remove?

conans/model/graph_lock.py Outdated Show resolved Hide resolved
@jgsogo jgsogo assigned memsharded and unassigned lasote Jan 13, 2020
Co-Authored-By: Javier G. Sogo <jgsogo@gmail.com>
@memsharded
Copy link
Member Author

@memsharded memsharded commented Jan 14, 2020

What is the status of this PR? Is there anything else you want to modify/add/remove?

@jgsogo No, I think it is ready, could be merged. I have reviewed my own code again, everything seems reasonable, the split of build_requires, the ordering of nodes, the "modified" status.

jgsogo
jgsogo approved these changes Jan 17, 2020
Copy link
Member

@jgsogo jgsogo left a comment

Yes, let's go! We need to keep improving the lockfiles

@memsharded memsharded merged commit 5458b92 into conan-io:develop Jan 17, 2020
2 checks passed
@memsharded memsharded deleted the refactor/graph_lock branch Jan 17, 2020
@sssgtg
Copy link

@sssgtg sssgtg commented Jun 2, 2020

@memsharded When I read the conversation above I assumed LOCKFILE_VERSION 0.2 and 0.3 would not be compatible. However, the check in graph_lock.py of version is still (Conan 1.25.2):

if version < "0.2":
raise ConanException("This lockfile was created with a previous incompatible "
"version. Please regenerate the lockfile")

Should I interpret that as lockfiles of version 0.2 and 0.3 being compatible or is it a miss in the version check?

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.

4 participants