explore if this fix header-only package-id

[memsharded]

Changelog: Feature: Implement a new package-ID computation that includes transitive dependencies even when the direct dependencies have remove them, for example when depending on a header-only library that depends on a static library.

Docs: conan-io/docs#1575

Close #6450

#tags: slow
#revisions: 1

[fulara]

Possibly proposed such test case already exists:

But consider adding testcase where libb is not header only - expectation would be that both libb and libc would require rebuild. - that should be a 'pass'

At least thats the current conan behavior - whether that should be expected is different thing :) ( I can see cases where one would like such behavior. )

Maybe conan should have a feature something like: 'propagates versioning_schema of dependencies upwards ) - yes I do realise that conan is working differently, just a thought.

[jgsogo]

For me, this is more a feature than a bugfix, I wasn't aware of this intended behavior (that's the reason I didn't talk about it in the blogpost about package_ids). Now I understand the direct in the semver_direct_mode mode 🎉

This change will affect recipe_revision_mode and package_revision_mode modes too, probably expected.

I found some breaking behavior (bugfix or breaking?):

• Add another package consuming your libd, and add self.info.requires.unrelated_mode() to the libc recipe. This PR modifies existing behavior: now libd becomes Missing too.

  ⚠️ If you use self.info.requires['libb'].unrelated_mode() in libc it doesn't behave the same way

  Should this unrelated_mode prevent the propagation of the indirect_prefs down the graph?

[fulara]

Hello @jgsogo If I may put my 2cents here

I propose to add a 'general' flag enabling this behavior.
I propose to add a possibility to override this flag per requires in package_id
I propose to explain this in a docs about versioning_scheme.

Now in Ideal world I would:
a) enable this feature by default - which what this PR does.

In current world I would:
b) disable this feature by deafult.

I personally would enable this feature the moment I see it,
And I would reocmmend the flag being enabled by default in 2.0

Why?

• Because I consider this change to be breaking in to conan specification of how dependencies propagate.
• Because I think for some use cases this functionality should be chosen by users
  I can imagine sometimes a libraries hidden behind this functionality.

[memsharded]

Hi @fulara @jgsogo

I have added 2 tests, for the above cases.

@jgsogo . The test:

    def transitive_unrelated_test(self):
        # https://github.com/conan-io/conan/issues/6450
        client = TestClient()
        client.run("config set general.default_package_id_mode=full_version_mode")
        # LibA
        client.save({"conanfile.py": GenConanfile()})
        client.run("create . liba/1.0@")
        client.run("create . liba/2.0@")
        # libB -> LibA
        client.save({"conanfile.py": GenConanfile().with_require_plain("liba/1.0")})
        client.run("create . libb/1.0@")
        # libC -> libB
        unrelated = "self.info.requires['libb'].unrelated_mode()"
        client.save({"conanfile.py": GenConanfile().with_require_plain("libb/1.0")
                                                   .with_package_id(unrelated)})
        client.run("create . libc/1.0@")
        # LibD -> LibC
        client.save({"conanfile.py": GenConanfile().with_require_plain("libc/1.0")})
        client.run("create . libd/1.0@")
        # LibE -> LibD, LibA/2.0
        client.save({"conanfile.py": GenConanfile().with_require_plain("libd/1.0")
                                                   .with_require_plain("liba/2.0")})
        client.run("create . libe/1.0@", assert_error=True)
        self.assertIn("liba/2.0:5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9 - Cache", client.out)
        self.assertIn("libb/1.0:e71235a6f57633221a2b85f9b6aca14cda69e1fd - Missing", client.out)
        self.assertIn("libc/1.0:e3884c6976eb7debb8ec57aada7c0c2beaabe8ac - Missing", client.out)
        self.assertIn("libd/1.0:9b0b7b0905c9bc2cb9b7329f842b3b7c6663e8c3 - Missing", client.out)

Seems to work the same with and without the patch. I might be missing something, please check.

[jgsogo]

In your new test, use unrelated = "self.info.requires.unrelated_mode()" instead of using ...requires['libb']....

There are two different things here:

1. self.info.requires.unrelated_mode() instead of self.info.requires['libb'].unrelated_mode() results in different behavior (not for this PR)
2. self.info.requires.unrelated_mode() works different now and then

[memsharded]

self.info.requires.unrelated_mode() instead of self.info.requires['libb'].unrelated_mode() results in different behavior (not for this PR)

self.info.requires.unrelated_mode() works different now and then

I'd say both are totally expected. In the first one, it is different to be unrelated of one of your dependencies, and the other to be completely unrelated to all of your dependencies.

In the second one, yes, the behavior is different. Without this PR, it will find a existing binary for a package, even if it is in full_version_mode and one of its dependencies changed to a new major version. I think this should be considered a bug with the current status and documentation. With this PR, now it will say that such binary is missing and will try to build it.

[jgsogo]

I need to review all this PR again because I was mistaken about the content of info.requires member. I'll review my comments too because many of them do not apply 🙈

[jgsogo]

I think this change is needed and it is indeed a bug, but probably it is one of those bugs that people depend on and it will break existing package_ids.

I mostly agree with @fulara here: #6451 (comment)

[fulara]

Sorry @memsharded @jsgogo but with the way things are right now, I think this PR has a potential to break a lot of stuff.

If i correctly understand the changes (I dont currently have a way to test conan behavior out of private branches. ) Consider below scenario:

global_mode: set to full_package_mode.
liba.
libb depends on liba with mode 'semver_mode' (set in package_id())
libc depends on libb with semver_mode.

If i correctly understand the changes done within this PR, libc will depends transitively by global_mode on liba, which seems very wrong... as it probably should depend via semver_mode here.
is my understanding correct?
If my understanding is wrong you can ignore thest of this comment.

In other words my question really is, whats the current conan behavior of inheriting of versioning_schema from transitive dependencies?
With this change it seems that it will default to 'global_mode' which actually now seems quite wrong?
I am now actually inclined to say that it should inherit mode from the tree of dependencies and maybe some kind of new switch is needed for header_only?
Something like: "I am agnostic about dependencies, but I can still steer how my dependencies will transitively affect my dependers."

Inheriting from the tree of dependencies should be maybe based of scanning the whole tree and taking a minimum from the dependencies?
(I think this conflicts with your vision of applying versioning_schema.. :) )

I think for header_only library below should still be valid (maybe)
in header_only:

def package_id:
#this combination will make it so that my dependencies does not affect myself, but affect my depender
self.info.requires.patch_mode()
self.info.requires.agnostic_mode()

Is this good idea? I dont know.
All I think I know at the moment is that PR as it stands has a potential of breaking lots of things. Unless I totally misunderstood things.

[jgsogo]

You can easily test Conan from any repo/branch, for example for this PR you can use:

pip install git+https://github.com/memsharded/conan.git@feature/fix_header_only_package_id

[jgsogo]

Thanks a lot, @fulara, for sharing all your feedback about this issue.

Your understanding is right, there is no inheritance of the package_id mode across dependencies. Each package depends on all the requirements (also transitive requirements) and a recipe can override the package_id mode for all of them (self.info.requires.unrelated_mode()) or for one of them (self.info.requires['libb'].unrelated_mode()).

We might modify the graph model for Conan 2.0 and it is an open issue: https://github.com/conan-io/conan/projects/8, a lot of things can change, but they should be moved to a different issue and grouped together.

For now, I totally agree that this PR is a bug fix, but we need an opt-in for this new behavior (it will be the default when using CONAN_V2_MODE), otherwise it can break many deployments.

[jgsogo]

@memsharded, please, add an opt-in for this new behavior

[fulara]

thanks @jgsogo for that hint about testing custom packages, that was helpful.

I stand by my opinion after testing it. see below example. (Now I only need to learn to run your unit tests so mb with next iteration it will be easier :) for you.

Example:

#as usual - default_pakcage_id is full_package_mode.
#liba
class Conan(ConanFile):
    name = "liba"
    pass
#libb
class Conan(ConanFile):
    name = "libb"
    requires="liba/1.0.0"
    
    def package_id(self):
        self.info.requires["liba"].semver_mode()
#libc
class Conan(ConanFile):
    name="libc"
    requires="libb/1.0.0"

    def package_id(self):
        self.info.requires["libb"].semver_mode()
#libd
class Conan(ConanFile):
    name="libd"
    requires="libc/1.0.0", "liba/1.0.1"   # note!! we are overriding liba with 1.0.1 here. 

    def package_id(self):
        self.info.requires["libc"].semver_mode()
        self.info.requires["liba"].semver_mode()

conan create:
liba 1.0.0@, liba 2.0.0@, libb 1.0.0@, libc 1.0.0@
Now when we try to conan create libd, because we have overrode it with 1.0.1 in libd project wont compile with this change. error:

ERROR: Missing prebuilt package for 'libc/1.0.0'
Try to build it from sources with "--build libc"

Because libc depends now by 'default_package_id_mode' on liba.

So even though User went out of their length (thats what we are doing now, because we dont like depending on global package_id_mode. ) to define versioning_schema on every single of the direct dependencies they had, they ended up having the dependency liba be defaulted into default_package_id.

Actually Now that I am aware of this, I could implement a workaround into our conan wrappers, we will always define a versioning_schema for everything, automatically ( fetch it from our db ), by iterating over self.requires object. So if this solution is inline with your expectation of the behavior, I'll manage. :)

However I think the behavior following this PR will be very confusing to some people out there.

[jgsogo]

Running the test suite should be easy too:

• First of all, I highly recommend you to use a virtualenv: python3 -m venv <name>, it will create a directory <name> with an isolated python installation
• Activate the virtualenv: source <name>/bin/activate (for Windows, there is a .bat file inside)
• Clone you preferred Conan version: git clone ... and cd conan
• Now you need to install all the requirements: pip install conans/requirements....txt (there is a requirements_dev.txt file that will install dependencies for testing)
• Run the test suite using nose: nose conans.test (or run nose conans.test.unittests for tests inside that module) (or run from inside the IDE if you are using one). Some tests require a compiler, CMake, SVN, Git,... so they will fail if the tool is not installed.

If you need anything else, let me know (you can write to me in Slack too if you prefer)

[memsharded]

Yes, I agree with you. I will introduce an opt-in to be able to apply the bug-fixed behavior, and not change anything otherwise.

[memsharded]

Added general.fix_transitive_package_id=1 to opt-in to this behavior. Please review.

[jgsogo]

👍
I think that the changelog should categorize it as a feature, and we will need clear docs for this one.

For CONAN_V2_MODE (#6490) I will consider it a bugfix and the user won't be able to run the buggy behavior, I think this is the way to go.

[memsharded]

I think that the changelog should categorize it as a feature, and we will need clear docs for this one.

I agree, changed the Changelog already. Should I change then the config.fix_transitive_package_id to something like config.full_transitive_package_id? Tell me and I will complete this with new docs.

[jgsogo]

To be honest, I don't know how to name that config parameter. I don't like the fix in it, that's for sure. Maybe full_closure_package_id?

[fulara]

Okay, so I finally had the tiem to pick this up again.
I have now 2 questions.

1. How do I get a list of requires so I can mark all the requires the way I want with this fix?
   What I would like to do is something along the lines of..
   I currently have a mapping defined within my aproach, in that mapping we decide what sort of versioning_schema particular library has - We feel thats the only aproach that we can have with current way of conans things. I would like to do something like this:

def package_id(self):
    for req in self.info.requires:
        mode = mylookup[req.name].get_mode();
        #eval req.mode()

We are doing something exactly like this ( maybe it will be actually presented on conan-days by one of my colleagues. ) but only on direct_dependencies.

How do I get to a list of all requires component after this PR? Because I have to, I cant have default_package_id.
Currently the only thing that does something like this is semver_direct_mode However there is no documentation that actually explains how that mode achieves this.

There is no documentation of self.info that shows how to achieve this. I found only this: https://docs.conan.io/en/latest/reference/conanfile/methods.html#self-info

When I try to iterate over self.into.requires i get:

for p in self.info.requires:
	ConanException: No requirement matching for 0

When brieflly skimming through the code I found this:

    def semver_direct_mode(self):
        for r in self._data.values():
            r.semver_direct_mode()

Seems like it uses _data which obviously is a private, so is there no way for me to do it?
I think I wont be able to use this fix in the end.

2. I am thinking of raising a feature request, for conan to provide an ability to inherit versioning_schema for transitive dependencies from the source dependencies.
   If no objections here I will go ahead and do that ( if no objections feel free to ignore this point. )

[memsharded]

Hi @fulara

I think you could try to use something like:

def package_id(self):
    for pkg_name in self.info.requires.pkg_names:  # Iterate all info.requires
        req_info = self.info.requires[pkg_name ]
        # You can set the mode
        req_info.minor_mode()
        # no representation of the mode, but you have access to
        # self.name, self.version,self.user, self.channel, self.package_id , self.recipe_revision, self.package_revision 
       # You can deduce the mode from this data

It is not very straightforward, but at least not using private data.

Regarding 2) I am not sure it is a good idea. If you mean that when LibB-(depends on)->LibA, and LibA is setting a certain mode, then suddenly LibB starts using that mode, then I think it might not be a good idea. If LibA has a mode "unrelated" or "header-only", that doesn't mean that its dependents should also have this mode. This will also raise a bunch of issues regarding diamonds (conflicts). If it is a way of propagating upstream (lets use the mode of LibB in LibA and all its dependencies, I think I don't see it either, for the same reason. I would say that propagation in the graph shouldn't be the approach for this issue. If what you want is to build your own custom package_id() for all your packages, then maybe the way to go is to put that logic in a new python_requires and use it from there. Please let me know what you think.

[jgsogo]

I think it is ready, I'd like to read the docs before approving

(And we should open a new issue targetting 2.0 to make this the default behavior --> probably something for CONAN_V2_MODE)

[fulara]

thanks @memsharded for that, I will test it out and probably do that.
We are already using python_requires and providing custom package_id for users to invoke. I'll test that lookup and if I come across any issues I'll surely raise it to you guys! :)

Further on mine 2) following your comments.
My personal views:

1. ... If LibA has a mode "unrelated" or "header-only", that doesn't mean that its dependents should also have this mode. ... - well, if they depend transitively on that package then they should inherit, if they depend directly then they should take it from default_mode. If you depend transitively multiple times then there should be some kind of comparision between these modes, and take the strongest one.
   However that conflicts with header_only, but I recognize the current header_only as a hack :)

2. .. I would say that propagation in the graph shouldn't be the approach for this issue. .. I slightly agree, but I dont think relying on default mode is much better, its basically the same, very confusing for anyone. Its maybe less confusing now, because conan default mode is semver - which is pretty much the least strict option ( and very error prone, but thats another story. ) But if anyone switches default mode to full_package_mode for example, and uses semver in package_id he will be very very confused whats going on.
   Unless you were to explain that in docs ofc. which at the moment is not that well explained at all, and probably best proof of that I myself only understood semver_direct_mode after this bug.
   And further on that, if anyone would be using semver mode for one of the packages, but full_package_mode for any of his packages, then immediatelly he will probably come to a conclusion that now he needs to have some kind of python_requires package. Unless of course he will deal with only few packages ( I deal with dozens if not hundreds here.. ) - then he can set versioning schema for transitive dependencies.

Anyway, thanks for this fix, I will be incorporating that into my environment when 1.23 comes out.

[fulara]

Hello @memsharded

Just a note, using your proposed code:

        for pkg_name in self.info.requires.pkg_names:
            req_info = self.info.requires[pkg_name]
            req_info.minor_mode()

Causes all transitive dependencies to appear in the requires section in conaninfo (basically mimics the behavior after this patch enabled ) in 1.21.0

I was worried that it would change the way conan info -g graph outputs but that does not seems to be the case - I checked that with loop on 1.21.0.

[memsharded]

Causes all transitive dependencies to appear in the requires section in conaninfo (basically mimics the behavior after this patch enabled ) in 1.21.0

Yes, as long as the transitive dependencies are not cleared with something like header_only(), if using a "transitive" package-id mode like minor_mode, will make transitive dependencies to be considered in the package-id, and thus reflected in the conaninfo.txt. That was the intended behavior from the beginning, it is just that using info.header_only() produced a bug and broke that transitivity.

I was worried that it would change the way conan info -g graph outputs but that does not seems to be the case - I checked that with loop on 1.21.0.

Yes, the -g graph is not using the conaninfo.txt, that is used mostly for the return of the search command and displaying in the server side.

[fulara]

@memsharded well. Okay - it seems that it was my misunderstanding again, so they were transitive already,

Today we have tested our new version and we received scary missing prebuilt and it was very hard to debug that, lost around 2h looking at conaninfo.txt to understand the root cause. In summary if you set package_id once, usually you will have to select it for all your packages in the tree. So yes -you are totally correct as usual: That was the intended behavior from the beginning, .

Anyway:
Please, do consider more debugging for missing prebuilt :( This is actually my biggest pain with conan at the moment. example: #6364

I am waiting for 1.23.0!

[DoDoENT]

I've just seen this PR after reading the release notes for v1.23.

Isn't that basically the partial implementation of #4038 (i.e. only special case for header-only packages)?

[memsharded]

Isn't that basically the partial implementation of #4038 (i.e. only special case for header-only packages)?

Nop, I don't think so. Actually the opposite, this feature recovers the effect that upstream dependencies have over consumers packageID, when there are header-only dependencies in the middle. It does not change how the cpp_info is propagated, so it doesn't encapsulate headers in any way.

The docs contain a graphic in conan-io/docs#1575, maybe that one is useful to illustrate the problem that this PR is resolving.

[DoDoENT]

... this feature recovers the effect that upstream dependencies have over consumers packageID, when there are header-only dependencies in the middle

Exactly that. From the discussion on #4038, the header-only package is always treated as all its dependencies are api dependencies, not implementation dependencies (i.e. they are not encapsulated).

So, from the example in the documentation, with terminology from my comment in #4038, the PkgB would have an api dependency on PkgA, which means that PkgC, when depending on PkgB must also change it's package ID whenever package ID of either PkgA or PkgB change. I said partial implementation because in case of PkgB being header-only, it's package ID can never change. But this is only a special case of #4038.

[memsharded]

Oh, yes, in that regard yes, that is true, PkgB package ID will never change if header-only, but at least with this fix, its consumers will be affected by the upstream PkgA.

[DoDoENT]

So, in a way #4038 is a generalization over that, as it also allows for non-header-only packages to have that same behaviour (e.g. some features of my package C are just header-only wrappers around some headers from upstream packages A and B and some other features are provided directly by my package in binary - in that case, I would expect consumers of my package (e.g. D) to change their package ID whenever package ID of my package changes (i.e. of C) or package ID of my dependencies which headers I wrap (i.e. both A and B)).

[memsharded]

But that is already the behavior of other package_id_modes rather than the default one (semver_direct_mode) that is only affected by its direct dependencies. Those modes take into account all of the transitive dependencies, but there was this issue when a header-only package was in the middle of the chain.