Relax lockfiles config_options and configure options evaluation #7993
Conversation
memsharded
changed the title
| @@ -494,7 +494,6 @@ def pre_lock_node(self, node): | |||
| node.graph_lock_node = locked_node | |||
| if locked_node.options is not None: # This was a "partial" one, not a "base" one | |||
| node.conanfile.options.values = locked_node.options | |||
| node.conanfile.options.freeze() | |||
There was a problem hiding this comment.
This is the key of this PR: the options are not freezed anymore, so they do not error while being evaluated and getting different values.
| @@ -343,6 +343,16 @@ def evaluate_graph(self, deps_graph, build_mode, update, remotes, nodes_subset=N | |||
| for node in deps_graph.ordered_iterate(nodes_subset=nodes_subset): | |||
| self._propagate_options(node) | |||
|
|
|||
| # Make sure that locked options match | |||
| if (node.graph_lock_node is not None and | |||
There was a problem hiding this comment.
This has replaced the previous options.freeze() instead of detecting when someone is trying to modify a frozen option value, check at the end of the process that the final options match the locked ones.
| self.assertIn(expected, client.out) | ||
|
|
||
| # Order of LibC, LibB does matter, in this case it will not raise | ||
| client.save({"conanfile.py": GenConanfile().with_requires("LibC/1.0", "LibB/1.0")}) |
There was a problem hiding this comment.
This test has changed, it cannot raise an error anymore. It will be a new (missing) binary for the added LibC, that will get the locked option for LibA:myoption. Depending on the evaluation error it can be different, and this is annoying but at least it is correct in the sense that it respects the lockfile, but in some cases it will error early, and in other cases the locked value will prevail and if anything it can fail later (missing binary, or binary cannot be built).
This is not ideal, but I think it is complicated and it is not worth such a huge investment, and this can wait until the new graph in Conan 2.0
There was a problem hiding this comment.
I like the idea of validating the options after the graph has been built. I've added some suggestions, have a look at them, the most important one is the new exception message, I think that the order of parameters is wrong.
(Digression for Conan 2.0)
I've been playing with different scenarios and the resulting behavior makes sense to me.
BUT I would like to consider a different approach to the default_options for requirements' options. I mean, we are used to recipes like:
from conans import ConanFile
class LibC(ConanFile):
requires = 'LibA/1.0'
def configure(self):
self.options['LibA'].myoption = FalseWhat is the purpose of this statement (or the similar one default_options = {'LibA:myoption': False})? To force the value of the option of a requirement. What for? Many times it is because LibC doesn't compile unless LibA uses that value for that option.
What would be the consequences of deprecating the set value of requirement in favor of validate the final value (upcoming PR)?
from conans import ConanFile
class LibC(ConanFile):
requires = 'LibA/1.0'
def validate(self):
assert self.options['LibA'].myoption == False, "LibC requires LibA:myoption=False because ...."The user will need to set the value of libA:myoption explicitly in the profile/cli or use a lockfile with the required value (like the test that is being modified here), I'm sure that the implementation and the option resolution would be much simpler,...
Co-authored-by: Javier G. Sogo <jgsogo@gmail.com>
Changelog: Fix: When using lockfiles, allow
config_optionsandconfigureto compute different options as long as the final evaluated values match the locked ones.Docs: Omit
Fix #7991
#tags: slow, svn
#revisions: 1