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

expect: Fix install name for libexpect dylib #25920

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

saagarjha
Copy link
Contributor

Description

This fixes a bug introduced in #25873 and reported here: https://lists.macports.org/pipermail/macports-users/2024-September/053033.html

Type(s)
  • bugfix
  • enhancement
  • security fix
Tested on

macOS 15.1 24B5046f
Xcode 16.1 16B5014f

Verification

Have you

  • followed our Commit Message Guidelines?
  • squashed and minimized your commits?
  • checked that there aren't other open pull requests for the same change?
  • checked your Portfile with port lint --nitpick?
  • tried existing tests with sudo port test?
  • tried a full install with sudo port -vst install?
  • tested basic functionality of all binary files?
  • checked that the Portfile's most important variants haven't been broken?

@pietvo
Copy link
Contributor

pietvo commented Sep 26, 2024

This would also need a revision bump.

@saagarjha
Copy link
Contributor Author

Bumped.

saagarjha referenced this pull request Sep 26, 2024
Also remove the platforms line (it's the default and port lint warns
about it) and --disable-shared from configure.args (it breaks tests.)
system "install_name_tool -id ${prefix}/lib/libexpect.dylib ${destroot}${prefix}/lib/libexpect.dylib"
# The expect binary has already been linked against the old install
# name, so we need to fix that too.
system "install_name_tool -change libexpect${version}.dylib ${prefix}/lib/libexpect.dylib ${destroot}${prefix}/bin/expect"
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of running install_name_tool afterward to fix the wrong install name, can the correct install name be set to begin with?

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 tried to do this but wasn't able to figure it out. How does one typically do this for an arbitrary port? expect doesn't seem to know about install names at all, and it uses the same flags for compiling the library as it does the actual code:

# TEA specific: use LDFLAGS_DEFAULT instead of LDFLAGS
SHLIB_LD='${CC} -dynamiclib ${CFLAGS} ${LDFLAGS_DEFAULT}'
AC_CACHE_CHECK([if ld accepts -single_module flag], tcl_cv_ld_single_module, [
hold_ldflags=$LDFLAGS
LDFLAGS="$LDFLAGS -dynamiclib -Wl,-single_module"
AC_TRY_LINK(, [int i;], tcl_cv_ld_single_module=yes, tcl_cv_ld_single_module=no)
LDFLAGS=$hold_ldflags])
AS_IF([test $tcl_cv_ld_single_module = yes], [
SHLIB_LD="${SHLIB_LD} -Wl,-single_module"
])
# TEA specific: link shlib with current and compatiblity version flags
vers=`echo ${PACKAGE_VERSION} | sed -e 's/^\([[0-9]]\{1,5\}\)\(\(\.[[0-9]]\{1,3\}\)\{0,2\}\).*$/\1\2/p' -e d`
SHLIB_LD="${SHLIB_LD} -current_version ${vers:-0} -compatibility_version ${vers:-0}"
SHLIB_SUFFIX=".dylib"
# Don't use -prebind when building for Mac OS X 10.4 or later only:
AS_IF([test "`echo "${MACOSX_DEPLOYMENT_TARGET}" | awk -F '10\\.' '{print int([$]2)}'`" -lt 4 -a \
"`echo "${CPPFLAGS}" | awk -F '-mmacosx-version-min=10\\.' '{print int([$]2)}'`" -lt 4], [
LDFLAGS="$LDFLAGS -prebind"])
LDFLAGS="$LDFLAGS -headerpad_max_install_names"
AC_CACHE_CHECK([if ld accepts -search_paths_first flag],
    tcl_cv_ld_search_paths_first, [
hold_ldflags=$LDFLAGS
LDFLAGS="$LDFLAGS -Wl,-search_paths_first"
AC_TRY_LINK(, [int i;], tcl_cv_ld_search_paths_first=yes,
	tcl_cv_ld_search_paths_first=no)
LDFLAGS=$hold_ldflags])
AS_IF([test $tcl_cv_ld_search_paths_first = yes], [
LDFLAGS="$LDFLAGS -Wl,-search_paths_first"
])

Should we patch this file instead?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, they're using TEA. That explains a lot and makes sense given the nature of expect. Unfortunately it's aimed at making extensions that Tcl can load, not libraries intended to be linked against by other programs, so some patching is probably unavoidable if we want it to make the latter correctly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants