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

Adding an argument to pass a lockfile #294

Closed
wants to merge 3 commits into from
Closed

Adding an argument to pass a lockfile #294

wants to merge 3 commits into from

Conversation

prince-chrismc
Copy link
Contributor

resolves #271

This would be appreciated to help enable more workflows which depend on the reliability of lock files.

I have not looked at the test code but I given the work I've seen from the Conan team, it's well above my python skills 🙊

I am namely opening this to show there is interest for this feature in hopes it might make the 0.16 release that's around the corner!

resolves #271 

This would be appreciated to help enable more workflows which depend on the reliability of lock files.

I have not looked at the test code but I given the work I've seen from the Conan team, it's well above my python skills 🙊
@CLAassistant
Copy link

CLAassistant commented Oct 30, 2020

CLA assistant check
All committers have signed the CLA.

@prince-chrismc
Copy link
Contributor Author

To link issues so I can find them again...

I was looking at implementing https://github.com/conan-io/conan/issues/7802, but I encountered this impediment

@czoido
Copy link
Contributor

czoido commented Oct 30, 2020

Hi @prince-chrismc,
Thanks a lot for the contribution. In the current state the lockfile profile is going to conflict with other settings/options passed to conan through cmake-conan.I think that if we want to add the lockfiles functionality to cmake-conan we have to invalidate all of the detected settings and options and also the ones that are passed as arguments can't be taken into account.
We should also add a warning message so that when you run cmake-conan with a lockfile you know that only the profile in the lockfile is going to be applied, so it's the user's responsability to have a consistent lockfile.

conan.cmake Outdated Show resolved Hide resolved
Co-authored-by: Carlos Zoido <mrgalleta@gmail.com>
@prince-chrismc
Copy link
Contributor Author

I think that if we want to add the lockfiles functionality to cmake-conan we have to invalidate all of the detected settings and options and also the ones that are passed as arguments can't be taken into account.

I certainly agree that this is a tricky situation, I do not see any other features that collide currently so I think it's important to address that.

As the PR stands it's the Conan client itself that will report an error if other settings are passed. Could a solution be to fail at the cmake level (when conflict args are passed)? I think "invalidate" and "override" are very confusing for a consumer, and may be a nasty surprise.

conan.cmake Outdated Show resolved Hide resolved
Copy link

@a4z a4z left a comment

Choose a reason for hiding this comment

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

exactly the feature I was looking for, and it turns out it's under review. would be nice to have this soon merged

conan.cmake Outdated Show resolved Hide resolved
@a4z
Copy link

a4z commented Nov 4, 2020

An other general note, or question: I wonder if there could not, and should not, be a dependency to the lockfile, if one is used, so in case the lockfile gets updated, conan install will be called again ... ?

@a4z
Copy link

a4z commented Nov 11, 2020

additional note,
since it turns out that conan should be able to take a lock file instead of a conanfile.txt/py, it might be that no changes need to be done at all, but therefore the conan version need to be checked.

or what are the general thought to this topic

@a4z
Copy link

a4z commented Feb 2, 2021

what is the status of this PR, when will it be merged?

@czoido
Copy link
Contributor

czoido commented Feb 2, 2021

Hi @a4z,
Sorry for the delay in this.
At the current state of the PR it can't be merged, it will pass settings to Conan at the same time that will set a lockfile and will always fail. I'll check this week with the team if we can come up with a way of making lockfiles usable from cmake-conan.

@a4z
Copy link

a4z commented Feb 2, 2021

as far as I see, @prince-chrismc addressed that in 5cc18ea

@czoido czoido removed the PR: Wait FB label Feb 3, 2021
@czoido
Copy link
Contributor

czoido commented Feb 3, 2021

Hi @a4z,
Those changes are not enough to solve the collision of arguments, all the settings autodetection should be bypassed and also if any environment variable is passed. I'll do some tests this week to see if we can move it forward.

@prince-chrismc
Copy link
Contributor Author

I am only familiar with the basic usage, but if there's any issue I'll do my best to address them

@czoido
Copy link
Contributor

czoido commented Feb 3, 2021

Hi @prince-chrismc,
Thanks a lot for the contribution. We have decided that it's better not to merge this for the moment event if the PR works because it won't work without bypassing all the settings auto-detection. We also have the feeling that more of the new Conan features are colliding with the initial purpose of cmake-conan that was not more than having a CMake wrapper for simple use cases.
So, instead of supporting more features in the current state of the application, after discussing the future of cmake-conan with the team, we have decided to make a refactor so that it's flexible enough to support all of the upcoming new Conan features and those that are not yet implemented in cmake-conan without causing all these problems to the users.
We'll keep updating about this. Thanks a lot!

cc @a4z

@prince-chrismc
Copy link
Contributor Author

I agree, this was very difficult to add to the current workflow, perhaps it could be a seperate conan_run() function for the challenge you outlined above? Not sure a temporary alternative is the best but it would be helpful to bridge the gap.

In the meantime, I have an implementation https://github.com/prince-chrismc/user-management/blob/d58a17baa6e64c706154984f8791c7f1269192b1/backend/cmake/conan-setup.cmake#L16-L30 in case any one else is missing this feature

@a4z
Copy link

a4z commented Feb 3, 2021

OK thanks for the info!

since I do not need all the flexibility in my project because I know exactly what to call, it might be more easy to wite a execute_process call that does what I would else do on the command line anyway.

I understand that providing all the possible combinations that conan gives is maybe a too hard and work intensive task, especially when looking to upcoming iterations of conan and the future conan 2

@czoido
Copy link
Contributor

czoido commented Feb 3, 2021

Yes, I think we are going in that direction, split current conan_cmake_run functionalities so that you can choose which part you can run without colliding one to others. The, in the end you will be able to have a wrapper for the conan install command with all the arguments you want to call.
Thanks a lot for all the feedback and sorry for delaying this so much.

@czoido
Copy link
Contributor

czoido commented Feb 23, 2021

Hi @prince-chrismc, @a4z
I'm closing this as we have added new functions (conan_cmake_configure/autodetect/install) in v0.16 that can replace the flow with conan_cmake_run and enable things like using lockfiles or build and host profiles without colliding with the auto-detection. From now on it will be the recommended flow as it is more explicit and flexible.

@czoido czoido closed this Feb 23, 2021
@a4z
Copy link

a4z commented Feb 23, 2021

Thanks for the ping Carlos! Looking forward exploring the new features of 016!!

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.

Support lockfiles in conan_cmake_run()
4 participants