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

try to fix package_id with package_revision_mode and mixed modes #7051

Merged
merged 10 commits into from Jun 2, 2020

Conversation

memsharded
Copy link
Member

@memsharded memsharded commented May 19, 2020

Changelog: Bugfix: Fix crash while computing the package_id of a package when different package_id_mode are mixed and include package_revision_mode.
Docs: Omit

Continuation of #6947
Fix #6942

#tags: slow

cc/ @fulara

@memsharded memsharded marked this pull request as ready for review May 19, 2020
@memsharded memsharded added this to the 1.26 milestone May 19, 2020
@memsharded memsharded requested a review from jgsogo May 19, 2020
Copy link
Member

@jgsogo jgsogo left a comment

I'm starting to find all this implementation around the package-id computation really complex, and also the package ID itself when mixing modes, I think I need a place in the docs explaining at least how the propagation works.

# the node transitive information necessary to compute the package_id
# as it will be used by reevaluate_node() when package_revision_mode is used and
# PACKAGE_ID_UNKNOWN happens due to unknown revisions
node.package_id_transitive_reqs()
Copy link
Member

@jgsogo jgsogo May 20, 2020

Choose a reason for hiding this comment

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

Is there a reason to do this only when full_transitive_package_id is activated?

Copy link
Member

@jgsogo jgsogo May 20, 2020

Choose a reason for hiding this comment

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

reevaluate_node will run _compute_package_id that calls this same function. Why is it needed to update this information before doing those calls?

Copy link
Member Author

@memsharded memsharded May 20, 2020

Choose a reason for hiding this comment

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

Because reevaluate_node only runs for those nodes that has PACKAGE_ID_UNKNOWN, but the information that needs to be re-computed after being built are the one of the dependencies of the node with PACKAGE_ID_UNKNOWN.

Only full_transitive_package_id is causing the issue in this case, because if not defined, a different computation is done, based on conanfile.requires (which is updated).

I think the root cause of this is the immutability of the reference objects, that produces that there are several copies of ConanFileReference and PackageReference. I think for the future we should aim for a central definition of these for each node, in a way that an update to them automatically is seen by everyone else, because they contain a reference to this object.

Copy link
Member

@jgsogo jgsogo May 20, 2020

Choose a reason for hiding this comment

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

Because reevaluate_node only runs for those nodes that has PACKAGE_ID_UNKNOWN, but the information that needs to be re-computed after being built are the one of the dependencies of the node with PACKAGE_ID_UNKNOWN.

Here you mean, the CONSUMERS of the node with PACKAGE_ID_UNKNOWN, right?


Only full_transitive_package_id is causing the issue in this case, because if not defined, a different computation is done, based on conanfile.requires (which is updated).

Having a quick look to conanfile.requires, it is built using the direct and the indirect prefs too, so maybe we can simplify all of this a lot moving logic into ConanInfo (not this PR).

Copy link
Member Author

@memsharded memsharded May 20, 2020

Choose a reason for hiding this comment

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

PkgA->PkgB->PkgC

PkgA has PACKAGE_ID_UNKNOWN, because we still need to build PkgC or PkgB and awe are in package_revision_mode

reevaluate_node will only be executed for PkgA. But the computation of transitive updated PackageReferences need to be computed for both PkgB and PkgC.

Does this clarify the issue a bit?

Copy link
Member

@jgsogo jgsogo May 20, 2020

Choose a reason for hiding this comment

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

Ok, we are running them to populate Node::id_direct_prefs and Node::id_indirect_prefs with the new values (are we doing this twice for pkgC?)

Copy link
Member Author

@memsharded memsharded May 20, 2020

Choose a reason for hiding this comment

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

Yes, it is being called twice. That is the key of the issue, we need to run this 2 times:

  • One to compute the first package_id, while analyzing the graph binaries
  • A second one, while installing the binaries, to update for possible changes upstream that will be used in reevaluate_node to compute the package_id of consumers with PACKAGE_ID_UNKNOWN.

It seems it would be very challenging to skip running this for leaf nodes that wouldn't need to run this.

node.id_indirect_prefs.difference_update(node.id_direct_prefs)
direct_reqs = node.id_direct_prefs
indirect_reqs = node.id_indirect_prefs
direct_reqs, indirect_reqs = node.package_id_transitive_reqs()
Copy link
Member

@jgsogo jgsogo May 20, 2020

Choose a reason for hiding this comment

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

Probably if we are moving only one branch this function doesn't belong to node, but to self, or we should move both branches.

Copy link
Member Author

@memsharded memsharded May 20, 2020

Choose a reason for hiding this comment

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

I put it in the node because now it needs to be called from 2 different locations: graph_binaries.py and installer.py. I could leave it here in the graph_binaries.py as a public static method that receives a single node as argument, no problem.

# the node transitive information necessary to compute the package_id
# as it will be used by reevaluate_node() when package_revision_mode is used and
# PACKAGE_ID_UNKNOWN happens due to unknown revisions
node.package_id_transitive_reqs()
Copy link
Member

@jgsogo jgsogo May 20, 2020

Choose a reason for hiding this comment

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

Because reevaluate_node only runs for those nodes that has PACKAGE_ID_UNKNOWN, but the information that needs to be re-computed after being built are the one of the dependencies of the node with PACKAGE_ID_UNKNOWN.

Here you mean, the CONSUMERS of the node with PACKAGE_ID_UNKNOWN, right?


Only full_transitive_package_id is causing the issue in this case, because if not defined, a different computation is done, based on conanfile.requires (which is updated).

Having a quick look to conanfile.requires, it is built using the direct and the indirect prefs too, so maybe we can simplify all of this a lot moving logic into ConanInfo (not this PR).

@fulara
Copy link
Contributor

@fulara fulara commented May 26, 2020

Hello @memsharded just tested it - but it doesnt seem to resolve the issue I have mentioned.

With this branch the:
self.info.requires.pkg_names
still contains the name of the currently processed conanfile - which was not the case on 1.24.0.

@memsharded
Copy link
Member Author

@memsharded memsharded commented May 26, 2020

Hi @fulara I have introduced a check in 40be838, seems to work fine. Can you please tell how to reproduce what you are seeing there?

@fulara
Copy link
Contributor

@fulara fulara commented May 26, 2020

@memsharded
Problem i've encountered and described happens when depending upon something, not when conan create'ing it: see:

As i've written in my original problem description:

conan create of bla will not print out any errors.
conan create of bla2 will print 2 errors bla and bla2.

described in depth here:
#6942 (comment)

I got this by looking at the logs - if you are seeing different behavior i can try to look in depth.

@memsharded
Copy link
Member Author

@memsharded memsharded commented May 26, 2020

I got this by looking at the logs - if you are seeing different behavior i can try to look in depth.

Yes, please, check again.

I have had a look, and added an extra check (75ebf2d) to the intermediate dependency, seems to be working fine. I have also had a look to the source code, but couldn't figure out what could be the reason of what you are seeing there.

@fulara
Copy link
Contributor

@fulara fulara commented May 26, 2020

I've checked this again - I think I came to wrong conclusions about the problem,
my build still fails - but I am not sure why yet,

I will get back to you.

Possibly my conclusion about the dependency being on the pkg_names was totally wrong, and I apologise for giving you the false hints.

I'll try to analyze the issue here and tell you the rootcause why the build fails for me.

@memsharded
Copy link
Member Author

@memsharded memsharded commented May 26, 2020

Possibly my conclusion about the dependency being on the pkg_names was totally wrong, and I apologise for giving you the false hints.

No problem! Thanks for the continuous feedback and support. Let me know if you have any further insight about this issue that I could check.

@fulara
Copy link
Contributor

@fulara fulara commented May 26, 2020

Hello @memsharded
Yes, my build tree now builds.

The issue seems to be that sometimes the build_requires are being added into the self.info.requires.
THey are being selectively added - only when the dependencies are actually being build,
I am not sure how exactly - I havent analyzed,

In my case with your changes the gtest was sometimes added to self.info.requires.pkg_names - which was not the case before.

What I observed was:
Change the default_package_mode
conan create . --build missing
This fails on some package. repeat:
conan create . --build missing

The building process goes over ( the build_requires are no longer on the self.info.requires.pkg_names list then. )

@memsharded
Copy link
Member Author

@memsharded memsharded commented May 27, 2020

Yes, finally understood the root cause:

  • package_revision_mode causes PACKAGE_ID_UNKNOWN
  • When rebuilding packages, the PACKAGE_ID_UNKNOWN needs to be re-evaluated, to compute a updated package-id
  • The package-id requires information of its dependencies, down to the package_revision, including transitive dependencies.
  • The accumulated information about transitive dependencies need to be recomputed for each node, while the node is finally installed in disk
  • The re-evaluation of transitive dependencies was pulling the build-requires too, that they didn't exist the first time the package_id was computed, because at that time, the build-requires were not expanded at all.

It has been fixed and a new red-green test added.

Thanks @fulara for all the work on this. Lets try to release this soon in 1.26.

@memsharded memsharded requested a review from jgsogo May 27, 2020
jgsogo
jgsogo approved these changes Jun 2, 2020
@memsharded memsharded merged commit dd9ca1e into conan-io:develop Jun 2, 2020
2 checks passed
@memsharded memsharded deleted the fix/multi_mode_package_id branch Jun 10, 2020
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.

3 participants