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.

Sign up for GitHub

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
Merged

Feature: update conan new to latest guidelines #8106

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6d46b12
- update conan new to latest guidelines
SSE4 Nov 25, 2020
29d6ba3
- update cmake_minimum_required(VERSION 3.1)
SSE4 Nov 28, 2020
4ab6dc7
- fix test
SSE4 Nov 28, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 19 additions & 8 deletions conans/client/cmd/new.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,14 @@ class {package_name}Conan(ConanFile):
description = "<Description of {package_name} here>"
topics = ("<Put some tag here>", "<here>", "<and here>")
settings = "os", "compiler", "build_type", "arch"
options = {{"shared": [True, False]}}
default_options = {{"shared": False}}
options = {{"shared": [True, False], "fPIC": [True, False]}}
default_options = {{"shared": False, "fPIC": True}}
generators = "cmake"

def config_options(self):
if self.settings.os == "Windows":
del self.options.fPIC
Comment on lines +29 to +31
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 •
edited
Loading

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.


def source(self):
self.run("git clone https://github.com/conan-io/hello.git")
# This small hack might be useful to guarantee proper /MT /MD linkage
Expand Down Expand Up @@ -91,11 +95,15 @@ class {package_name}Conan(ConanFile):
description = "<Description of {package_name} here>"
topics = ("<Put some tag here>", "<here>", "<and here>")
settings = "os", "compiler", "build_type", "arch"
options = {{"shared": [True, False]}}
default_options = {{"shared": False}}
options = {{"shared": [True, False], "fPIC": [True, False]}}
default_options = {{"shared": False, "fPIC": True}}
SSE4 marked this conversation as resolved.
Show resolved Hide resolved
generators = "cmake"
exports_sources = "src/*"
{configure}
def config_options(self):
if self.settings.os == "Windows":
del self.options.fPIC

def build(self):
cmake = CMake(self)
cmake.configure(source_folder="src")
Expand Down Expand Up @@ -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.

"""


Expand Down Expand Up @@ -175,7 +186,7 @@ def test(self):
self.run(".%sexample" % os.sep)
"""

test_cmake = """cmake_minimum_required(VERSION 2.8.12)
test_cmake = """cmake_minimum_required(VERSION 3.1)
project(PackageTest CXX)

include(${CMAKE_BINARY_DIR}/conanbuildinfo.cmake)
Expand All @@ -191,7 +202,7 @@ def test(self):
# COMMAND example)
"""

test_cmake_pure_c = """cmake_minimum_required(VERSION 2.8.12)
test_cmake_pure_c = """cmake_minimum_required(VERSION 3.1)
project(PackageTest C)

include(${CMAKE_BINARY_DIR}/conanbuildinfo.cmake)
Expand Down Expand Up @@ -250,7 +261,7 @@ def test(self):
}}
"""

cmake_pure_c = """cmake_minimum_required(VERSION 2.8)
cmake_pure_c = """cmake_minimum_required(VERSION 3.1)
project({name} C)

include(${{CMAKE_BINARY_DIR}}/conanbuildinfo.cmake)
Expand All @@ -259,7 +270,7 @@ def test(self):
add_library({name} {name}.c)
"""

cmake = """cmake_minimum_required(VERSION 2.8)
cmake = """cmake_minimum_required(VERSION 3.1)
project({name} CXX)

include(${{CMAKE_BINARY_DIR}}/conanbuildinfo.cmake)
Expand Down
5 changes: 0 additions & 5 deletions conans/test/functional/generators/make_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,6 @@ class MakeGeneratorTest(unittest.TestCase):
def test_complete_creation_reuse(self):
client = TestClient(path_with_spaces=False)
client.run("new myhello/1.0.0 --sources")
conanfile_path = os.path.join(client.current_folder, "conanfile.py")
replace_in_file(conanfile_path, "{\"shared\": [True, False]}",
"{\"shared\": [True, False], \"fPIC\": [True, False]}", output=client.out)
replace_in_file(conanfile_path, "{\"shared\": False}", "{\"shared\": False, \"fPIC\": True}",
output=client.out)
client.run("create . danimtb/testing")
hellowrapper_include = """
#pragma once
Expand Down