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/install ref #4197

Merged
merged 11 commits into from Jan 24, 2019

Conversation

Projects
None yet
4 participants
@memsharded
Copy link
Contributor

commented Dec 26, 2018

Changelog: Feature: Enable a new reference argument in conan install <path> <reference>, where reference can be a partial reference too (identical to what is passed to conan create or conan export. This allows defining all pkg,version,user,channel fields of the recipe for the local flow.

Docs: conan-io/docs#1045

memsharded added some commits Dec 26, 2018

@ghost ghost assigned memsharded Dec 26, 2018

@ghost ghost added the stage: review label Dec 26, 2018

@memsharded memsharded requested a review from jgsogo Dec 26, 2018

@memsharded memsharded removed their assignment Dec 26, 2018

@ghost ghost assigned memsharded Dec 26, 2018

@jgsogo
Copy link
Member

left a comment

I'm trying things like the one below, to test if it is the same behavior with .py and .txt files (and it is not):

class InstallReferenceTest(unittest.TestCase):

    @parameterized.expand([(True, ), (False, )])
    def test_install_reference_full(self, use_py):
        client = TestClient()
        if use_py:
            conanfile_py = 'from conans import ConanFile\nclass Pkg(ConanFile):\n\tpass'
            client.save({"conanfile.py": conanfile_py})
        else:
            conanfile_txt = '[generators]\ncmake'
            client.save({"conanfile.txt": conanfile_txt})
        client.run("install . Pkg/0.1@myuser/testing")
        client.run("info . -n None")
        self.assertIn("Pkg/0.1@myuser/testing", client.out)
@memsharded

This comment has been minimized.

Copy link
Contributor Author

commented Dec 26, 2018

Yeah, I thought about this one. It is not equal, because .txt cannot create packages, doesn't have any method that could use the attributes, or contain any logic, etc.

It seemed that it didn't make a lot of sense to add this behavior to conanfile.txt if it is never used. Should I add it? Should it be blocked (raising an Exception)?

@jgsogo

This comment has been minimized.

Copy link
Member

commented Dec 26, 2018

I would like to have the same behavior for a conanfile.txt and a conanfile.py for all the valid actions, I think it would simplify docs (probably sources too), and batch scripts ➡️ Someone could be using a batch script like: for every project in my company, install it with the project name (conan install . <project>/version@user/channel), compile and do something with it...

From my point of view, block/raise is not an option (we will force the batch script to check if it is a conanfile.txt in order to change the call). We can warn the user about this extra information he/she is providing and won't be used, but I think it will be easier to use the information, no need to explain anything, no need for extra checks in the implementation...

Is it hard to add this behavior? Does it require too many changes?

@memsharded

This comment has been minimized.

Copy link
Contributor Author

commented Dec 26, 2018

Make sense, I'll try to change that.

@memsharded

This comment has been minimized.

Copy link
Contributor Author

commented Dec 26, 2018

I have added code for printing the package information in conan info. The command conan install . <pkg-ref> worked already, it was just not printing the reference in the conan info command.

Now it prints:

conanfile.txt (Pkg/version@user/channel)
  ...

Or just conanfile.txt if there is no reference. The first read a bit weird IMO, if the conan install . <pkg-ref> command works consistently, I am not sure if I'd keep just this output.

@danimtb
Copy link
Member

left a comment

I am not sure what pain this PR is trying to solve. I guess something related to consumer nodes of the graph not having a proper name but for me this is adding complexity to users to solve a "Conan issue".

For me the syntax conan install . Pkg/0.1@myuser/testing means: Install the requirements of my conanfile.txt (located in .) and additionally this Pkg/0.1@myuser/testing one. I think this will confuse users.

Also this is missing documentation

client.save({"conanfile.txt": ""})
client.run("info .")
self.assertIn("conanfile.txt", str(client.out).splitlines())
client.run("install . Pkg/0.1@myuser/testing")

This comment has been minimized.

Copy link
@danimtb

danimtb Dec 27, 2018

Member

This is confusing for conanfile.txt users

This comment has been minimized.

Copy link
@memsharded

memsharded Dec 27, 2018

Author Contributor

It is not necessary, you don't need to specify it. Furthermore, raising an error was one of the possibilities suggested above. @jgsogo wanted to keep it to be consistent for all kinds of conanfiles

@memsharded

This comment has been minimized.

Copy link
Contributor Author

commented Dec 27, 2018

There are several real problems that this PR is trying to solve:

  • Local commands (conan build, conan package) are inconsistent with local cache behavior if they try to use package name, version, etc, which is not very common but not that uncommon:
# This will fail with command ``conan build``
class Pkg(ConanFile):
    #no version explictly defined, it is passed dynamically in conan create/export
    def build(self):
        if self.version == "1.68":
            self.run("some build...")
        else:
            self.run("other build...")
  • Needing environment variables CONAN_USER, CONAN_CHANNEL for defining them when consumer:
class Pkg(ConanFile):
     def requirements(self):
           self.requires("Pkg/0.1@%s/%s" % (self.user, self.channel))

The above fails if the environment variables are not defined and you use it for local development with conan install. And this is a very common pattern. Seems that conan install . myuser/channel is a more reasonable and homogeneous way to pass user and channel to the system than via env-vars.

These environment variables should probably be deprecated in conan 2.0

  • Finally, not having this functionality will make very, very difficult to consume graph locks files in some cases. When doing a conan install, the current node needs to be identified. In some cases, if the conanfile.py specifies name and version, they might be used. But there are conanfiles that do not define name and version in the recipe. In the extreme cases where there are multiple nodes with the same reference, it might be needed an ID. Those cases would probably be fired from CI anyway, and the ID could be automatically managed. But in most cases, having a human-understandable way to specify the complete reference of the package currently being installed, seems the way to go.
@jgsogo

This comment has been minimized.

Copy link
Member

commented Dec 31, 2018

We should take into account this comment by @danimtb, it could be the expected behavior (pip install pkg1 pkg2), and it is incompatible with the proposed implementation in this PR:

For me the syntax conan install . Pkg/0.1@myuser/testing means: Install the requirements of my conanfile.txt (located in .) and additionally this Pkg/0.1@myuser/testing one. I think this will confuse users.

...and the only alternative I can think about is using and argument 😕 or adding interactivity 😟

I'm sorry, but I don't have a silver bullet.

@memsharded

This comment has been minimized.

Copy link
Contributor Author

commented Dec 31, 2018

I thought that it was very aligned with other conan commands:

$ conan export <path> <partial-ref>
$ conan create <path> <partial-ref>
$ conan export-pkg <path> <partial-ref>
$ conan test <path> <full-ref>

None of them mean to export, create, or test 2 different things, so apparently the commands:

$ conan install <path> <partial-ref>
$ conan info <path> <partial-ref>

had a nice symmetry with the above

Note that it doesn't necessarily exclude installing multiple references, as the first argument could be tested to not be a reference, as it is already done right now where there is just 1 argument.

I understand the point, but as you commented, the alternative of using an argument is a bit ugly too.

@lasote
Copy link
Contributor

left a comment

I agree with @memsharded this is the best approach we could do and totally understand the need. I suggest a couple of UX changes.

@@ -346,6 +346,9 @@ def install(self, *args):
parser.add_argument("path_or_reference", help="Path to a folder containing a recipe"
" (conanfile.py or conanfile.txt) or to a recipe file. e.g., "
"./my_project/conanfile.txt. It could also be a reference")
parser.add_argument("reference", nargs="?",
help='user/channel, version@user/channel or pkg/version@user/channel '

This comment has been minimized.

Copy link
@lasote

lasote Jan 2, 2019

Contributor

Suggested help:

Reference for the conanfile path of the first argument: user/channel, version@user/channel or pkg/version@user/channel 
@@ -370,7 +373,9 @@ def install(self, *args):
try:
reference = ConanFileReference.loads(args.path_or_reference)

This comment has been minimized.

Copy link
@lasote

lasote Jan 2, 2019

Contributor

Check that args.reference is None or raise. (Two references makes no sense)

@jgsogo

jgsogo approved these changes Jan 2, 2019

@jgsogo

This comment has been minimized.

Copy link
Member

commented Jan 2, 2019

I'm approving it, but still needed what other reviews are requesting:

  • docs
  • check that two references makes no sense
@lasote

This comment has been minimized.

Copy link
Contributor

commented Jan 8, 2019

Everything good BUT DOCS

@danimtb
Copy link
Member

left a comment

Still think the command conan install . <ref> is confusing as this command interacts both in the cache and in user space depending on using a ref as first argument or using a path. So let's document this clearly

@lasote lasote added this to the 1.12 milestone Jan 23, 2019

@memsharded memsharded referenced this pull request Jan 24, 2019

Merged

install-ref docs #1045

@memsharded memsharded merged commit c446790 into conan-io:develop Jan 24, 2019

2 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
license/cla Contributor License Agreement is signed.
Details

@ghost ghost removed the stage: review label Jan 24, 2019

@memsharded memsharded deleted the memsharded:feature/install_ref branch Jan 24, 2019

@jgsogo jgsogo referenced this pull request Feb 1, 2019

Closed

Error when using 'conan info .' (Conan 1.12.0) #4443

3 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.