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

Feature: update conan new to latest guidelines #8106

Merged
merged 3 commits into from Dec 1, 2020

Conversation

SSE4
Copy link
Contributor

@SSE4 SSE4 commented Nov 25, 2020

update several minor things in conan new command:

  • add fPIC option handling
  • add homepage attribute
  • add header_only to package_id
  • update to cmake_minimum_required(VERSION 3.1)

this makes it easier to create package skeletons - fewer things need to be edited

Changelog: Feature: Update conan new to latest guidelines.
Docs: omit

#tags: slow

Signed-off-by: SSE4 <tomskside@gmail.com>
@memsharded memsharded added this to the 1.32 milestone Nov 25, 2020
Copy link
Contributor

@n-bes n-bes left a comment

Choose a reason for hiding this comment

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

Could you update test_cmake template? By default it is not allowed by hook KB-H048:

[HOOK - conan-center.py] pre_export(): ERROR: [CMAKE VERSION REQUIRED (KB-H048)] The test_package/CMakeLists.txt requires CMake 3.1 at least. Update to 'cmake_minimum_required(VERSION 3.1)'. (https://github.com/conan-io/conan-center-index/blob/master/docs/error_knowledge_base.md#KB-H048)

@SSE4
Copy link
Contributor Author

SSE4 commented Nov 28, 2020

Could you update test_cmake template? By default it is not allowed by hook KB-H048:

[HOOK - conan-center.py] pre_export(): ERROR: [CMAKE VERSION REQUIRED (KB-H048)] The test_package/CMakeLists.txt requires CMake 3.1 at least. Update to 'cmake_minimum_required(VERSION 3.1)'. (https://github.com/conan-io/conan-center-index/blob/master/docs/error_knowledge_base.md#KB-H048)

np, updated

@SSE4
Copy link
Contributor Author

SSE4 commented Nov 28, 2020

/cc @ericLemanissier @Croydon @madebr @uilianries @SpaceIm @prince-chrismc
lemme know if there is something else useful to update in that shot

Signed-off-by: SSE4 <tomskside@gmail.com>
Copy link
Contributor

@prince-chrismc prince-chrismc left a comment

Choose a reason for hiding this comment

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

A few suggestions because of comments that are common

conans/client/cmd/new.py Show resolved Hide resolved
Comment on lines +29 to +31
def config_options(self):
if self.settings.os == "Windows":
del self.options.fPIC
Copy link
Contributor

Choose a reason for hiding this comment

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

I am fairly certain this is also expected during review

Suggested change
def config_options(self):
if self.settings.os == "Windows":
del self.options.fPIC
def config_options(self):
if self.settings.os == "Windows":
del self.options.fPIC
def configure(self):
if self.options.shared:
del self.options.fPIC

Copy link
Member

Choose a reason for hiding this comment

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

Not completely sure, many recipes do not implement it yet. I would say wait until this is discussed a bit more, seems that some Conan built-in helpers would be better.

Copy link
Contributor Author

@SSE4 SSE4 Nov 30, 2020

Choose a reason for hiding this comment

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

agree

    def config_options(self):
        if self.settings.os == "Windows":
            del self.options.fPIC

this code has 361 occurrences on CCI

    def configure(self):
        if self.options.shared:
            del self.options.fPIC

this one is only 264.
so, probably, it might be too early to enforce that, as it's not an established practice on CCI.
earlier we also had proposal for so called shared_library_package_id to be used in recipes, I don't remember what was a conclusion.
we need to talk about these two IMO before we start to recommend them.

@memsharded memsharded assigned czoido and unassigned jgsogo Nov 30, 2020
@@ -145,6 +153,9 @@ def source(self):

def package(self):
self.copy("*.h", "include")

def package_id(self):
self.info.header_only()
Copy link
Member

Choose a reason for hiding this comment

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

I understand this is more explicit than just a recipe without settings, but we are adding more code here that is unneeded. If this is a pattern followed in CCI for example, then I am ok with it. Maybe we should consider it for conan 2.0 (enforce settings always?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess it might be error prone without explicit, e.g. if you add some settings and options to the recipe after running conan new, it's easy to forget about this method.
same if you decide to add more requirements, as header_only now contains:

self.requires.clear()

I believe the same package id is expected to be generated regardless of requires, which is not the case in the current template.

Copy link
Contributor

@jgsogo jgsogo left a comment

Choose a reason for hiding this comment

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

LGTM

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.

None yet

8 participants