Skip to content

[vcpkg] Always use an autotools build triplet#18130

Merged
BillyONeal merged 4 commits intomicrosoft:masterfrom
dg0yt:mingw-triplet
Nov 4, 2021
Merged

[vcpkg] Always use an autotools build triplet#18130
BillyONeal merged 4 commits intomicrosoft:masterfrom
dg0yt:mingw-triplet

Conversation

@dg0yt
Copy link
Contributor

@dg0yt dg0yt commented May 26, 2021

  • What does your PR fix?

    This PR ensures that vcpkg_configure_make always determines autotools triplet when building for mingw. This fixes the case when both vcppkg target triplet and vcpkg host triplet are set to the same mingw triplet, i.e. building on a Windows PC without MSVC.

  • Which triplets are supported/not supported? Have you updated the CI baseline?

    all (but targeting mingw), no

  • Does your PR follow the maintainer guide?

    yes

  • If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

    -/-

PS: I didn't want to add yet another expression to the if clause following that change.

@Neumann-A
Copy link
Contributor

Hmm why? shouldn't _csc_AUTOCONFIG not decide here? Which port is affected by this change?

@NancyLi1013 NancyLi1013 added the category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly label May 27, 2021
@dg0yt
Copy link
Contributor Author

dg0yt commented May 27, 2021

Which port is affected by this change?

It happened a few days ago on a different computer. Let's try here anyway:

$ grep -l vcpkg_configure_make ports/*/portfile.cmake | while read I; do grep "AUTOCONFIG" || echo $I; done
ports/apr/portfile.cmake

Now I remember: Port apr normally doesn't use vcpkg_configure_make on Windows, but it was one of the ports I exerimented with when I investigated @Neumann-A's question about the linkage flags for uuid.

shouldn't _csc_AUTOCONFIG not decide here?

If it is present, yes. But the documenation doesn't give me the impression that this option is mandatory when building a project based on autotools, or that it is related to triplets:

AUTOCONFIG

Need to use autoconfig to generate configure file.

Perhaps DETERMINE_BUILD_TRIPLET documentation might be interpreted as making that option mandatory:

DETERMINE_BUILD_TRIPLET

For ports having a configure script following the autotools rules for selecting the triplet

But it isn't use very often:

$ grep -l vcpkg_configure_make ports/*/portfile.cmake | while read I; do grep -q "DETERMINE_BUILD_TRIPLET" $I && echo $I ; done
ports/gettext/portfile.cmake
ports/libiconv/portfile.cmake
ports/libpq/portfile.cmake
ports/mp3lame/portfile.cmake

Last not least the if statement following the change deals not only with the AUTOCONFIG and DETERMINE_BUILD_TRIPLET options but also with VCPKG_CROSSCOMPILING. However, mingw-target with mingw-host does not activate VCPKG_CROSSCOMPILING.

@longnguyen2004
Copy link
Contributor

IIRC, AUTOCONFIG is used for libraries that don't come with a pre-generated configure script, so it's not related to triplets.

@Neumann-A
Copy link
Contributor

IIRC, AUTOCONFIG is used for libraries that don't come with a pre-generated configure script, so it's not related to triplets.

it is also required if the configure script was generated with an old version of autotools/libtool

@NancyLi1013 NancyLi1013 changed the title Always use an autotools build triplet for mingw [vcpkg] Always use an autotools build triplet for mingw May 31, 2021
@strega-nil-ms strega-nil-ms added requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. and removed requires:discussion labels Jun 7, 2021
@ras0219-msft
Copy link
Contributor

This does smell very weird; I don't see why MinGW should be special here.

@ras0219-msft ras0219-msft removed the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Jun 17, 2021
@dg0yt
Copy link
Contributor Author

dg0yt commented Jun 18, 2021

I don't see why MinGW should be special here.

So it is up to community support to add AUTOCONFIG (for its side effect) or DETERMINE_BUILD_TRIPLET (for its regular effect) for all ports that don't build with mingw on Windows (i.e. are not cross-compiling)?

@dg0yt
Copy link
Contributor Author

dg0yt commented Jun 28, 2021

This does smell very weird; I don't see why MinGW should be special here.

For configure in WIN32 MinGW must be handled as a crossbuild even if host triplet == target triplet.

@ras0219-msft
Copy link
Contributor

... MinGW must be handled as a crossbuild even if host triplet == target triplet.

Mechanically, why is this? And then, why should we not always cross build?

@dg0yt
Copy link
Contributor Author

dg0yt commented Jun 29, 2021

... MinGW must be handled as a crossbuild even if host triplet == target triplet.

Mechanically, why is this? And then, why should we not always cross build?

You left out the context before my statement:

For configure in WIN32

When vcpkg_configure_make is run on Windows, with a mingw target triplet and mingw host triplet, it must pass triplets to configure as it does in cross compiling, or the build will fail due to assumptions about non-mingw tools being available.

This PR is the result of building with mingw on Windows.

@dg0yt
Copy link
Contributor Author

dg0yt commented Jun 29, 2021

(Wrong button.)

The alternative is to always set the proper mingw --host=<arch>-w64-minw32 when VCPKG_TARGET_IS_MINGW.

@dg0yt
Copy link
Contributor Author

dg0yt commented Jul 23, 2021

Any other comments how to generally ensure proper triplet when building mingw on windows?

@NancyLi1013
Copy link
Contributor

@vicroms

Could you please help take a look?

@dg0yt
Copy link
Contributor Author

dg0yt commented Aug 18, 2021

Ping.

@NancyLi1013
Copy link
Contributor

Any other comments how to generally ensure proper triplet when building mingw on windows?

@ras0219-msft Could you please help leave some comments for this?

@strega-nil-ms
Copy link
Contributor

Given that we don't have any experts on mingw on our team, and the expert on mingw says this is important, could we merge as "strictly better and also on a community triplet"? cc @ras0219, opinions?

@BillyONeal BillyONeal added requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. and removed requires:discussion labels Sep 14, 2021
@BillyONeal BillyONeal removed the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Sep 23, 2021
@BillyONeal
Copy link
Member

BillyONeal commented Sep 23, 2021

It seems like the correct fix for this is to yank out a vcpkg_autoconf_configure() and friends in a vcpkg-autoconf port which always does the DETERMINE_BUILD_TRIPLET behavior, and transition autoconf based ports to use that rather than patching vcpkg_configure_make.

(Unfortunately, vcpkg_configure_make doesn't really make up its mind as to whether it is or is not targeting autoconf; assuming things that are not autoconf but still name themselves configure will be happy to get extra --build, --host, and --target switches is probably not safe ... and yes we are already bad about that when VCPKG_CROSSCOMPILING)

@ras0219-msft
Copy link
Contributor

I observe that in the conditional

if (_csc_AUTOCONFIG AND NOT _csc_BUILD_TRIPLET OR _csc_DETERMINE_BUILD_TRIPLET OR VCPKG_CROSSCOMPILING AND NOT _csc_BUILD_TRIPLET)

the variable VCPKG_CROSSCOMPILING is already being set for all our x86-windows and x64-windows-static customers (that leave the default x64-windows host triplet). This gives a strong indication that it should be safe to always engage this block.

Perhaps we should try that approach as a more minimal change that doesn't introduce further diversification?

@Neumann-A
Copy link
Contributor

When vcpkg_configure_make is run on Windows, with a mingw target triplet and mingw host triplet, it must pass triplets to configure as it does in cross compiling, or the build will fail due to assumptions about non-mingw tools being available.

Why is that? is the environment not correctly setup? Or is it choosing the MSYS compilers?

Also there is the undocumented triplet variable VCPKG_MAKE_BUILD_TRIPLET to force a certain triplet if needed.

@dg0yt
Copy link
Contributor Author

dg0yt commented Sep 23, 2021

This is a fairly simple change from May. It makes ports pass a crucial setting to configure on native mingw builds, instead of delegating to configure's detection mechanisms.

Why is that? is the environment not correctly setup?

Who knows? Perhaps some ports didn't properly read the environment?

If you don't want to make this a general behaviour, we will have to make individual changes to all ports which fail due to missing AUTOCONFIG.

For more fundamental long-term changes, coordinate with #19753 or #20165 audits by @JackBoosY. Possibly including the flip from opt-in AUTOCONFIG to opt-out SKIP_AUTOCONFIG. (I would have more ideas but don't want to go OT.)

Copy link
Contributor

@ras0219-msft ras0219-msft left a comment

Choose a reason for hiding this comment

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

I've pushed the simplification I suggested above -- LGTM if it passes :)

Thanks!

@BillyONeal
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@BillyONeal
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@dg0yt dg0yt changed the title [vcpkg] Always use an autotools build triplet for mingw [vcpkg] Always use an autotools build triplet Oct 15, 2021
@JackBoosY JackBoosY added info:world-rebuild info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. and removed requires:discussion labels Nov 2, 2021
@BillyONeal BillyONeal merged commit fdb5932 into microsoft:master Nov 4, 2021
@BillyONeal
Copy link
Member

Thanks and sorry it took a while to review!

@dg0yt dg0yt deleted the mingw-triplet branch November 19, 2021 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants