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

switch to host triplet as the default. #640

Closed
wants to merge 10 commits into from

Conversation

Neumann-A
Copy link
Contributor

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

https://twitter.com/augustin_popa/status/1549127848140480512 ....

(ready to be merged in september 2023 ;) )

@BillyONeal
Copy link
Member

As I said back in #614 (and elsewhere), this is a change we can't make lightly because it breaks existing scripts (and our 'no changes means no changes' mantra).

We have discussed doing this before back when VS2022 changed the default for new projects, and said that in order to do it we need:

  1. A period of time (~6 months) before we make the change where we warn that we are going to make the change and how to fix it. (Add --triplet or qualify all references with :triplet)
  2. A period of time (~6 months) after we make the change where we warn that the behavior changed.

@Neumann-A
Copy link
Contributor Author

Don't you have telemetry how many people actually still build x86 outside manifest mode?

Most people probably already have VCPKG_DEFAULT_TRIPLET set because of the default vcpkg has and will never observe the change any way ;)
Furthermore it is more surprising that the default host triplet is x64-windows while the target is x86-windows.

Just make a big blog post with the release that the default triplet switched and add an extra message to bootstrap that the triplet has switched.... I really don't see a reason to give any grace period at all. New users have the problem to adapt. Old users probably don't care that much and know what they need to do.

@autoantwort
Copy link
Contributor

I think you can close this now because of #881

@Neumann-A
Copy link
Contributor Author

I think you can close this now because of #881

Just prints the warning does not do the change. But according to the warning something more sophisticated will be implemented? I'll keep this open until that more sophisticated stuff is actually implemented ;)

@BillyONeal
Copy link
Member

Just prints the warning does not do the change. But according to the warning something more sophisticated will be implemented? I'll keep this open until that more sophisticated stuff is actually implemented ;)

It isn't any more sophisticated; it's just that the clock is now running on giving people reasonable warning that the change is happening.

@Neumann-A
Copy link
Contributor Author

It isn't any more sophisticated;

Ok the message gives the impression that at least the correct host default would be used, e.g. on arm64 it should be arm64-windows and not x64-windows. Maybe this can be changed to simply call the function for the default host-triplet instead?

E.g. removing the #ifdef and call default_host_triplet(args) instead of system_triplet()

@BillyONeal
Copy link
Member

Ah, maybe. I kinda forget arm64-windows exists 😅

@Neumann-A Neumann-A changed the title switch to x64-windows as the default switch to host triplet as the default. Mar 9, 2023
@Neumann-A Neumann-A marked this pull request as draft April 19, 2023 10:34
# Conflicts:
#	include/vcpkg/triplet.h
#	src/vcpkg/commands.build.cpp
#	src/vcpkg/commands.check-support.cpp
#	src/vcpkg/commands.ci.cpp
#	src/vcpkg/commands.dependinfo.cpp
#	src/vcpkg/commands.export.cpp
#	src/vcpkg/commands.install.cpp
#	src/vcpkg/commands.remove.cpp
#	src/vcpkg/commands.set-installed.cpp
#	src/vcpkg/commands.upgrade.cpp
#	src/vcpkg/triplet.cpp
@Neumann-A
Copy link
Contributor Author

Neumann-A commented Aug 23, 2023

Don't know what to do about:

  D:\a\vcpkg-tool\vcpkg-tool\out\build\windows-ci\vcpkg.ps1 --x-buildtrees-root=D:\a\vcpkg-tool\vcpkg-tool\out\build\windows-ci\work\testing\buildtrees --x-install-root=D:\a\vcpkg-tool\vcpkg-tool\out\build\windows-ci\work\testing\installed --x-packages-root=D:\a\vcpkg-tool\vcpkg-tool\out\build\windows-ci\work\testing\packages --overlay-ports=D:\a\vcpkg-tool\vcpkg-tool\azure-pipelines/e2e-ports/overlays --overlay-triplets=D:\a\vcpkg-tool\vcpkg-tool\azure-pipelines/e2e-ports/triplets depend-info vcpkg-hello-world-1
  vcpkg-cmake: 
  vcpkg-cmake-config: 
  vcpkg-hello-world-1: vcpkg-cmake, vcpkg-cmake-config
  
  Exception: D:\a\vcpkg-tool\vcpkg-tool\azure-pipelines\end-to-end-tests-dir\cli.ps1:24
  Line |
    24 |          throw 'depend-info with unqualified spec should emit the trip …
       |          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
       | depend-info with unqualified spec should emit the triplet warning

@BillyONeal: I tested with an official release and it also doesn't print the triplet with depend-info.
See:

neumann@heineken MINGW64 /e/vcpkg_folders/master_clean (update_zlib)
$ ./bootstrap-vcpkg.sh
Downloading https://github.com/microsoft/vcpkg-tool/releases/download/2023-08-09/vcpkg.exe -> E:\vcpkg_folders\master_clean\vcpkg.exe... done.
Validating signature... done.

vcpkg package management program version 2023-08-09-9990a4c9026811a312cb2af78bf77f3d9d288416

See LICENSE.txt for license information.

neumann@heineken MINGW64 /e/vcpkg_folders/master_clean (update_zlib)
$ ./vcpkg.exe depend-info zlib
vcpkg-cmake:
zlib: vcpkg-cmake

@Neumann-A
Copy link
Contributor Author

Neumann-A commented Aug 23, 2023

Ok figured out why. I had VCPKG_DEFAULT(_HOST)?_TRIPLET set in the environment.
But this:

E:\vcpkg_folders\master_clean>vcpkg.exe depend-info zlib
warning: Starting with the September 2023 release, the default triplet for vcpkg libraries will change from x86-windows to the detected host triplet (x64-windows). To resolve this message, add --triplet x86-windows to keep the same behavior.
vcpkg-cmake:
zlib: vcpkg-cmake

is probably not what the test wanted to have or is it? (I currently assume that the test is generally broken an did not want to detect the warning)

@@ -438,11 +438,6 @@ namespace vcpkg::Export
return parse_package_spec(
arg, default_triplet, default_triplet_used, COMMAND_STRUCTURE.get_example_text());
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 the usage of
default_triplet, default_triplet_used
generally be removed?
I kind of let them in since removing then would create a large changeset.

@Neumann-A
Copy link
Contributor Author

Ah okay i misread I thought the idea was to emit the triplet for host deps or something in depend-info but actually the tests tells me that it is about warning itself.

@Neumann-A Neumann-A marked this pull request as ready for review August 23, 2023 09:18
# Conflicts:
#	src/vcpkg/commands.build.cpp
#	src/vcpkg/commands.ci.cpp
BillyONeal added a commit to BillyONeal/vcpkg-tool that referenced this pull request Sep 5, 2023
This removes the 'in September 2023' message and does what the message said we would do. For the next 6 months we'll warn that the behavior changed.

The specific change to triplet.cpp to change the triplet setting is from microsoft#640

Co-authored-by: Alexander Neumann <30894796+Neumann-A@users.noreply.github.com>
@Neumann-A Neumann-A closed this Sep 5, 2023
BillyONeal added a commit that referenced this pull request Sep 6, 2023
#1180)

This removes the 'in September 2023' message and does what the message said we would do. For the next 6 months we'll warn that the behavior changed.

The specific change to triplet.cpp to change the triplet setting is from #640

Co-authored-by: Alexander Neumann <30894796+Neumann-A@users.noreply.github.com>
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.

3 participants