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

Use reference with recipe revision for build order #5863

Merged
merged 5 commits into from Oct 9, 2019

Conversation

@danimtb
Copy link
Member

danimtb commented Oct 4, 2019

Changelog: Bugfix: Fix conan graph build-order output so it uses references including its recipe revision
Docs: omit

#REVISIONS: 1

  • Refer to the issue that supports this Pull Request: Coming from the investigation done in conan-io/examples#35
  • 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.

If this fix is right, then conan-io/examples#35 should be closed

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.

Copy link
Member

memsharded left a comment

Yes, I was wrong about copy_clear_rev() behavior, this is right.

This means that the conan graph build-order is broken for revisions, might be worth of a 1.19.2. @lasote please eval.

@lasote

This comment has been minimized.

Copy link
Contributor

lasote commented Oct 7, 2019

Wow, I was also wrong about the behavior of the copy_clear_rev() of the package reference. Actually I'm wondering what is the difference between these two functions:

    def copy_clear_rev(self):
        ref = self.ref.copy_clear_rev()
        return PackageReference(ref, self.id, revision=None)

    def copy_clear_revs(self):
        return self.copy_with_revs(None, None)

Are both doing the same, right??? I suggest renaming copy_clear_rev to copy_clear_prev and changing the behavior to clear only the prev. Then run the tests and see case by case if we meant
copy_clear_revs or copy_clear_prev.

Also check for pref.copy_with_revs(pref.ref.revision, None) and replace with copy_clear_prev

I agree this should go to a minor.

@memsharded memsharded changed the base branch from develop to release/1.19.2 Oct 7, 2019
@memsharded memsharded changed the base branch from release/1.19.2 to develop Oct 7, 2019
Copy link
Member

memsharded left a comment

Please, target it to the new release/1.19.2 branch

@memsharded memsharded added this to the 1.19.2 milestone Oct 7, 2019
@danimtb danimtb force-pushed the danimtb:fix/build_order_ref branch from d8e6b43 to c89061a Oct 8, 2019
@danimtb danimtb changed the base branch from develop to release/1.19.2 Oct 8, 2019
@danimtb

This comment has been minimized.

Copy link
Member Author

danimtb commented Oct 8, 2019

Changed to target the release/1.19.2 branch

@@ -290,8 +290,7 @@ def copy_with_revs(self, revision, p_revision):
return PackageReference(self.ref.copy_with_rev(revision), self.id, p_revision)

def copy_clear_rev(self):

This comment has been minimized.

Copy link
@lasote

lasote Oct 9, 2019

Contributor

Won't be better to call it copy_clear_prev? It will it would be less confusing and will help to identify the actual calls to copy_clear_rev and analyze if it should be copy_clear_revs or copy_clear_prev.

This comment has been minimized.

Copy link
@danimtb

danimtb Oct 9, 2019

Author Member

ok, no problem. I was trying to be consistent with the naming ⚖️

This comment has been minimized.

Copy link
@danimtb

danimtb Oct 9, 2019

Author Member

Done!

@danimtb danimtb self-assigned this Oct 9, 2019
danimtb added 3 commits Oct 9, 2019
fix
@memsharded memsharded merged commit 58df655 into conan-io:release/1.19.2 Oct 9, 2019
2 checks passed
2 checks passed
continuous-integration/jenkins/pr-head This commit looks good
Details
license/cla Contributor License Agreement is signed.
Details
@lasote

This comment has been minimized.

Copy link
Contributor

lasote commented Oct 10, 2019

A couple of interesting lines and I would say useless pref = pref1.copy_clear_prev().copy_with_revs(pref1.ref.revision, None) has been left.

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