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_SCRIPTS_DIR and VCPKG_TOOLS_DIR to the CMake command call #1420

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Neumann-A
Copy link
Contributor

To make stuff mentioned in #1315 (comment) not magically (and maybe even wrongly) calculate those paths.

@@ -17,6 +17,8 @@ namespace vcpkg
local_variables.emplace_back("BUILDTREES_DIR", paths.buildtrees());
local_variables.emplace_back("_VCPKG_INSTALLED_DIR", paths.installed().root());
local_variables.emplace_back("DOWNLOADS", paths.downloads);
local_variables.emplace_back("VCPKG_SCRIPTS_DIR", paths.scripts);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should this be just SCRIPTS since it is already defined in ports.cmake ?

Copy link
Member

Choose a reason for hiding this comment

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

That makes sense to me

@BillyONeal
Copy link
Member

Alternately I think --x-scripts-root should die because there is no safe way for it to be used at present.

@Neumann-A
Copy link
Contributor Author

Alternately I think --x-scripts-root should die because there is no safe way for it to be used at present.

Please explain

@BillyONeal
Copy link
Member

Alternately I think --x-scripts-root should die because there is no safe way for it to be used at present.

Please explain

The only acceptable content for the scripts tree is the same content that matches the copy of vcpkg, the tool, itself. There is no meaning to the user trying to replace where that is, the only reason the switch exists has long gone.

@Neumann-A
Copy link
Contributor Author

Alternately I think --x-scripts-root should die because there is no safe way for it to be used at present.

Please explain

The only acceptable content for the scripts tree is the same content that matches the copy of vcpkg, the tool, itself.

I am still missing the why explanation here. So I still need a sentence which starts something like "because otherwise ...." or something logically similar.

Whatever the decision about the cli switch this PR just passes down internal path knowledge. If this is scripts dir or the dir of the cmake entry point (ports.cmake) with the scripts subfolder added doesnt really matter.

@BillyONeal
Copy link
Member

I am still missing the why explanation here. So I still need a sentence which starts something like "because otherwise ...." or something logically similar.

There isn't going to be a 'because otherwise'. We reserve the right to make coordinated tool and scripts tree changes, so the set of potential issues is undefined.

Whatever the decision about the cli switch this PR just passes down internal path knowledge. If this is scripts dir or the dir of the cmake entry point (ports.cmake) with the scripts subfolder added doesnt really matter.

In general I'm happy with not calculating something multiple times; I just think the right fix here is to delete the scripts dir setting entirely. It does not exist to do practical things, it exists because once upon a time we blindly added knobs for "all the directories" even ones that would be meaningless to touch.

local_variables.emplace_back("VCPKG_TOOLS_DIR", paths.tools);

This proposed VCPKG_TOOLS_DIR and the one in scripts\buildsystems\vcpkg.cmake are totally different; the one in VcpkgPaths refers to %DOWNLOADS%\tools, while the one in vcpkg.cmake refers to in the installed tree.

@Neumann-A
Copy link
Contributor Author

This proposed VCPKG_TOOLS_DIR and the one in scripts\buildsystems\vcpkg.cmake are totally different; the one in VcpkgPaths refers to %DOWNLOADS%\tools, while the one in vcpkg.cmake refers to in the installed tree.

There is no VCPKG_TOOLS_DIR in vcpkg.cmake there is only a Z_VCPKG_TOOLS_DIR which is not related to this one. (and it is correct to point into downloads; maybe rename vcpkg.cmake into Z_VCPKG_INSTALLED_TOOLS_DIR to be more explicit) I could also rename this one to VCPKG_DOWNLOADED_TOOLS_DIR (I really don't care about the naming)

There isn't going to be a 'because otherwise'. We reserve the right to make coordinated tool and scripts tree changes, so the set of potential issues is undefined.

I don't think you can do that considering that ports.cmake defines:

set(SCRIPTS "${CMAKE_CURRENT_LIST_DIR}" CACHE PATH "Location to stored scripts")
list(APPEND CMAKE_MODULE_PATH "${SCRIPTS}/cmake")

which makes the script dir an (indirectly) official variable and changing that would be a breaking change for others? I don't mind the scripts variable too much IF the triplet file always gets to to see the same set of predefined functions. Currently the triplet is included in two different ways once from ports.cmake with everything defined and once with nothing when calculating the settings.

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

Successfully merging this pull request may close these issues.

2 participants