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

possible fix for develop attribute with export-pkg #6585

Merged
merged 7 commits into from Mar 6, 2020

Conversation

memsharded
Copy link
Member

@memsharded memsharded commented Feb 24, 2020

Changelog: BugFix: Set the self.develop=True attribute for recipes when they are used with conan export-pkg, in all methods, it was previously only setting it for the package() method.
Docs: Omit

Fix #6583

This is a exploratory PR, to see if something breaks. Not guaranteed it will be merged, need to understand better if it could break something.

@memsharded memsharded marked this pull request as ready for review Mar 5, 2020
@memsharded memsharded added this to the 1.23 milestone Mar 5, 2020
Copy link
Member

@jgsogo jgsogo left a comment

I see it is a small change, but it modifies the graph that is created: from a consumer one to a test_package one. I can imagine an scenario where expanding the graph to the test_package will download more recipes or even fail if using a graphlock.

if create_reference: # Test_package -> tested reference

... nevertheless, I would say it has low risk because affects only to one command and can be merged, but I think we should improve the "tricky, but works" part, can we?

deps_graph = graph_manager.load_graph(ref, None, graph_info=graph_info, build_mode=[ref.name],
# passing here the create_reference=ref argument is useful so the recipe is in "develop",
# because the "package()" method is in develop=True already
deps_graph = graph_manager.load_graph(ref, ref, graph_info=graph_info, build_mode=[ref.name],
check_updates=False, update=False, remotes=remotes,
recorder=recorder, apply_build_requires=False)
# this is a bit tricky, but works. The root (virtual), has only 1 neighbor,
Copy link
Member

@jgsogo jgsogo Mar 5, 2020

Choose a reason for hiding this comment

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

If we are merging this, I think we cannot trust in this trick anymore. Now, with the test_package the root has two neighbours and I don't know if the nodes[0] will be always the one we want (I cannot run Conan now, and having a look to the sources in the browser is a bit harder...)

@memsharded
Copy link
Member Author

@memsharded memsharded commented Mar 6, 2020

I see it is a small change, but it modifies the graph that is created: from a consumer one to a test_package one. I can imagine an scenario where expanding the graph to the test_package will download more recipes or even fail if using a graphlock.

No, it is not true. The graph is not modified at all, only the loader.dev_reference is assigned, that is the only effective change.

@jgsogo
Copy link
Member

@jgsogo jgsogo commented Mar 6, 2020

What I see is that, from the _load_root_node we start to call

return self._load_root_test_package(path, create_reference, graph_lock, profile)

instead of

root_node, ref = self._load_root_consumer(path, graph_lock, profile, graph_info.root)

but yes, having a look to the sources it looks like there is not much difference between those two functions.

Ok then

jgsogo
jgsogo approved these changes Mar 6, 2020
@memsharded
Copy link
Member Author

@memsharded memsharded commented Mar 6, 2020

What I see is that, from the _load_root_node we start to call

Nop. Check the conditional:

 if isinstance(reference, ConanFileReference):
            return self._load_root_direct_reference(reference, graph_lock, profile)

@jgsogo
Copy link
Member

@jgsogo jgsogo commented Mar 6, 2020

OMG, there is another if before to call a different _load_xxx! 😰

Now I see, this is almost changing nothing 👍

@jgsogo jgsogo merged commit bc0a1bf into conan-io:develop Mar 6, 2020
2 checks passed
@memsharded memsharded deleted the feature/fix_develop_export_pkg branch Mar 6, 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.

2 participants