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

Add VCPKG_(PRE|POST)_PORTFILE_INCLUDES for code injection before/after the portfile #25847

Merged
merged 2 commits into from
Jul 25, 2024

Conversation

Neumann-A
Copy link
Contributor

@Neumann-A Neumann-A commented Jul 18, 2022

This is basically needed for reproduceable builds. This allows injection of code which changes the created binaries to not have timestamps etc or implement further user controlled checks and policies.

The pre part is needed if the triplet ever gets changed to something easier to parse which is not cmake ;)

probably needs changes to vcpkg-tool to hash these additional files.

docs pr: microsoft/vcpkg-docs#336
tool pr: microsoft/vcpkg-tool#1417

  • change variable name to Z_VCPKG_POST_PORTFILE_INCLUDES in ports.cmake after tool has been updated.

github-actions[bot]
github-actions bot previously approved these changes Jul 18, 2022
@JackBoosY JackBoosY self-assigned this Jul 19, 2022
@JackBoosY JackBoosY added the category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed label Jul 19, 2022
@JackBoosY JackBoosY assigned Cheney-W and unassigned JackBoosY Oct 14, 2022
@Osyotr
Copy link
Contributor

Osyotr commented May 26, 2024

Now that we have VCPKG_HASH_ADDITIONAL_FILES, can it be properly reviewed?

@Neumann-A Neumann-A marked this pull request as ready for review May 29, 2024 20:03
@Cheney-W Cheney-W added the category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly label May 31, 2024
@Cheney-W Cheney-W added the info:reviewed Pull Request changes follow basic guidelines label May 31, 2024
@vicroms vicroms added the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Jun 3, 2024
Copy link
Member

@vicroms vicroms left a comment

Choose a reason for hiding this comment

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

@BillyONeal
@data-queue
@JavierMatosD
@ras0219-msft
@vicroms

We discussed this PR during our meeting and have the following comments to make:

  • Post-portfile injection is interesting for the example use case you mentioned (reproducible builds).

  • Pre-portfile injection we don't see a motivation right now. Do you have a definitive use case in mind? Otherwise, we would like to wait until we have a use case for it.

  • We noticed that pre-injection runs after port configs are included. So it might be used to override helper functions (but that can be done already with port overlays).

  • All files included in VCPKG_POST_PORTFILE_INCLUDES should be "laundered" and hashed through the tool. The vcpkg tool should read the variable to validate the files, hash them, and then reinject them into the process (maybe using a Z_VCPKG_UGLY_NAME variable) so that the specified files are not changed during the build (for example, by portfile.cmake).

  • This needs the usual documentation and tests.

@vicroms vicroms marked this pull request as draft June 7, 2024 08:48
@Neumann-A
Copy link
Contributor Author

Pre-portfile injection we don't see a motivation right now. Do you have a definitive use case in mind? Otherwise, we would like to wait until we have a use case for it.

Installing https://cmake.org/cmake/help/latest/command/variable_watch.html
or overriding any of

vcpkg/scripts/ports.cmake

Lines 24 to 125 in 14b9179

set(SCRIPTS "${CMAKE_CURRENT_LIST_DIR}" CACHE PATH "Location to stored scripts")
list(APPEND CMAKE_MODULE_PATH "${SCRIPTS}/cmake")
# Increment this number if we intentionally need to invalidate all binary caches due a change in
# the following scripts: 1
include("${SCRIPTS}/cmake/execute_process.cmake")
include("${SCRIPTS}/cmake/vcpkg_acquire_msys.cmake")
include("${SCRIPTS}/cmake/vcpkg_add_to_path.cmake")
include("${SCRIPTS}/cmake/vcpkg_apply_patches.cmake")
include("${SCRIPTS}/cmake/vcpkg_backup_restore_env_vars.cmake")
include("${SCRIPTS}/cmake/vcpkg_build_cmake.cmake")
include("${SCRIPTS}/cmake/vcpkg_build_make.cmake")
include("${SCRIPTS}/cmake/vcpkg_build_msbuild.cmake")
include("${SCRIPTS}/cmake/vcpkg_build_ninja.cmake")
include("${SCRIPTS}/cmake/vcpkg_build_nmake.cmake")
include("${SCRIPTS}/cmake/vcpkg_build_qmake.cmake")
include("${SCRIPTS}/cmake/vcpkg_buildpath_length_warning.cmake")
include("${SCRIPTS}/cmake/vcpkg_check_features.cmake")
include("${SCRIPTS}/cmake/vcpkg_check_linkage.cmake")
include("${SCRIPTS}/cmake/vcpkg_clean_executables_in_bin.cmake")
include("${SCRIPTS}/cmake/vcpkg_clean_msbuild.cmake")
include("${SCRIPTS}/cmake/vcpkg_configure_cmake.cmake")
include("${SCRIPTS}/cmake/vcpkg_configure_gn.cmake")
include("${SCRIPTS}/cmake/vcpkg_configure_make.cmake")
include("${SCRIPTS}/cmake/vcpkg_configure_meson.cmake")
include("${SCRIPTS}/cmake/vcpkg_configure_qmake.cmake")
include("${SCRIPTS}/cmake/vcpkg_copy_pdbs.cmake")
include("${SCRIPTS}/cmake/vcpkg_copy_tool_dependencies.cmake")
include("${SCRIPTS}/cmake/vcpkg_copy_tools.cmake")
include("${SCRIPTS}/cmake/vcpkg_download_distfile.cmake")
include("${SCRIPTS}/cmake/vcpkg_download_sourceforge.cmake")
include("${SCRIPTS}/cmake/vcpkg_execute_build_process.cmake")
include("${SCRIPTS}/cmake/vcpkg_execute_required_process.cmake")
include("${SCRIPTS}/cmake/vcpkg_execute_required_process_repeat.cmake")
include("${SCRIPTS}/cmake/vcpkg_extract_archive.cmake")
include("${SCRIPTS}/cmake/vcpkg_extract_source_archive.cmake")
include("${SCRIPTS}/cmake/vcpkg_extract_source_archive_ex.cmake")
include("${SCRIPTS}/cmake/vcpkg_fail_port_install.cmake")
include("${SCRIPTS}/cmake/vcpkg_find_acquire_program.cmake")
include("${SCRIPTS}/cmake/vcpkg_fixup_cmake_targets.cmake")
include("${SCRIPTS}/cmake/vcpkg_fixup_pkgconfig.cmake")
include("${SCRIPTS}/cmake/vcpkg_from_bitbucket.cmake")
include("${SCRIPTS}/cmake/vcpkg_from_git.cmake")
include("${SCRIPTS}/cmake/vcpkg_from_github.cmake")
include("${SCRIPTS}/cmake/vcpkg_from_gitlab.cmake")
include("${SCRIPTS}/cmake/vcpkg_from_sourceforge.cmake")
include("${SCRIPTS}/cmake/vcpkg_get_program_files_platform_bitness.cmake")
include("${SCRIPTS}/cmake/vcpkg_get_windows_sdk.cmake")
include("${SCRIPTS}/cmake/vcpkg_host_path_list.cmake")
include("${SCRIPTS}/cmake/vcpkg_install_cmake.cmake")
include("${SCRIPTS}/cmake/vcpkg_install_copyright.cmake")
include("${SCRIPTS}/cmake/vcpkg_install_gn.cmake")
include("${SCRIPTS}/cmake/vcpkg_install_make.cmake")
include("${SCRIPTS}/cmake/vcpkg_install_meson.cmake")
include("${SCRIPTS}/cmake/vcpkg_install_msbuild.cmake")
include("${SCRIPTS}/cmake/vcpkg_install_nmake.cmake")
include("${SCRIPTS}/cmake/vcpkg_install_qmake.cmake")
include("${SCRIPTS}/cmake/vcpkg_list.cmake")
include("${SCRIPTS}/cmake/vcpkg_minimum_required.cmake")
include("${SCRIPTS}/cmake/vcpkg_replace_string.cmake")
include("${SCRIPTS}/cmake/vcpkg_test_cmake.cmake")
include("${SCRIPTS}/cmake/z_vcpkg_apply_patches.cmake")
include("${SCRIPTS}/cmake/z_vcpkg_forward_output_variable.cmake")
include("${SCRIPTS}/cmake/z_vcpkg_function_arguments.cmake")
include("${SCRIPTS}/cmake/z_vcpkg_get_cmake_vars.cmake")
include("${SCRIPTS}/cmake/z_vcpkg_prettify_command_line.cmake")
include("${SCRIPTS}/cmake/z_vcpkg_setup_pkgconfig_path.cmake")
include("${SCRIPTS}/cmake/z_vcpkg_fixup_rpath.cmake")
function(debug_message)
if(PORT_DEBUG)
z_vcpkg_function_arguments(ARGS)
list(JOIN ARGS " " ARG_STRING)
message(STATUS "[DEBUG] " "${ARG_STRING}")
endif()
endfunction()
function(z_vcpkg_deprecation_message)
z_vcpkg_function_arguments(ARGS)
list(JOIN ARGS " " ARG_STRING)
message(DEPRECATION "${ARG_STRING}")
endfunction()
option(_VCPKG_PROHIBIT_BACKCOMPAT_FEATURES "Controls whether use of a backcompat only support feature fails the build.")
if (_VCPKG_PROHIBIT_BACKCOMPAT_FEATURES)
set(Z_VCPKG_BACKCOMPAT_MESSAGE_LEVEL "FATAL_ERROR")
else()
set(Z_VCPKG_BACKCOMPAT_MESSAGE_LEVEL "WARNING")
endif()
vcpkg_minimum_required(VERSION 2022-10-12)
file(TO_CMAKE_PATH "${BUILDTREES_DIR}" BUILDTREES_DIR)
file(TO_CMAKE_PATH "${PACKAGES_DIR}" PACKAGES_DIR)
set(CURRENT_INSTALLED_DIR "${_VCPKG_INSTALLED_DIR}/${TARGET_TRIPLET}" CACHE PATH "Location to install final packages")
if(PORT)
set(CURRENT_BUILDTREES_DIR "${BUILDTREES_DIR}/${PORT}")
set(CURRENT_PACKAGES_DIR "${PACKAGES_DIR}/${PORT}_${TARGET_TRIPLET}")
endif()

without needing to use --x-scripts-root and copying all of the scripts to a different directory.

We noticed that pre-injection runs after port configs are included. So it might be used to override helper functions (but that can be done already with port overlays).

Wasn't the idea. The idea was to inject directly before/after the portfile as the name of the variable suggest. Some stuff does not work in the triplet (e.g. using VCPKG_TARGET_IS_WINDOWS, vcpkg function calls etc.). Best example to override for the pre injection is probably vcpkg_find_acquire_program(<TOOL_NAME>)

@BillyONeal
Copy link
Member

Wasn't the idea. The idea was to inject directly before/after the portfile as the name of the variable suggest. Some stuff does not work in the triplet (e.g. using VCPKG_TARGET_IS_WINDOWS, vcpkg function calls etc.). Best example to override for the pre injection is probably vcpkg_find_acquire_program(<TOOL_NAME>)

Can you show an example where someone would want to do that?

@BillyONeal BillyONeal removed the info:reviewed Pull Request changes follow basic guidelines label Jun 10, 2024
@Neumann-A
Copy link
Contributor Author

Can you show an example where someone would want to do that?

I cannot show an example I also noticed that overriding vcpkg_find_acquire_program is a bit more complex. I thought you could simply add vcpkg_find_acquire_program(<NEW_TOOL_NAME>) but no that doesn't work. (Due to set(program_information "${CMAKE_CURRENT_FUNCTION_LIST_DIR}/vcpkg_find_acquire_program(${program}).cmake"), so it would require overwriting vcpkg_find_acquire_program itself .)
However, you could use it if vcpkg_configure_make (or any other) is broken for you (and vcpkg won't fix it due to backwards compat concerns) and you want to use old versions.
Another interesting function to override could be vcpkg_install_copyright where it does some extra steps cleaning up the copyright or even extracting additional info which is then stored somewhere else in the tree.

@Osyotr
Copy link
Contributor

Osyotr commented Jul 12, 2024

Can you show an example where someone would want to do that?

Overriding vcpkg_from_* is something people ask for from time to time.

@Neumann-A
Copy link
Contributor Author

Overriding vcpkg_from_* is something people ask for from time to time.

Can be done in the triplet.

@BillyONeal BillyONeal removed the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Jul 24, 2024
@BillyONeal BillyONeal marked this pull request as ready for review July 25, 2024 20:54
@BillyONeal BillyONeal merged commit b16e501 into microsoft:master Jul 25, 2024
17 checks passed
@BillyONeal
Copy link
Member

Thanks!

@Neumann-A Neumann-A deleted the pre_post_portfile_injections branch July 25, 2024 20:55
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 category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants