Add libftdi versions 1.2-1.5. #3957

planetmarshall · 2020-12-19T17:24:42Z

Specify library name and version: lib/1.0

[x ] I've read the guidelines for contributing.
[ x] I've followed the PEP8 style guides for Python code in the recipes.
[ x] I've used the latest Conan client version.
[ x] I've tried at least one configuration locally with the
conan-center hook activated.

To avoid conflict with libftdi 0.x releases

madebr · 2020-12-19T17:30:13Z

recipes/libftdi/1.x/conanfile.py

+        self.cpp_info.includedirs.append(
+            os.path.join(self.package_folder, "include", "libftdi1")
+        )
+        self.cpp_info.libs = ["ftdipp1", "ftdi1"]


You combine both libraries.
Does the cmake config file, installed by libftdi's cmake config (and removed by us), create separate targets for the c and c++ library?
Maybe this requires conan components?

These batch of errors (the .pc related errors) show that you need to use components,
and set the pkg_config names.

I'm not sure it's possible to exactly mimic LibFTDI's CMake configuration with conan (Libftdi uses LibFTDI1 as the package name but LIBFTDI and LIBFTDIPP as the package variable names), but I've attempted to get as close as I can with conan components using the targets LibFTDI1::ftdi and LibFTDI1::ftdipp.

Conan has 3 settings

Filenames (sets the cmake package name, is find_package)
Names (the cmake namespace of the targets)
Components names (each unique target)

I am sure we have an example I just can't recall one.

The find_package name is LibFTDI1, as used in examples/cmake_example. The built-in find package then generates the variables $LIBFTDI_LIBRARIES and $LIBFTDIPP_LIBRARIES,

However the variables generated by the conan-generated find package when using components are ${LibFTDI1_component_LIBRARIES} etc. I'm not sure it's possible to create multiple top-level variable names with conan, with a different name from the find_package (I did try various combinations as you suggested).

The targets created by LibFTDI are not namespaced, which I know conan doesn't support. See also conan-io/conan#7615

Yes since the variables fashion is outdated its unlikely that conan will add support for that. So yes it seems this can not be done.

madebr · 2020-12-19T17:31:29Z

recipes/libftdi/1.x/conanfile.py

+        self.cpp_info.includedirs.append(
+            os.path.join(self.package_folder, "include", "libftdi1")
+        )


No need for absolute paths.
.includedirs can contain relative paths.
self.deps_cpp_info[xxx].include_paths automatically converts these to absolute paths.

Suggested change

self.cpp_info.includedirs.append(

os.path.join(self.package_folder, "include", "libftdi1")

)

self.cpp_info.includedirs.append(os.path.join("include", "libftdi1"))

recipes/libftdi/1.x/conandata.yml

ghost · 2020-12-19T17:55:21Z

I detected other pull requests that are modifying libftdi/1.x recipe:

Add libftdi recipes #1880

This message is automatically generated by https://github.com/ericLemanissier/conan-center-conflicting-prs so don't hesitate to report issues/improvements there.

prince-chrismc

Please fix the license

I would allow appreciate if you could try the patch but it's not required to merge

prince-chrismc · 2020-12-26T20:20:58Z

recipes/libftdi/1.x/conanfile.py

+class LibFtdiConan(ConanFile):
+    name = "libftdi"
+    description = "A library to talk to FTDI chips"
+    license = ["GNU LGPL v2.1"]


https://www.intra2net.com/en/developer/libftdi/
https://spdx.org/licenses/LGPL-2.1.html

Suggested change

license = ["GNU LGPL v2.1"]

license = "LGPL-2.1"

prince-chrismc · 2020-12-26T21:07:59Z

recipes/libftdi/1.x/conanfile.py

+    def _patch_cmakelists(self, subfolder):
+        cmakelists_path = os.path.join(self._source_subfolder, subfolder, "CMakeLists.txt")
+        tools.replace_in_file(cmakelists_path, "CMAKE_SOURCE_DIR", "PROJECT_SOURCE_DIR", strict=False)
+        tools.replace_in_file(cmakelists_path, "CMAKE_BINARY_DIR", "PROJECT_BINARY_DIR", strict=False)


Looking over the project. I think we can do sometime much less aggressive.

Can you please try to add the following? This way we can contribute it back to upstream 🤞

cmake-allow-subproject.patch.txt

prince-chrismc · 2020-12-26T21:11:05Z

recipes/libftdi/1.x/conanfile.py

+        tools.rmdir(os.path.join(lib_folder, "pkgconfig"))
+        # STATIC config creates both static and shared libraries
+        if not self.options.shared:
+            tools.remove_files_by_mask(lib_folder, "*.so*")


https://docs.conan.io/en/latest/changelog.html#id127

Please add required_conan_version

madebr · 2020-12-27T02:51:55Z

@planetmarshall
Please consider planetmarshall#1

prince-chrismc · 2021-02-05T14:07:04Z

Missing MacOS framework symbols? ocornut/imgui#2546 (comment)

conan-center-bot · 2021-02-06T17:09:18Z

All green in build 13 (0e4f0e5fcbedb34af238a169146be26341e0a45c):

libftdi/1.5@:
All packages built successfully! (All logs)

prince-chrismc

Try aim to keep the patches to a minimum. These modification make them less portable to newer versions and the recipe harder to maintain...

#3903 and the related PR have a lot of discussion on the topic

I feel this patch is far larger than is required and there's a lot of non trivial changes. I stopped marking but I scanned over the file.

➡️ Please reduce this patch to only the essential changes

prince-chrismc · 2021-02-06T20:02:43Z

recipes/libftdi/1.x/patches/0001-cmake-targets.patch

+-# Project
+ project(libftdi1 C)
+


prince-chrismc · 2021-02-06T20:04:31Z

recipes/libftdi/1.x/patches/0001-cmake-targets.patch

+-# CMake
+ if("${CMAKE_BUILD_TYPE}" STREQUAL "")
+    set(CMAKE_BUILD_TYPE     RelWithDebInfo)
+ endif("${CMAKE_BUILD_TYPE}" STREQUAL "")
+ set(CMAKE_COLOR_MAKEFILE ON)
+-cmake_minimum_required(VERSION 2.6 FATAL_ERROR)
+
+cmake_minimum_required(VERSION 3.0 FATAL_ERROR)


prince-chrismc · 2021-02-06T20:05:11Z

recipes/libftdi/1.x/patches/0001-cmake-targets.patch

+if(POLICY CMP0057)
+    cmake_policy(SET CMP0057 NEW)
+endif()
+if(POLICY CMP0011)
+    cmake_policy(SET CMP0011 NEW)
+endif()
+if(POLICY CMP0099)
+    cmake_policy(SET CMP0099 NEW)
+endif()


Please explain why you are adjusting the policies, these do not seem required

If the policies are not set, there are warnings in the CMake log for recent versions.

prince-chrismc · 2021-02-06T20:06:09Z

recipes/libftdi/1.x/patches/0001-cmake-targets.patch

+-message("-- Build type: ${CMAKE_BUILD_TYPE}")
+message(STATUS "Build type: ${CMAKE_BUILD_TYPE}")


prince-chrismc · 2021-02-06T20:06:41Z

recipes/libftdi/1.x/patches/0001-cmake-targets.patch

+ include(CMakeOptions.txt)
+
+set(as_subproject LibFTDI)
+macro(find_package)


Overriding the cmake function? 😕 Why is this needed?

This allows the library to be consumed by examples and applications in the build tree as if it were an external project, and is considered CMake best practice. See Effective CMake by Daniel Pfeiffer (CPPNow 2017) Slide 31

prince-chrismc · 2021-02-06T20:07:45Z

recipes/libftdi/1.x/patches/0001-cmake-targets.patch

+ if ( BUILD_TESTS )
+  project(libftdi1 C CXX)
+  find_package(Boost COMPONENTS unit_test_framework REQUIRED)
+  enable_testing()
+   add_subdirectory(test)
+ endif ()


CCI should not be building tests!

extra

It isn't. BUILD_TESTS is not defined by the cmake configuration in the conanfile.

prince-chrismc · 2021-02-06T20:09:06Z

recipes/libftdi/1.x/patches/0001-cmake-targets.patch

+-if(CMAKE_VERSION VERSION_LESS 2.8.8) 
+-  configure_file ( cmake/LibFTDI1Config.cmake.in ${CMAKE_CURRENT_BINARY_DIR}/LibFTDI1Config.cmake @ONLY )
+-  configure_file ( cmake/LibFTDI1ConfigVersion.cmake.in ${CMAKE_CURRENT_BINARY_DIR}/LibFTDI1ConfigVersion.cmake @ONLY )
+-else ()
+-  include(CMakePackageConfigHelpers)
+-
+-  configure_package_config_file (
+-    cmake/LibFTDI1Config.cmake.in
+-    ${CMAKE_CURRENT_BINARY_DIR}/LibFTDI1Config.cmake
+-    INSTALL_DESTINATION ${LIBFTDI_CMAKE_CONFIG_DIR}
+-    PATH_VARS
+-      LIBFTDI_USE_FILE
+-      LIBFTDI_ROOT_DIR
+-      LIBFTDI_INCLUDE_DIR
+-      LIBFTDI_INCLUDE_DIRS
+-      LIBFTDI_LIBRARY_DIRS
+-    NO_CHECK_REQUIRED_COMPONENTS_MACRO
+-  )
+-  write_basic_package_version_file (
+-    LibFTDI1ConfigVersion.cmake 
+-    VERSION ${LIBFTDI_VERSION_STRING} 
+-    COMPATIBILITY AnyNewerVersion
+-  )
+-endif ()
+include(CMakePackageConfigHelpers)
+
+configure_package_config_file (
+cmake/LibFTDI1Config.cmake.in
+${CMAKE_CURRENT_BINARY_DIR}/LibFTDI1Config.cmake
+INSTALL_DESTINATION ${LIBFTDI_CMAKE_CONFIG_DIR}
+PATH_VARS
+  LIBFTDI_USE_FILE
+  LIBFTDI_ROOT_DIR
+  LIBFTDI_INCLUDE_DIR
+  LIBFTDI_INCLUDE_DIRS
+  LIBFTDI_LIBRARY_DIRS
+NO_CHECK_REQUIRED_COMPONENTS_MACRO
+)
+write_basic_package_version_file (
+LibFTDI1ConfigVersion.cmake
+VERSION ${LIBFTDI_VERSION_STRING}
+COMPATIBILITY AnyNewerVersion
+)


This seems like an overkill. The if is always true in CCI and should not need to be changed

Is this really required?

planetmarshall · 2021-02-06T20:38:48Z

Try aim to keep the patches to a minimum. These modification make them less portable to newer versions and the recipe harder to maintain...

#3903 and the related PR have a lot of discussion on the topic

I took a quick look at the discussion but it seemed to be pretty personal and argumentative. In short though I think the patch meets danimtb's criteria in that it only modifies the build system.

I feel this patch is far larger than is required and there's a lot of non trivial changes. I stopped marking but I scanned over the file.

While a smaller patch would work for CCI, it would not be sufficient to build the tests and examples which would be necessary for the patch to be accepted upstream, which was mentioned above as a goal for using patch files.

conan-center-bot · 2021-03-07T20:06:05Z

All green in build 14 (0e4f0e5fcbedb34af238a169146be26341e0a45c):

libftdi/1.5@:
All packages built successfully! (All logs)

stale · 2021-04-06T22:17:46Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

uilianries · 2021-04-09T13:10:07Z

While a smaller patch would work for CCI, it would not be sufficient to build the tests and examples which would be necessary for the patch to be accepted upstream, which was mentioned above as a goal for using patch files

@planetmarshall Is it an official patch provided by upstream? Or did you create it and pushed to upstream? If it's official, no problem accepting. Otherwise, let's reduce it, testing and examples anre not used on CCI

planetmarshall · 2021-04-09T16:34:46Z

...let's reduce it, testing and examples anre not used on CCI

Sure, will take a look over the weekend. I've submitted the patch upstream but could be a while before seeing any feedback.

stale · 2021-05-09T21:29:31Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

jgsogo

I agree with @prince-chrismc that the patch should be simplified. Here the aim should be to modify the build-system the minimum just to make it buildable using Conan... OTH I agree it is better to merge the recipe and improve it in the following iterations.

50/50, wdyt, @prince-chrismc ?

prince-chrismc

There's too many changes in the patch which likely change the output so I do feel comfortable approving... but that's why we have lots of reviewers!

Given the age there's a few conventions that have changed too if we move this forwarded

prince-chrismc · 2021-06-22T22:34:53Z

recipes/libftdi/1.x/patches/0001-cmake-targets.patch

+    for subdivisors 0, 0.5, 0.25, 0.125
+ */
+-static int ftdi_to_clkbits(int baudrate, unsigned int clk, int clk_div, unsigned long *encoded_divisor)
+static int ftdi_to_clkbits(int baudrate, int clk, int clk_div, unsigned long *encoded_divisor)


Actually digging in the patch I found code changes which I was not expect ... this is beyond "fixing cmake"... 700+ lines of diff and lots of meaningless changes

thmahe and others added 5 commits December 19, 2020 16:19

libftdi : initial recipe

565a224

libftdi: test_package added

559e5c4

libftdi: recipes moved to 1.x sub-folder

65a1b42

To avoid conflict with libftdi 0.x releases

update boost and make requirements idiomatic

f182315

Add 1.5 and fix dependency location

8bae6b3

planetmarshall changed the title ~~Add libftdi versions 1.2-1.5. Follows from stale PR #1880~~ Add libftdi versions 1.2-1.5. Dec 19, 2020

planetmarshall mentioned this pull request Dec 19, 2020

Add libftdi recipes #1880

Closed

4 tasks

Add root cmake file

87eb2dc

madebr reviewed Dec 19, 2020

View reviewed changes

ghost mentioned this pull request Dec 19, 2020

[service] Potentially conflicting recipes #3571

Closed

This comment has been minimized.

Sign in to view

conan-center-bot added the Failed label Dec 19, 2020

planetmarshall added 2 commits December 19, 2020 19:25

remove older 1.x versions

f5ff180

use relative include paths

efab530

planetmarshall force-pushed the libftdi branch from ec5aa11 to efab530 Compare December 19, 2020 19:26

This comment has been minimized.

Sign in to view

Use C++ example and add components

8cefa3a

This comment has been minimized.

Sign in to view

planetmarshall added 2 commits December 20, 2020 17:05

modernize test package code

381cd7a

delete fpic for windows and disable macos support

007992c

This comment has been minimized.

Sign in to view

rollback test package to C++98

5c1137e

This comment has been minimized.

Sign in to view

Visual Studio builds are not supported

23fa2cc

This comment has been minimized.

Sign in to view

conan-center-bot removed the Failed label Dec 26, 2020

prince-chrismc suggested changes Dec 26, 2020

View reviewed changes

SSE4 requested a review from uilianries December 29, 2020 08:00

This comment has been minimized.

Sign in to view

update patch to link apple frameworks

0e4f0e5

conan-center-bot removed the Failed label Feb 6, 2021

prince-chrismc suggested changes Feb 6, 2021

View reviewed changes

matte86 mentioned this pull request Mar 12, 2021

[question] libftdi1 issues loading dylib conan-io/conan#8628

Closed

stale bot added the stale label Apr 6, 2021

stale bot removed the stale label Apr 9, 2021

stale bot added the stale label May 9, 2021

conan-center-bot requested a review from memsharded May 11, 2021 11:59

stale bot removed the stale label May 11, 2021

conan-center-bot requested review from jgsogo and czoido May 18, 2021 11:57

conan-center-bot requested a review from SSE4 June 3, 2021 11:59

conan-center-bot requested a review from danimtb June 18, 2021 11:59

danimtb approved these changes Jun 22, 2021

View reviewed changes

github-actions bot mentioned this pull request Jun 22, 2021

Pull Requests Ready to be Reviewed prince-chrismc/conan-center-index-pending-review#1

Open

madebr approved these changes Jun 22, 2021

View reviewed changes

jgsogo reviewed Jun 22, 2021

View reviewed changes

prince-chrismc reviewed Jun 22, 2021

View reviewed changes

SSE4 approved these changes Jun 26, 2021

View reviewed changes

conan-center-bot merged commit f7b143c into conan-io:master Jun 26, 2021

planetmarshall deleted the libftdi branch March 4, 2024 22:20

		-message("-- Build type: ${CMAKE_BUILD_TYPE}")
		+message(STATUS "Build type: ${CMAKE_BUILD_TYPE}")

Add libftdi versions 1.2-1.5. #3957

Add libftdi versions 1.2-1.5. #3957

Conversation

planetmarshall commented Dec 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Dec 19, 2020

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

prince-chrismc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madebr commented Dec 27, 2020

This comment has been minimized.

prince-chrismc commented Feb 5, 2021

conan-center-bot commented Feb 6, 2021

prince-chrismc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

planetmarshall commented Feb 6, 2021 • edited Loading

conan-center-bot commented Mar 7, 2021

stale bot commented Apr 6, 2021

uilianries commented Apr 9, 2021

planetmarshall commented Apr 9, 2021

stale bot commented May 9, 2021

jgsogo left a comment

Choose a reason for hiding this comment

prince-chrismc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

planetmarshall commented Feb 6, 2021 •

edited

Loading