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

[vcpkg-make] Add new port #39050

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

Conversation

JavierMatosD
Copy link
Contributor

@JavierMatosD JavierMatosD commented May 30, 2024

Testing to see what happens when various options are on/off. Added a test port and cleaned up some of the code.

@JavierMatosD
Copy link
Contributor Author

I would like to repeat my concerns regarding parameters taking boolean values instead of choosing zero-parameter keywords without arguments.

@dg0yt, mind offering an example? I see a lot of places where we are passing boolean-like options such as DISABLE_CPP_FLAGS, NO_FLAG_ESCAPING, and DISABLE_MSVC_WRAPPERS. Are you suggesting these turn into keyword-like options like USE_CPP_FLAGS?

I took your previous comment to mean "don't compare cmake variables to any particular literal"

Comment on lines +77 to +89
message(FATAL_ERROR "${PORT} currently requires the following programs from the system package manager:
autoconf automake autoconf-archive
On Debian and Ubuntu derivatives:
sudo apt-get install autoconf automake autoconf-archive
On recent Red Hat and Fedora derivatives:
sudo dnf install autoconf automake autoconf-archive
On Arch Linux and derivatives:
sudo pacman -S autoconf automake autoconf-archive
On Alpine:
apk add autoconf automake autoconf-archive
On macOS:
brew install autoconf automake autoconf-archive\n")
endif()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@BillyONeal, are we ok with this messaging?

@dg0yt
Copy link
Contributor

dg0yt commented Oct 28, 2024

I took your previous comment to mean "don't compare cmake variables to any particular literal"

This was the must-have. And there still is:
https://github.com/microsoft/vcpkg/pull/39050/files#diff-ec85869615f18a9570937cbc887348e720f2c195157eefe38f9dad3660913a58R443

But this should not be needed at all with a zero-option keyword.

I would like to repeat my concerns regarding parameters taking boolean values instead of choosing zero-parameter keywords without arguments.

@dg0yt, mind offering an example? I see a lot of places where we are passing boolean-like options such as DISABLE_CPP_FLAGS, NO_FLAG_ESCAPING, and DISABLE_MSVC_WRAPPERS. Are you suggesting these turn into keyword-like options like USE_CPP_FLAGS?

Option Owner Type ❤️
DISABLE_CPP_FLAGS n/a n/a (using git grep DISABLE_CPP_FLAGS) n/a
DISABLE_CPPFLAGS vcpkg-make zero-value ✔️
NO_FLAG_ESCAPING vcpkg-make zero-value ✔️
DISABLE_MSVC_WRAPPERS vcpkg-make zero-value ✔️
VCPKG_TRANSFORM_LIBS vcpkg_make one-value (requiring literal "ON") 💥

VCPKG_TRANSFORM_LIBS is the only exception in the list, in vcpkg_make_common.cmake.

I see how this is initialized in the calling spots. But I am not convinced that this is a sufficient justification.

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.

Zero-value VCPKG_TRANSFORM_LIBS option.

ports/vcpkg-make/vcpkg_make_common.cmake Outdated Show resolved Hide resolved
ports/vcpkg-make/vcpkg_make_common.cmake Outdated Show resolved Hide resolved
ports/vcpkg-make/vcpkg_make_common.cmake Outdated Show resolved Hide resolved
ports/vcpkg-make/vcpkg_make_common.cmake Outdated Show resolved Hide resolved
ports/vcpkg-make/vcpkg_make_common.cmake Outdated Show resolved Hide resolved
@JavierMatosD
Copy link
Contributor Author

Hmmm, I wasn't able to reproduce the windows failures locally...

@dg0yt
Copy link
Contributor

dg0yt commented Nov 5, 2024

Hmmm, I wasn't able to reproduce the windows failures locally...

You probably tested the head of your branch (before the msys2 update),
not the merge commit in CI (after the msys2 update, before the gmp fix).
Just merge sync the branch again, so that CI runs a new merge commit.

Comment on lines 62 to +71
if(VCPKG_HOST_IS_WINDOWS)
# dumpbin detection fails with autoconf 2.72
set(ENV{WANT_AUTOCONF} 2.71)
endif()
vcpkg_configure_make(
SOURCE_PATH "${SOURCE_PATH}"
AUTOCONFIG
ADDITIONAL_MSYS_PACKAGES
vcpkg_acquire_msys(MSYS_ROOT
PACKAGES
autoconf2.71
DIRECT_PACKAGES
"https://mirror.msys2.org/msys/x86_64/autoconf2.71-2.71-3-any.pkg.tar.zst"
dd312c428b2e19afd00899eb53ea4255794dea4c19d1d6dea2419cb6a54209ea2130d48abbc20af12196b9f628143436f736fbf889809c2c2291be0c69c0e306
"dd312c428b2e19afd00899eb53ea4255794dea4c19d1d6dea2419cb6a54209ea2130d48abbc20af12196b9f628143436f736fbf889809c2c2291be0c69c0e306"
)
endif()
Copy link
Contributor Author

@JavierMatosD JavierMatosD Nov 5, 2024

Choose a reason for hiding this comment

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

I doubt this will work like this. dumpbin detection is still failing.

./libtool: eval: line 10827: syntax error near unexpected token `|'
./libtool: eval: line 10827: `dumpbin.exe -symbols -headers .libs/libgmp.la-2.obj  |  | /usr/bin/sed -e '/^[BCDGRS][ ]/s/.*[ ]\([^ ]*\)/\1,DATA/' | /usr/bin/sed -e '/^[AITW][ ]/s/.*[ ]//' | sort | uniq > .libs/gmp.exp'
make[2]: *** [Makefile:890: libgmp.la] Error 2
make[1]: *** [Makefile:1005: all-recursive] Error 1
make: *** [Makefile:797: all] Error 2

Copy link
Contributor

Choose a reason for hiding this comment

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

This is another issue, cf. #41510.
With 2.72, the error was during configure.
This error is during make.

Copy link
Contributor

Choose a reason for hiding this comment

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

Between | |, there should be the expansion of $global_symbol_pipe.

Copy link
Contributor

Choose a reason for hiding this comment

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

dumpbin detection is still failing.

Indeed:

checking for BSD- or MS-compatible name lister (nm)... dumpbin.exe -symbols -headers
checking the name lister (dumpbin.exe -symbols -headers) interface... BSD nm

Copy link
Contributor

@dg0yt dg0yt Nov 6, 2024

Choose a reason for hiding this comment

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

configure has (with another autoconf version):

  if $GREP 'External.*some_variable' conftest.out > /dev/null; then
    lt_cv_nm_interface="MS dumpbin"
  fi

GREP is (in current config.log):

GREP='/usr/bin/grep'

conftest.out has (in current config.log):

009 00000000 SECT3  notype       External     | some_variable

The question is: Why doesn't it reach lt_cv_nm_interface="MS dumpbin"?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, wait, config.log says:

It was created by GNU MP configure 6.3.0, which was
generated by GNU Autoconf 2.72.  Invocation command line was

This is the incompatible version.


if(VCPKG_CROSSCOMPILING)
set(ENV{HOST_TOOLS_PREFIX} "${CURRENT_HOST_INSTALLED_DIR}/manual-tools/${PORT}")
endif()

if(VCPKG_HOST_IS_WINDOWS)
# dumpbin detection fails with autoconf 2.72
Copy link
Contributor

Choose a reason for hiding this comment

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

Please leave the comment.

SOURCE_PATH "${SOURCE_PATH}"
AUTOCONFIG
ADDITIONAL_MSYS_PACKAGES
vcpkg_acquire_msys(MSYS_ROOT
Copy link
Contributor

Choose a reason for hiding this comment

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

How is this passed to / used in vcpkg_make_configure?

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! category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly info:internal This PR or Issue was filed by the vcpkg team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants