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

Feature/graph lock new #5035

Closed
wants to merge 79 commits into from

Conversation

memsharded
Copy link
Member

@memsharded memsharded commented Apr 25, 2019

Changelog: (Feature | Fix | Bugfix): Describe here your pull request
Docs:

This PR provides the basic functionality for "lockfiles" in Conan, that allows to pin versions (when using version-ranges) and revisions (if using revisions).

The functionality can be summarized as:

  • Every conan command that generates a graph_info.json file (like conan install <path>), will generate also a conan.lock file, that locks the graph dependencies versions and revisions. Nothing changes in the command line.
  • Commands will use a --install-folder argument to indicate where to ouput the resulting lockfiles. Some commands like install already have this by default, and other commands like create have added this argument.
  • Conan commands define a --use-lock to indicate the lockfile is to be used to pin dependencies. It is an opt-in feature. If --use-lock is defined, the --install-folder to indicate where the lockfiles are is necessary.
  • When the --use-lock argument is provided, version-ranges will not be evaluated, but resolved to the fixed one. Also, specific, pinned recipe and package revisions will be forced
  • Normal requires, build_requires and python_requires are locked.

This is very experimental feature, expect it to fail in difficult and challenging graphs (like with multiple python-requires, or using complicated graphs of build-requires and private requires).

Testing and feedback very welcome.

@ghost ghost assigned memsharded Apr 25, 2019
@lasote
Copy link
Contributor

lasote commented Jun 11, 2019

Question: Is there a reason why the conan graph update-lock should not clean the modified (the merged) automatically when updating?

@memsharded
Copy link
Member Author

Added --json, and "lockfile folder" in help descriptions

Regarding conan graph update-lock. You don't want to invalidate "modified" flag when you are gathering info from build slaves into the orchestrator job, as having "modified" coming twice would mean something is not working well.

@jgsogo
Copy link
Contributor

jgsogo commented Jun 11, 2019

Having a look at this, just reading the commands and help messages (I suppose you have already talk about some of these things, but this is my first approach to the feature):

  • I don't like the install-folder being an input and output, and also I think it is not needed:
    • commands like install or create can consume the information from the --use-lock argument if it accepts a value (I was expecting that)
    • other commands like export, export-pkg, test or info will only consume the lock, so they only need the --use-lock argument.
  • the file conan.lock contains all the information (am I right?), I expect files like graph_info.json and conaninfo.txt to be deprecated in the future, so there is no need to talk about the "directory where the lock fileS are" but about "the lock file" (just the --use-lock=<path/file> argument).
  • If I want to generate the lock in a local folder, I would use conan graph lock <path_or_ref>, I think it is better and more clear than using create or install.

More on install and create: if I want to check the graphlock used to generate the package, I don't want to specify an install-folder in the creation command, I expect it to be stored with the package (why is it not stored in the cache?) and I would do a conan get <pref> conan.lock or call conan graph lock to generate it again.

And, from my point of view, we shouldn't add commands to manipulate the graph into conan executable. Subcommands like update-lock, clean-modified and build-order (we already have conan info --build-order ...) are not related to Conan itself, if we want to keep them into the same repository I suggest a conanci entry.

@@ -21,6 +21,7 @@ def __init__(self, proxy, range_resolver):
self._check_updates = False
self._update = False
self._remote_name = None
self.locked_versions = None
Copy link
Contributor

Choose a reason for hiding this comment

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

Having a look at the way you are using this locked_versions attribute, I feel like it should be a context_manager:

class ConanPythonRequire(object):
    ...
    @contextmanagere
    def lock_versions(self, versions_to_lock):
        old_cache = self._cached_requires
        try:
            self._cache_requires.update(  <data from versions_to_lock>)
            return
        catch:
        finally:
            self._cached_requires = old_cache

I think it will result in a cleaner implementation

self.cached_conanfiles[conanfile_path] = conanfile

self._python_requires.locked_versions = None
self.cached_conanfiles[conanfile_path] = (conanfile, lock_python_requires)
Copy link
Contributor

Choose a reason for hiding this comment

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

The conanfile object already stores the python requires list/dict, conanfile.python_requires. I know it makes a little bit more difficult the above getter, but we won't duplicate info.

return result


class GraphLock(object):
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to know the motivation for these classes.

I agree that we need to store the graph lock and have a memory representation of it, but here there are more things: we are at least duplicating business logic that was already available. If the plan is to substitute existing implementation, then I think it is worth considering a more standard representation of the graph so we can implement different algorithms on top of it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Need to keep things as much decoupled as possible. Premature engineering is evil, much better to have some duplication until behavior is much more stabilized, than refactoring and abstracting away too early.
This is not substituting anything, it is totally a new feature, not replacing or changing any existing functionality.

Copy link
Contributor

Choose a reason for hiding this comment

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

So, the plan is to substitute the existing graph? Without entering into the details (and without having a look at how it is used) I see that both graphs have neighbours, closure, inverse levels,... and I'm assuming that they should have the same information, right? So, we are going to maintain both implementations during some releases.

It's ok, I just want to know. If this is going to be merged soon, I will need to add all the changes related to cross-building to this new graph too.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, this doesn't plan to substitute the existing dependency graph at all.

Yes, some concepts are very similar, indeed. Still the implementation is very different in the details and interfaces, so that is why better keep them decoupled.

Regarding cross-building, this should be totally transparent to it. This graph does not manage conanfiles or settings at all. It doesn't even model build_requires!

It is just some data that the other graph will use. I'd say this doesn't need any modification for the cross-building model.

graph_node = GraphLockNode(node.pref if node.ref else None, python_reqs,
node.conanfile.options.values, False)
self._nodes[node.id] = graph_node
self._edges[node.id] = dependencies
Copy link
Contributor

Choose a reason for hiding this comment

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

Do not create the edge if the dependency list dependencies is empty.

@memsharded
Copy link
Member Author

other commands like export, export-pkg, test or info will only consume the lock, so they only need the --use-lock argument.

Not true, export-pkg, info, test, they can generate locks too.

the file conan.lock contains all the information (am I right?), I expect files like graph_info.json and conaninfo.txt to be deprecated in the future, so there is no need to talk about the "directory where the lock fileS are" but about "the lock file" (just the --use-lock=<path/file> argument).

It was redundant with the install-folder, but maybe it makes sense. @lasote, do you prefer that --use-lock points to a file rather than a folder? I'd say it will help for multiple profiles, because you can have all locks in a single folder.

I expect it to be stored with the package (why is it not stored in the cache?)

This is not the main use case we are trying to solve, lets focus on the main use case, which is implementing CI flows of whole products. Storing locks in packages is problematic, specially at this stage, because they will change for sure (and break if they are inside packages, outside, they can be migrated or re-generated without problems).

And, from my point of view, we shouldn't add commands to manipulate the graph into conan executable. Subcommands like update-lock, clean-modified and build-order

These commands look simple now, but for example, build-order will most likely want to expand the whole graph, not only the information from the lockfile, which will be very similar to conan info. I'd like to move some functionality from the current conan info to the conan graph command, like the output of the html graph. I definitely do not want to split this functionality into another different application, at least by now.

@jgsogo
Copy link
Contributor

jgsogo commented Jun 11, 2019

other commands like export, export-pkg, test or info will only consume the lock, so they only need the --use-lock argument.

Not true, export-pkg, info, test, they can generate locks too.

They can, but IMO they shouldn't, the same reason why they can generate the graph_info.json or the conaninfo.txt files and they are not doing it.

I expect it to be stored with the package (why is it not stored in the cache?)

This is not the main use case we are trying to solve, lets focus on the main use case, which is implementing CI flows of whole products. Storing locks in packages is problematic, specially at this stage, because they will change for sure (and break if they are inside packages, outside, they can be migrated or re-generated without problems).

From my point of view Conan should provide tools to help people "implement CI flows of whole products", but it shouldn't get mixed with a CI tool, this is also the point of my last paragraph. Also, it is easier to put new commands into a conanci ... entry point and add them to conan ... when they have been proven useful than to change/remove them once they have been added to Conan itself. It is always recommended to add new functionalities into a separate experimental API before adding them to the API itself.

@lasote
Copy link
Contributor

lasote commented Jun 12, 2019

It was redundant with the install-folder, but maybe it makes sense. @lasote, do you prefer that --use-lock points to a file rather than a folder?

I don't think that is the discussion. Initially I don't want the --use-lock point to a file, let's see if it makes sense to point to a folder and in that case we can enable point to a file later.

So, about the input/output arguments, I'll try to explain the issue and we can decide the less bad in case we are able to decide. We must have in mind two things, a temporary interface for the commands that can't be breaking and of course how we would like it at Conan 2.0, where we can assume the only generated file (in addition to the generators ones) will be the lockfile? (?¿ conanbuildinfo.txt)

conan create

The ìnstall-folder has been introduced in this branch.

Note that the result of a conan create is another lockfile containing the built node. So we need an input lockfile and an output lockfile.

Approach 1

--install-folder: New, introduced in this PR, place to write the lock and graph.info. (OUTPUT)
--use-lock: lockfile folder. (INPUT)

Approach 2

--use-lock: lockfile folder and the same file will be modified (INPUT/OUTPUT)

conan install

Note that the result of a conan install is a lockfile modified if a node has been built.

Approach 1

--install-folder: INPUT/OUTPUT? but not input for the lockfile. Output for the new lockfile
--use-lock: lockfile folder. (INPUT)

Approach 2

--install-folder: INPUT/OUTPUT? but not input for the lockfile. not output for the lockfile.
--use-lock: lockfile folder. (INPUT/OUTPUT)

conan info

We need from conan info to generate a lock with the RREVs for the CI flows.
We want a conan info accepting a lockfile.

Approach 1

--install-folder: INPUT but not for the lockfile. Will this exist on Conan 2.0?
--use-lock: lockfile folder (INPUT)

Others?

conan export

The ìnstall-folder has been introduced in this branch. We only want to use a lockfile as input.

Approach 1

--use-lock: lockfile folder (INPUT)

Question: Will the export lock something? conceptually the exported recipe could be locked but I don't think it is a real use case, right?

conan export-pkg

Approach 1

--install-folder: INPUT but not for the lockfile. Will this exist on Conan 2.0?
--use-lock: lockfile folder (INPUT)

Same Question: Will the expor-pkg lock something? conceptually the exported package could be locked but I don't think it is a real use case, right?

conan test

The ìnstall-folder has been introduced in this branch. We only want to use a lockfile (for the python requires) as input.

Approach 1

--use-lock: lockfile folder (INPUT)

Same Question: Will the test lock something? I don't think so

@jgsogo
Copy link
Contributor

jgsogo commented Jun 12, 2019

TLDR; Right now, any feedback related to workspaces would be mere speculation, I would remove the graphlock related arguments from the conan workspace command. At least until we agree on the most important use cases.

Workspaces

Current behavior related to workspaces (current implementation):

  • a conan.lock is generated in the build folder of every project declared as editable in the WS. That is the folder where all the other files like graph_info.json, generator files or conaninfo.txt are generated.
  • no conan.lock file in the folder where the workspace is installed.
  • generated graphlock is never used because the build that takes place uses tools external to Conan (the user will use the IDE, make,...).
  • If I manipulate a conan.lock and run again conan workspace install --install-folder=<path/to/dir/with/modified/conan.lock> --use-lock nothing different happens.

IMO, the graphlock feature in the workspace world has two major applications:

  1. lock version ranges
  2. being able to build packages not included in the workspace that depend on packages that can be modified inside the workspace.

Lock version ranges

It can we easily solved if we add all the editables to the root_list that is used to build the workspace. It is like overriding dependencies from the conanfile.txt, it already works.

Packages depending on editables

This is the other great use case of a graphlock outside the CI, but it is not implemented for workspaces yet: in a workspace I would like to be able to build a package locking the rest of the graph... the problem is that this graphlock has to take into account that some of the locked packages will be editable ones (they are not in the cache but in the user local folders).

I think that the graphlock shouldn't be able to handle editable packages (also remember that these editables are temporary) or to store information related to the layout... somehow, by definition, editable packages are not packages themselves and might be hard to lock.


In this first iteration, I would remove the graphlock related arguments from the conan workspace command, mature the idea and the implementation and then see if it is needed for the workspaces (right now it isn't) and how can it be adopted.

Right now, I would leave any feedback regarding workspaces for the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants