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

initial draft of conan search_binary command #6933

Closed

Conversation

solvingj
Copy link
Contributor

@solvingj solvingj commented Apr 30, 2020

Changelog: Feature: Add conan search_binary command which takes "conan install" arguments and identifies binaries which are exact or close matches. #4089 #6770 #6364
Docs: https://github.com/conan-io/docs/pull/XXXX
Note: Waiting to add docs until this feature until implementation has been reviewed.

#3316
#4089
#6770
#6364

Based upon the feature requests, this command's arguments supports all the same arguments as conan install (settings, options, profiles, etc). It has a few extra arguments related to the search results.

Here is an example of the command usage, using my default profile just as conan install would:
conan search_binary bzip2/1.0.8@ -r conan-center --closest-match --limit 3

The "effective configuration" is also resolved in the same way as conan install, including the traversal of the entire dependency graph. This resolved configuration is printed at the start of the output for reference.

Similar to conan search, this command will only look in local cache by default, unless -r/--remote is passed.

Here is a preview of the ouptut of the above command.
image

We are looking for as much feedback on this command as possible from as many users as possible. Thanks in advance!

  • Refer to the issue that supports this Pull Request.
  • If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
  • I've read the Contributing guide.
  • I've followed the PEP8 style guides for Python code.
  • I've opened another PR in the Conan docs repo to the develop branch, documenting this one.

Note: By default this PR will skip the slower tests and will use a limited set of python versions. Check here how to increase the testing level by writing some tags in the current PR body text.

@fulara
Copy link
Contributor

fulara commented May 4, 2020

Started testing this now @solvingj
First thing:
within the build tree i am trying to invoke

conan install .. -o zlib:shared=False

to trigger build missing, that works:
ERROR: Missing prebuilt package for ...
but when i invoke this for search_binary I am getting:

$ conan search_binary .. -o zlib:shared=False
ERROR: Specify the 'user' and the 'channel' or neither of them

So inconsitency there.

Second, same with conan create .
I am not sure if thats possible but actually most of the time that you see 'build missing' I think its at the building stage not the installation stage.
However if it would work with 'conan install' (above scenario ) then thats pretty much good enough.

Then onto actual use case.

  1. After I have somehow managed to conan create . and it landed in my artifactory I tried to:
    conan install mypkg/0.6.5@ -o zlib:shared=False
    It didnt show any mistmatches, I think thats because It does not work on full_options.

  2. I dont have a good example to test right this minute, but a second use case is when you override a dependencies.
    It would be nice if it would be capable of telling which exact dependencies are affected - but it may be a bit tricky.

Good luck.

@memsharded
Copy link
Member

This is a draft that we will consider to improve for new Conan 2.0.

There is no inconsistency, but the error message should be raising that search_binary requires a package reference as a input, not a path.

@solvingj
Copy link
Contributor Author

solvingj commented May 5, 2020

👍 It is certainly inconsistent with conan create and conan install support at this time.
My first review comment on this PR is on command.py and says

Need to fix support for passing path_to_conanfile. Currently only works with a reference.

  1. Doesn't seem actionable, not really clear of the details on that example case
  2. It will show a difference in requires when that difference affects package_id. By default, conan uses semver_direct_mode (major version difference on direct dependencies). So, if you bump a major version of a dependency, the results output does show the diff of requires right now, so it should be visible when there's an override.

@fulara @sztomi @dzolee @Marcin-Radecki the major question here is: is the output reasonably close to what you imagined, and suitable for the process of debugging a missing package_id which has been a common complaint in the past?

@fulara
Copy link
Contributor

fulara commented May 5, 2020

@memsharded the original description of the work states clearly:

this command's arguments supports all the same arguments as conan install (settings, options, profiles, etc). It has a few extra arguments related to the search results.

And as I've shown above it is not the case. based on reading that statement I expected it to be a drop in command for the same place where I invoked the conan install it is clearly not the case.

@solvingj unfortunatelly with the output of the command at the moment I am not able to say whether the output of the command would fulfill my needs.
in the 1)
I invoked:
conan install my_package/1.0.0@ -o zlib... I had errors about missing prebuilts.
then I invoked I
conan search binary my_package/1.0.0@ -o zlib... and it returned no errors, it didnt show any mismatches.
That was because it seem not to currently match on full_options. but only on the option of the latest binary.
If you think thats not the case and it should have shown mismatches I can provide you MVE.

re 2) Going by conan approved way there is no way to change requires when invoking the conan install that takes a reference. It will always match against something thats already prebuild.

At the moment search_binary does not provide any ability to improve my workflow.
The output looks very promising - and this is indeed very close to what I have imagined, but currently I cannot test it for the the cases it matters to me.
It needs to be capable for working on paths.

@sztomi
Copy link
Contributor

sztomi commented May 6, 2020

From my point of view, this would be useful even if it wasn't 100% compatible with the install invocation (that certainly helps though). Very grateful that you are tackling this @solvingj !

@sourcedelica
Copy link

I like the UI with the colors and the concept of distance. If there's a way to make it more compact as a table that would be great, though you might need to go to HTML for that.

Ideally you would use pass both reference and path_to_conanfile to pull to get the options that are set in the Conanfile, so you don't have to figure them out and pass then to this command. That would be a hybrid of what @fulara is proposing but limited to one reference to diagnose at a time. If you did a drop-in replacement for conan install you would definitely need to have a HTML-based UI because there could be many problem references. Actually if you went this route it could be an option for conan install. --missing_prebuilt_info FILE or something nicer.

@solvingj
Copy link
Contributor Author

solvingj commented Nov 7, 2020

@sourcedelica thanks for the feedback.

There are many ways to approach improving the experience of dealing with this error. Please let us know if you think that this approach of a command which takes arguments mirroring the command that fails and provides a heuristic list of results in the terminal sorted by distance is intuitive and improves the experience of diagnosing the error in a straightforward way for cases you've had. @fulara pointed out some potential bugs in the implementation under certain conditions, so obviously assume that all those bugs were worked out and the arguments and results were always as described and demonstrated in the example.

@sourcedelica
Copy link

Yes - it would be very useful as it stands. Just painting the old bikeshed above :)

@derived-coder
Copy link

  1. Why is it called search_binary?
  2. Why is it not merged?

@solvingj
Copy link
Contributor Author

@derived-coder good questions.

  1. It was named search_binary simply to avoid collision with the existing search command. It was much easier to POC and demonstrate as a separate command, even though it would arguably be better as an alternate mode under the regular search command. This was an implementation detail to be discussed.

  2. There are a few good reasons for this, mostly functions of timing and cost. But, keep in mind, the last comment from the maintainers indicates they were considering it for Conan 2.0. So, maybe it will come eventually.

Timing: This draft was submitted while Conan 2.0 was just starting to be developed, and all hands needed to be devoted to it. It just didn't make sense to add such a significant new feature under the 1.0 CLI at that time. Also, there was a lot of optimism about the new feature of "Custom Commands" , but it was unclear how it would pan out and what it's use-cases might be. Since this feature is in some ways a "wrapper" with extra processing around the existing search command, perhaps it could be left out of Conan core and implemented as a custom command. That still might be the decision.

Cost: This feature is actually very "expensive". This is because it requires somewhat tight coupling to details about how profiles, settings, and options are implemented, and those are things that have been in significant flux due to the changes in the cross compilation model over the past 4 years. If it had been implemented, it would have broken every time they made a major change and need to be seriously refactored, and that would have annoyed a lot of people, and complicated new feature discussions. Again, it was just a bad time to add this feature.

As much as I was excited for this feature, it was not the right time.

@memsharded
Copy link
Member

This PR became obsolete, all the internal classes and architecture it is following has changed a lot in the already released 2.0-alpha.

Good news is that Conan 2.0 features a PythonAPI and a user-commands framework. Implementing and maintaining any custom binary search with that will be trivial, and we will reconsidering something like this after 2.0 (2.X), but at the moment it is better to close this PR, as the new solution will be architecturally very different.

@memsharded memsharded closed this Feb 15, 2022
@solvingj solvingj deleted the add_search_binary_command branch February 16, 2022 04:43
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.

6 participants