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

allow conan install pkg/[*]@user/channel #5908

Merged

Conversation

@memsharded
Copy link
Member

@memsharded memsharded commented Oct 15, 2019

Changelog: Bugfix: Allow conan install pkg/[*]@user/channel resolving to a reference, not a path.
Docs: Omit

Fix #5905

@memsharded memsharded added this to the 1.19.2 milestone Oct 15, 2019
@puetzk
Copy link

@puetzk puetzk commented Oct 15, 2019

I think this PR would still leave things like pkg/[1.*]@user/channel broken? I don't have any usages like that, but they're equally valid syntax for an x-range: https://github.com/npm/node-semver#x-ranges-12x-1x-12-

@puetzk
Copy link

@puetzk puetzk commented Oct 15, 2019

Thanks!

@memsharded
Copy link
Member Author

@memsharded memsharded commented Oct 15, 2019

I think this PR would still leave things like pkg/[1.*]@user/channel broken? I don't have any usages like that, but they're equally valid syntax for an x-range:

Fixed too.

What we will definitely do is in Conan 2.0 to make the arguments non-ambiguous: a path will be explicitly a path, and a package reference will be explicitly a package reference

@puetzk
Copy link

@puetzk puetzk commented Oct 15, 2019

Yeah, the fact that conan install has such a split personality can be quite confusing (conan install <path> means "download the requirements listed in this conanfile.txt or conanfile.py and run generators) and conan install <ref> means "download or build this package and run deploy()".

I have no problem with some breaking changes to disambiguate this command, especially in a major version. I can adjust my scripts accordingly when upgrading. This change just seemed inadvertent.

@@ -148,6 +148,7 @@ class CheckValidRefTest(unittest.TestCase):
def test_string(self):
self.assertTrue(check_valid_ref("package/1.0@user/channel"))
self.assertTrue(check_valid_ref("package/1.0@user/channel"))
self.assertFalse(check_valid_ref("package/*@user/channel"))
Copy link
Contributor

@lasote lasote Oct 16, 2019

Choose a reason for hiding this comment

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

Check here the version ranges too (and with *)

Copy link
Member Author

@memsharded memsharded Oct 16, 2019

Choose a reason for hiding this comment

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

Done. Indeed the check_valid_ref is weird.

Copy link
Contributor

@lasote lasote Oct 16, 2019

Choose a reason for hiding this comment

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

And is that a reason to not test it or what? :P

Copy link
Member Author

@memsharded memsharded Oct 16, 2019

Choose a reason for hiding this comment

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

Nop, definitely tests were needed.
I am saying that I really want to get rid of all this mess, looking forward to Conan 2.0.

Copy link
Contributor

@lasote lasote Oct 16, 2019

Choose a reason for hiding this comment

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

agree

@lasote lasote merged commit 5f349ab into conan-io:release/1.19.2 Oct 16, 2019
2 checks passed
@memsharded memsharded deleted the hotfix/install_ref_version_range branch Oct 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants