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

[gdbm] Add new port, starting on version 1.24 #44186

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

noahknegt
Copy link
Contributor

This PR adds a new port for the GDBM package, I need this as a required port as avahi depends on it which I would like to add after this.

  • Changes comply with the maintainer guide.
  • The name of the port matches an existing name for this component on https://repology.org/ if possible, and/or is strongly associated with that component on search engines.
  • Optional dependencies are resolved in exactly one way. For example, if the component is built with CMake, all find_package calls are REQUIRED, are satisfied by vcpkg.json's declared dependencies, or disabled with CMAKE_DISABLE_FIND_PACKAGE_Xxx.
  • The versioning scheme in vcpkg.json matches what upstream says.
  • The license declaration in vcpkg.json matches what upstream says.
  • The installed as the "copyright" file matches what upstream says.
  • The source code of the component installed comes from an authoritative source.
  • The generated "usage text" is accurate. See adding-usage for context.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is in the new port's versions file.
  • Only one version is added to each modified port's versions file.

Signed-off-by: Noah Knegt <[email protected]>
@Mengna-Li Mengna-Li added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Mar 6, 2025
@Mengna-Li
Copy link
Contributor

@noahknegt Have you tested all the features? I encountered an error while testing the feature: (error: Autoconf version 2.71 or higher is required), but the autoconf version I can install to is 2.69.

Copy link
Contributor

@dg0yt dg0yt left a comment

Choose a reason for hiding this comment

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

Community feedback. Not tested.

list(APPEND options "--enable-libgdbm-compat=yes")
endif()

if(NOT ${ENABLE_READLINE})
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if(NOT ${ENABLE_READLINE})
if("readline" IN_LIST FEATURES)
list(APPEND options "--with-readline")
else()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Applied feedback, with some slight modifications. Readline is automatically enabled when found on the system, and thus to avoid using it implicitly the condition is reversed. Also added a comment with a link to the original docs that explains this.

list(APPEND options "--without-readline")
endif()

if(NOT ${ENABLE_MMAP})
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if(NOT ${ENABLE_MMAP})
if("memory-mapped-io" IN_LIST FEATURES)
list(APPEND options "--enable-memory-mapped-io")
else()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Applied feedback, with some slight modifications. Mmap is automatically enabled when found on the system, and thus to avoid using it implicitly the condition is reversed. Also added a comment with a link to the original docs that explains this.

This feedback includes the following:
- *FTP mirrors are added*
- *Readline is added as a dependency*
- *Archive variable is put in quotes to prevent word splitting*

Co-authored-by: @dg0yt
Signed-off-by: Noah Knegt <[email protected]>
@noahknegt
Copy link
Contributor Author

@noahknegt Have you tested all the features? I encountered an error while testing the feature: (error: Autoconf version 2.71 or higher is required), but the autoconf version I can install to is 2.69.

I have tested all the features, but done that on my WSL running ubuntu 24.04. It seems that I have autoconf version 2.71 so that why it probably worked when I was testing it. I've seen no specific requirements for a autoconf version in the readme of gdbm so I would not know why it does not work.

Is there any way the port can enforce a minimum version of autoconf through the portfile.cmake, @Mengna-Li ?

@noahknegt
Copy link
Contributor Author

Community feedback. Not tested.

Thanks @dg0yt for the feedback most of it I applied and worked, made some small changes to the suggestions as for both readline and memory-mapped-io these are enabled by default, see readme, when detected on the system. So to avoid using them implicitly when they are found I chose to disable them when the feature is not present.

@dg0yt
Copy link
Contributor

dg0yt commented Mar 6, 2025

"Detected when present" may need explicit control also to ensure failure when the feature is requested from the port but configuration fails to detect it.

Is there any way the port can enforce a minimum version of autoconf through the portfile.cmake

AFAICT no such check is present in any other port. The log should be clear enough in case of error.

Ubuntu 20.04 comes with autoconf 2.69. I wonder why @Mengna-Li even tests with this old version. And it is possible to use a local installation of a newer version.

@noahknegt
Copy link
Contributor Author

"Detected when present" may need explicit control also to ensure failure when the feature is requested from the port but configuration fails to detect it.

How would you do this properly, not that familiar with creating vcpkg ports yet nor with autoconf

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:new-port The issue is requesting a new library to be added; consider making a PR!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants