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

[many ports] Apply host dependencies #16479

Merged
merged 7 commits into from
Mar 26, 2021

Conversation

ras0219
Copy link
Contributor

@ras0219 ras0219 commented Mar 1, 2021

Reopening of #15424.

This PR solves a long-standing issue with manifests being unable to correctly install boost while cross-compiling. This is done by enabling consumers to mark a dependency as coming from the host (via "host": true). The dependency can then be accessed in the portfile via ${CURRENT_HOST_INSTALLED_DIR}/subpath.

This PR does not automatically add any subpaths of the host installed dir to helpers such as vcpkg_configure_cmake(). For now, consumers are intended to use vcpkg_add_to_path() to pick individual required executable folders:

vcpkg_add_to_path(PREPEND "${CURRENT_HOST_INSTALLED_DIR}/tools/grpc")

See docs/users/tools.md for more details.

Copy link
Contributor

@strega-nil strega-nil left a comment

Choose a reason for hiding this comment

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

just some minor notes, not fully reviewed

Comment on lines 52 to 53
string(COMPARE EQUAL "${TARGET_TRIPLET}" "${HOST_TRIPLET}" I_AM_NOT_CROSSCOMPILING)

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
string(COMPARE EQUAL "${TARGET_TRIPLET}" "${HOST_TRIPLET}" I_AM_NOT_CROSSCOMPILING)

no reason to have this, imo

Copy link
Contributor

Choose a reason for hiding this comment

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

Not useful for users, but imho useful for maintainers. If you are writing a port that depends on itself with the host flag, you have to make distinctions if you are cross compiling or not. But an example like

if(NOT "${TARGET_TRIPLET}" STREQUAL "${HOST_TRIPLET}")
  # specific cross compile code
endif()

would be more helpful than the string example.

Copy link
Contributor

Choose a reason for hiding this comment

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

nvm this is the next line.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it a bug? But for me there is no variable ${HOST_TRIPLET} or the value is the empty string?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it a bug? But for me there is no variable ${HOST_TRIPLET} or the value is the empty string?

nwm. set(HOST_TRIPLET "${_HOST_TRIPLET}") gets introduced with this PR. I thought it is already there...

Copy link
Contributor

Choose a reason for hiding this comment

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

better define VCPKG_(IS_)CROSSCOMPILING

ports/yasm-tool-helper/portfile.cmake Outdated Show resolved Hide resolved
@strega-nil
Copy link
Contributor

Is there a reason that this is a draft?

@ras0219
Copy link
Contributor Author

ras0219 commented Mar 1, 2021

I wanted to ensure it builds cleanly before requesting review, since build breaks will likely mean changing the approach in one or more ports

@Neumann-A
Copy link
Contributor

all ports using vcpkg_configure_qmake need an additional host dependency on qt5-base

@ras0219
Copy link
Contributor Author

ras0219 commented Mar 1, 2021

With this PR I'm specifically targeting this list:

# WORKAROUND: the x86-windows flavors of these are needed for all cross-compilation, but they are not auto-installed.
# Install them so the CI succeeds:
if ($Triplet -in @('x64-uwp', 'arm64-windows', 'arm-uwp')) {
.\vcpkg.exe install protobuf:x86-windows boost-build:x86-windows sqlite3:x86-windows yasm-tool:x86-windows ampl-mp:x86-windows @commonArgs
} elseif ($Triplet -in @('x64-windows', 'x64-windows-static', 'x64-windows-static-md')) {
.\vcpkg.exe install yasm-tool:x86-windows @commonArgs
}

Copy link
Contributor

@autoantwort autoantwort left a comment

Choose a reason for hiding this comment

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

For the port boost-context and grpc the port version field was not updated

ports/boost-concept-check/vcpkg.json Show resolved Hide resolved
@PhoebeHui PhoebeHui added info:internal This PR or Issue was filed by the vcpkg team. category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist labels Mar 2, 2021
@ras0219 ras0219 force-pushed the dev/roschuma/tool-ports-2 branch 2 times, most recently from 9d51fb4 to 51bafe6 Compare March 3, 2021 22:44
@Neumann-A
Copy link
Contributor

another todo: all ADD_BIN_TO_PATH options vcpkg_(install|build)_* need to be disabled if target!=host
@ras0219-msft: I hope you keep a list with those ToDo's :)

@Neumann-A
Copy link
Contributor

Hmm could it be that CURRENT_HOST_INSTALLED_DIR is not yet in vcpkg master?

@autoantwort
Copy link
Contributor

Hmm could it be that CURRENT_HOST_INSTALLED_DIR is not yet in vcpkg master?

Yes it gets introduced with this PR. See scripts/ports.cmake

@Neumann-A
Copy link
Contributor

hmm probably would make sense to split this PR up in two.
a) only provide the new/extra definitions
b) fix all the ports

a) could probably be merged way faster than b)

@ras0219
Copy link
Contributor Author

ras0219 commented Mar 5, 2021

The problem is really the changes to core helpers that provoke full rebuilds if any other core changes were merged; the handful of changed ports are insignificant here

strega-nil added a commit to strega-nil/vcpkg that referenced this pull request Mar 9, 2021
This contains all the docs and scripts changes from microsoft#16479,
without any of the ports changes, for easier CR
strega-nil added a commit that referenced this pull request Mar 10, 2021
This contains all the docs and scripts changes from #16479,
without any of the ports changes, for easier CR
@soumyamahunt
Copy link
Contributor

soumyamahunt commented Mar 12, 2021

This PR assumes there will be only one host triplet needed, what if multiple tools need be installed with multiple host triplet? For example, what if user wants to build contoso-code-generator for x86-windows triplet and contoso-build-system for x64-windows? Although I can't think of any port that requires such configuration, I think this could cause issues in future and would be nice to see this scenario to be addressed by this PR. My proposed solution would be to allow overriding host triplet per dependency, something like this:

{
    "name": "contoso-http-library",
    "version-string": "1.0.0",
    "description": "Contoso's http runtime library",
    "dependencies": [
        "contoso-core-library",
        {
            "name": "contoso-code-generator",
            "host": true,
            "host-triplet": "x86-windows"
        },
        {
            "name": "contoso-build-system",
            "host": true,
            "host-triplet": "x64-windows"
        }
    ]
}

Basically host-triplet combined with platform field can be used to handle any scenario for building dependencies with manifest.

@Neumann-A
Copy link
Contributor

@soumyamahunt that was already discussed in #15424

@JackBoosY JackBoosY linked an issue Mar 24, 2021 that may be closed by this pull request
@JackBoosY
Copy link
Contributor

We also need to modify libvpx dependency yasm-tool.

@ras0219-msft ras0219-msft marked this pull request as ready for review March 26, 2021 17:26
@ras0219-msft
Copy link
Contributor

another todo: all ADD_BIN_TO_PATH options vcpkg_(install|build)_* need to be disabled if target!=host

While using that is suspect, it would certainly be a breaking change to do that now. We may wish to add a warning in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist 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.

[protobuf] build failure Error: Building package gmp:x64-windows failed with: Build_failed
9 participants