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

Make sure that options are fully locked when creating lockfiles based on an existing lockfile #7513

Merged
merged 13 commits into from Sep 30, 2020

Conversation

memsharded
Copy link
Member

@memsharded memsharded commented Aug 6, 2020

Changelog: Fix: Raising conflict errors when options doesn't match in the evaluation of graphs with lockfiles.
Docs: Omit

@memsharded memsharded requested a review from jgsogo August 6, 2020 09:04
Copy link
Contributor

@jgsogo jgsogo left a comment

Choose a reason for hiding this comment

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

This is testing a diamond, does it work the same if the option is overridden in a transitive consumer?


I think that the resulting graph should contain exactly the recipes-packages defined in the input lockfile. We cannot generate a different graph and keep going. 👍

Now the question, what's better for the user?

  • If we don't take into account the value that is overriding the one in the lockfile, it "can fail later (build time, link time)"... or not. Even if we print a warning no one will read it (it would be some of those warnings to be configured as errors). Without the warning, IMHO we cannot merge this behavior.
  • If we raise because of the new computed package-ID, it is evident to the user and they need to solve the conflict and create a new lockfile to use it.

To be honest, I prefer the second one. I think it is easier for the user: see the failure, generate new lockfile, run the command again. We can implement the other behavior in the future if people complain (maybe a CI use-case find that behavior quite inconvenient).

conans/test/functional/graph_lock/dynamic_test.py Outdated Show resolved Hide resolved
conans/test/functional/graph_lock/dynamic_test.py Outdated Show resolved Hide resolved
conans/test/functional/graph_lock/dynamic_test.py Outdated Show resolved Hide resolved
conans/test/functional/graph_lock/dynamic_test.py Outdated Show resolved Hide resolved
conans/test/functional/graph_lock/dynamic_test.py Outdated Show resolved Hide resolved
conans/test/functional/graph_lock/dynamic_test.py Outdated Show resolved Hide resolved
conans/test/functional/graph_lock/lock_recipe_test.py Outdated Show resolved Hide resolved
@memsharded memsharded added this to the 1.30 milestone Sep 7, 2020
@memsharded memsharded changed the base branch from release/1.28 to develop September 7, 2020 16:18
memsharded and others added 4 commits September 7, 2020 18:30
Co-authored-by: Javier G. Sogo <jgsogo@gmail.com>
Co-authored-by: Javier G. Sogo <jgsogo@gmail.com>
Co-authored-by: Javier G. Sogo <jgsogo@gmail.com>
@memsharded
Copy link
Member Author

Hi @GordonJess,

This is proposing some fixes for locked options, planned for 1.30, in case you wanted to try it before it is released from the source branch and make sure it is good to go.

@GordonJess
Copy link

GordonJess commented Sep 22, 2020

Hi @memsharded,

Thanks, will try get round to it later and let you know!

Copy link
Contributor

@jgsogo jgsogo left a comment

Choose a reason for hiding this comment

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

I need to know what others think about this issue because it has implications: is the lockfile a snapshot or is it (also) a profile-to-apply to commands where the lockfile is used? @solvingj @czoido

self.assertIn("LibA/1.0:5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9 - Build", client.out)
self.client = client

def partial_lock_option_command_line_test(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO, the difference between the behavior in this test and the next ones is too subtle.

I would expect the lockfile to contain a snapshot, not instructions, so having LibA:myoption=True because it was assigned in the command line while creating the lockfile or because it is the value in the default_options should lead to the same snapshot: a lockfile libb.lock where the snapshot of the node LibA contains myoption=True. Following this reasoning, all the tests should behave the same.

In this test I see that the lockfile, because it was generated with -o LibA:myoption=True, is somehow applying this argument to the next conan lock create --lockfile=libb.lock command as if it actually was explicit conan lock create --lockfile=libb.lock -o LibA:myoption=True.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure I understand the issue. The lockfile stores all the inputs, the "effective profile" applied to create the lock, including the options, in the same way that it stores the settings, build_requires and environment variables. This stored profile is applied entirely when the lockfile is used. Without it, lockfiles would be a bit incomplete as they will not be enough to reconstruct a dependency graph deterministically.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, the snapshot, not instructions statement doesn't seem valid, but I think @memsharded comments probably clear it up. Lockfiles definitely are "instructions", and so if that is unclear maybe we should look to clarify it in the documentation.

Copy link
Contributor

@czoido czoido Sep 30, 2020

Choose a reason for hiding this comment

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

For me it's also a bit strange the change of behaviour, if you use the -o LibA:myoption=True in the command line you end with a profile_host in the lockfile that includes the option [options]\nLibA:myoption=True but you also have that info in the nodes of the lockfile:

   "2": {
    "ref": "LibA/1.0",
    "options": "myoption=True",
    "package_id": "d2560ba1787c188a1d7fabeb5f8e012ac53301bb",
    "prev": "0",
    "context": "host"
   }

That makes that that option will affect nodes that are not yet in the lockfile...

Maybe the profile should not change and the -o LibA:myoption=True should only affect to the information stored in the nodes of the lockfile?

Copy link
Contributor

Choose a reason for hiding this comment

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

I know @memsharded said the string value of the profile is effectively serving a "unique key" and part of the "identity" of the lockfile. I think it might be really hard to figure out a way to change that.

Copy link
Contributor

Choose a reason for hiding this comment

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

After talking to @czoido and thinking about it... and some drawing 😄 I think that maybe the graph+profile is easier to manage than the snapshot approach when dealing with lockfiles that contains a subgraph. Here you can find some of my reasoning (probably only clear to me): https://docs.google.com/presentation/d/1G8E5sPfWN04NmJyjagjFPJeQgcPqmTiwTTDkzZySuCo/edit?usp=sharing

I've realized that my main concern is about using a lockfile smaller than the graph: using the lockfile from LibB to build the App, for example. As the lockfile is also providing the profile, it will affect packages that are outside the lockfile itself. I mean, if we created the lockfile with conan lock create .... -o *:sahred=True it will apply the option shared=True to every package of the graph, not only those that are into the lockfile. Alternatives:

  • Do not store command line arguments in the profile, value will be applied only to packages inside the lockfile.
  • Do not let the user run conan create ... --lockfile=libb.lock if the lockfile doesn't cover all the packages. The user will be forced to run conan lock create ... --lockfile --lockfile-out [config for _extra_ packages] to build the full lockfile, and then use it for conan create.

I hope my concern is clear enough. After changing my mind these are just details... but yes, I would need to read something else in the documentation to clarify it from the very beginning (maybe it is just me, it happens that when you are working on something you stop reading the docs).

Copy link
Member Author

Choose a reason for hiding this comment

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

Do not let the user run conan create ... --lockfile=libb.lock if the lockfile doesn't cover all the packages.

conan create and conan install doesn't allow at all to bring new packages to the graph. All packages involved in those commands will be covered and locked by the lockfiled, otherwise, it will fail

The mechanism to deal with this, is effectively, as you said, to create a new lockfile from the previous. It is possible to compute a new one with a new profile, if the lockfile used as a base was captured without profile (--base argument). It is also possible to remove the locked profile and create a new lockfile without it with the --base argument, from a full lockfile. So at least with a 2 step process (first remove profile information with --base, then use that base lockfile and provide new profile arguments), this seems doable.

Copy link
Contributor

@jgsogo jgsogo left a comment

Choose a reason for hiding this comment

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

Things said in my last comment are details that can be modified afterward in this experimental feature. I also need to run locally more lockfiles examples to get a better understanding of different workflows.

@memsharded memsharded merged commit 2541915 into conan-io:develop Sep 30, 2020
@memsharded memsharded deleted the fix/lockfile_options_partial2 branch September 30, 2020 17:56
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.

None yet

5 participants