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

ncurses: migrate to Conan v2 #20355

Merged
merged 33 commits into from
Dec 2, 2023
Merged

Conversation

valgur
Copy link
Contributor

@valgur valgur commented Oct 4, 2023

Some notable changes:

  • Moved the definition of the compatibility CMake module to an external file.
  • Condensed the component definitions in package_info().
  • Added an unofficial Curses::Curses CMake target. CMake currently does not set one (https://gitlab.kitware.com/cmake/cmake/-/issues/23051), but I think having it is helpful and much less error-prone than using package variables.

These can probably be closed if this PR gets merged:

@ghost
Copy link

ghost commented Oct 4, 2023

I detected other pull requests that are modifying ncurses/all recipe:

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

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@github-actions
Copy link
Contributor

Hooks produced the following warnings for commit e08332d
ncurses/6.3@#9c8ff067110359ed333d2451b482ab65
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library './lib/libncurses++w.so' links to system library 'util' but it is not in cpp_info.system_libs.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

Comment on lines 26 to 36
"with_ticlib": ["auto", True, False],
"with_ticlib": [True, False],
"with_reentrant": [True, False],
"with_tinfo": ["auto", True, False],
"with_tinfo": [True, False],
Copy link
Member

Choose a reason for hiding this comment

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

Please, inform how do you change these values in term of always True, but considering that the system will actually do a check to enable/disable first.

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 restored the "auto" default value for clarity and backwards compatibility.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, I checked https://cmake.org/cmake/help/latest/module/FindCurses.html and this file follows the variables there.

@RubenRBS
Copy link
Member

RubenRBS commented Nov 28, 2023

I've tested locally with these combinations which seem to work:

9   conan create . --version=6.3 -o "&:with_pcre2=True" --build=missing -c:h "tools.files.download:verify=False" -c:b "tools.files.download:verify=False"
10  conan create . --version=6.3 -o "&:with_pcre2=True" --build=missing -c:h "tools.files.download:verify=False" -c:b "tools.files.download:verify=False" -o "*/*:shared=True"
11  conan create . --version=6.3 -o "&:with_pcre2=True" --build=missing -c:h "tools.files.download:verify=False" -c:b "tools.files.download:verify=False" -o "*/*:shared=True" -o "&:with_reentrant=True"
12  conan create . --version=6.3 -o "&:with_pcre2=True" --build=missing -c:h "tools.files.download:verify=False" -c:b "tools.files.download:verify=False" -o "*/*:shared=True" -o "&:with_reentrant=True" -o "&:with_progs=False"

Copy link
Member

@RubenRBS RubenRBS left a comment

Choose a reason for hiding this comment

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

Question about the handling of with_ticlib, looks good otherwise :)


def config_options(self):
if self.settings.os == "Windows":
del self.options.fPIC
# Set the default value based on OS
self.options.with_ticlib = self.settings.os != "Windows"
Copy link
Member

Choose a reason for hiding this comment

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

This is overriding whatever the user is setting these options as even if they are not set to auto, right?

Copy link
Member

Choose a reason for hiding this comment

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

I see #20355 (comment) but not clear to me what the correct way would be

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This only overwrites the default "auto" value with a correct OS-specific default value. User-specified values for these options are not affected.

Believe me, this is approach is positively fantastic and I'm glad it works. Any alternatives approaches would need an insane amount of hacking around self.options and self.info.options being expected in different methods.

@uilianries
Copy link
Member

@valgur Could you please add test_v1_package too. It's missing and this recipe is not a new one.

@valgur
Copy link
Contributor Author

valgur commented Nov 30, 2023

@valgur Could you please add test_v1_package too. It's missing and this recipe is not a new one.

@uilianries test_v1_package turned out to be too much of a headache for the included .cmake module with different naming conventions and what not used in the old generators. It will probably fail when using the custom CMake variables as is done in the test_package CMakeLists.txt, but would work just fine with a normal CMake target. Not worth the extra effort, IMO.

@uilianries
Copy link
Member

@valgur Could you please add test_v1_package too. It's missing and this recipe is not a new one.

@uilianries test_v1_package turned out to be too much of a headache for the included .cmake module with different naming conventions and what not used in the old generators. It will probably fail when using the custom CMake variables as is done in the test_package CMakeLists.txt, but would work just fine with a normal CMake target. Not worth the extra effort, IMO.

@valgur no problem! Thank you for trying, let's keep only test_package then.

uilianries
uilianries previously approved these changes Nov 30, 2023
Copy link
Member

@uilianries uilianries left a comment

Choose a reason for hiding this comment

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

LGTM

@conan-center-bot
Copy link
Collaborator

Conan v1 pipeline ✔️

All green in build 38 (17cf0eba5ef1325c380084cf7f9a87ac00eec9ad):

  • ncurses/6.3:
    All packages built successfully! (All logs)

  • ncurses/6.4:
    All packages built successfully! (All logs)

  • ncurses/6.2:
    All packages built successfully! (All logs)


Conan v2 pipeline ✔️

Note: Conan v2 builds are now mandatory. Please read our discussion about it.

All green in build 38 (17cf0eba5ef1325c380084cf7f9a87ac00eec9ad):

  • ncurses/6.4:
    All packages built successfully! (All logs)

  • ncurses/6.3:
    All packages built successfully! (All logs)

  • ncurses/6.2:
    All packages built successfully! (All logs)

@conan-center-bot conan-center-bot merged commit e149e57 into conan-io:master Dec 2, 2023
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants