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

config of package_id default mode #4644

Merged
merged 9 commits into from Mar 6, 2019

Conversation

Projects
None yet
4 participants
@memsharded
Copy link
Contributor

commented Mar 1, 2019

Changelog: Feature: Allow configuring in conan.conf a different default package_id mode.
Docs: conan-io/docs#1106

Close #4071

@ghost ghost assigned memsharded Mar 1, 2019

@ghost ghost added the stage: review label Mar 1, 2019

@memsharded memsharded added this to the 1.13 milestone Mar 1, 2019

@memsharded memsharded assigned lasote and unassigned memsharded Mar 1, 2019

@ghost ghost assigned memsharded Mar 4, 2019

@jgsogo

This comment has been minimized.

Copy link
Member

commented Mar 5, 2019

This one is targeting (and closing) #4071, isn't it?

Show resolved Hide resolved conans/client/conf/__init__.py Outdated
try:
return self.get_item("general.package_id_mode")
except ConanException:
return None

This comment has been minimized.

Copy link
@lasote

lasote Mar 5, 2019

Contributor

Should the value be None or semver? Why not present in the conan.conf? I would like to see default_package_id_mode=semver explicit, always. In case someone doesn't have it, return "semver" instead of None

This comment has been minimized.

Copy link
@lasote

lasote Mar 5, 2019

Contributor

The values shouldn't contain the "_mode" suffix.

This comment has been minimized.

Copy link
@danimtb

danimtb Mar 5, 2019

Member

I would like to see the possible values as comments in the conan.conf too. I even like the idea of having a link to the docs section if we feel this feature is important enough...

This comment has been minimized.

Copy link
@memsharded

memsharded Mar 5, 2019

Author Contributor

It is different to not define a new default_package_id_mode, than defining it to semver. This is the logic:

# sha values
        if package_id_mode:
            try:
                getattr(self, package_id_mode)()
            except AttributeError:
                raise ConanException("'%s' is not a known package_id_mode" % package_id_mode)
        else:
            if indirect:
                self.unrelated_mode()
            else:
                self.semver_mode()

So if defaults to semver it will never apply the current unrelated_mode() that is applying to indirect (transitive, but not declared) dependencies, changing the current behavior.

Maybe we need to discuss this, but I thought that the new default mode would apply to all dependencies, transitive indirect dependencies too.

This comment has been minimized.

Copy link
@danimtb

danimtb Mar 5, 2019

Member

I really fail to see the whole picture now, so I will need a brief explanation

Show resolved Hide resolved conans/client/graph/graph_binaries.py Outdated
Show resolved Hide resolved conans/test/functional/graph/package_id_modes_test.py
""" parse the input into fields name, version...
"""
pref = PackageReference.loads(value_str)
def __init__(self, pref, indirect=False, package_id_mode=None):

This comment has been minimized.

Copy link
@lasote

lasote Mar 5, 2019

Contributor

I think package_id_mode shouldn't be optional.

Show resolved Hide resolved conans/model/info.py
@jgsogo

This comment has been minimized.

Copy link
Member

commented Mar 5, 2019

A comment about naming: I think it should resemble the naming we already have documented: semver_mode, major_mode,... so the suffix might be redundant, but I think it should be there.

@lasote

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2019

One thing is the mode in the config, and a different one is the helpers in the package_id.
In the config default_package_id_mode=semver_mode is redundant

@jgsogo

This comment has been minimized.

Copy link
Member

commented Mar 5, 2019

Totally redundant, but the user will search for semver_mode (or any other mode) and will find the documentation about the default_package_id_mode in the conan.conf and the docs about the ABI compatibility definition (I also think that maybe the name should be default_abi_compatibility instead of default_package_id_mode)

I'm just commenting these things in case you haven't realized about them; naming is hard, keeping the docs updated too, linking one section of the docs with other sections is also important. I think that having the same naming for the strings that the user has to manage is better, I'm not saying anything about the naming of the variables. But let's ping @danimtb, as the guardian of the docs his opinion about naming is important too.

@danimtb

This comment has been minimized.

Copy link
Member

commented Mar 5, 2019

Although it may be redundant, I think it is better to keep the same naming as in that section. Users are not very used to this kind of use cases and I think the section is well documented, so having to explain the modes twice is not necessary.

I am not sure about the default_abi_compatibility name, as there might be options not necessarily affecting the ABI like having a package with tests or stuff like that

@lasote

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2019

Totally opposed to default_abi_compatibility

@lasote lasote self-requested a review Mar 6, 2019

@lasote
Copy link
Contributor

left a comment

Pending create and document a new mode describing the default behavior and introduce it as the default, visible, in the conan.conf and return it instead of the None

@lasote

lasote approved these changes Mar 6, 2019

Copy link
Contributor

left a comment

Conficts!

@danimtb

danimtb approved these changes Mar 6, 2019

Show resolved Hide resolved conans/model/info.py
Show resolved Hide resolved conans/test/unittests/model/transitive_reqs_test.py Outdated

memsharded added some commits Mar 6, 2019

@lasote lasote merged commit 84d178f into conan-io:develop Mar 6, 2019

2 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
license/cla Contributor License Agreement is signed.
Details

@ghost ghost removed the stage: review label Mar 6, 2019

@memsharded memsharded deleted the memsharded:feature/new_package_id_modes branch Mar 6, 2019

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