Add warning message for change in default triplet to host triplet #881

dan-shaw · 2023-02-01T23:56:19Z

No description provided.

autoantwort · 2023-02-02T11:26:50Z

So it will always change to x64-windows even on arm64 windows systems? Or will it change to what the system is like on every other os?

dan-shaw · 2023-02-03T02:25:37Z

What is the current default triplet for arm64 windows systems? I'm guessing it is x86-windows and it should be arm64-windows.

dg0yt · 2023-02-03T06:17:51Z

I think the user expects a native build. So the default triplet should generally become what is the auto-detected host triplet. Even x86-windows for those machines.

Neumann-A · 2023-02-04T09:45:17Z

i thinkl the default triplet should be the detected host triplet. I mean vcpkg already as a default for that right?

BillyONeal · 2023-02-06T20:34:22Z

I think the user expects a native build. So the default triplet should generally become what is the auto-detected host triplet. Even x86-windows for those machines.

I'm pretty sure that already doesn't work because we try to fetch amd64 dependencies.

src/vcpkg/triplet.cpp

include/vcpkg/base/messages.h

ras0219-msft · 2023-02-07T23:37:30Z

src/vcpkg/triplet.cpp

+        }
+        else
+        {
+            for (auto&& arg : args.command_arguments)


Does this trigger on vcpkg search or vcpkg create or vcpkg find?

I thought those commands do not use the default triplet?

Then that's good -- the answer's no. I just wanted to confirm that you've checked them.

Oh -- does this trigger on an invalid command? vcpkg asdf zlib?

Yes it will trigger for that.

BillyONeal · 2023-02-08T03:14:30Z

include/vcpkg/base/messages.h

+    DECLARE_MESSAGE(DefaultTriplet,
+                    (msg::triplet),
+                    "",
+                    "Starting with the September 2023 release, the default triplet for builds will change from "
+                    "x86-windows to the detected host triplet ({triplet}).");


We need to tell the user what to do about the message if they want it to go away:

Starting with the September 2023 release, the default triplet for builds will change from x86-windows to the detected host triplet ({triplet}). To resolve this message, add --triplet {triplet} for the new value, or --triplet {value} to keep the same behavior after the change.

BillyONeal · 2023-02-08T03:26:35Z

src/vcpkg/triplet.cpp

+            }
+            else
+            {
+                for (auto&& arg : args.command_arguments)


I'm really not a fan of assuming that the command arguments are ports regardless of the executed command like this. For instance, I believe this will do the wrong thing for things like add artifact. (That is vcpkg add artifact gcc should not emit a warning but I believe this will make it do so)

I think commands that are calling check_and_get_full_package_spec need different handling here than commands that do not.

BillyONeal · 2023-02-08T03:37:54Z

You might want to consider something like:

struct DefaultTripletProvider {
    DefaultTripletProvider(Triplet current_default_triplet, Triplet new_default_triplet)
        : current_default_triplet(current_default_triplet)
        , new_default_triplet(new_default_triplet)
        , provided(current_default_triplet == new_default_triplet) // don't print if there is no change
    {
    }

    Triplet get() {
        if (!std::exchange(provided, true)) {
            // print the message
        }

        return new_default_triplet;
    }

private:
    Triplet current_default_triplet;
    Triplet new_default_triplet;
    bool provided;
};

then pass this around instead of the naked triplet through all the bits that are processing user input. Then, so long as they never call get(), they emit no warning.

In particular,

    // in commands.interface.h
    struct TripletCommand
    {
        virtual void perform_and_exit(const VcpkgCmdArguments& args,
                                      const VcpkgPaths& paths,
                                      Triplet default_triplet,
                                      Triplet host_triplet) const = 0;
        virtual ~TripletCommand() = default;
    };

    // in input.h
    FullPackageSpec check_and_get_full_package_spec(std::string&& spec_string,
                                                    Triplet default_triplet,
                                                    ZStringView example_text,
                                                    const VcpkgPaths& paths);

would become:

    struct TripletCommand
    {
        virtual void perform_and_exit(const VcpkgCmdArguments& args,
                                      const VcpkgPaths& paths,
                                      DefaultTripletProvider& default_triplet,
                                      Triplet host_triplet) const = 0;
        virtual ~TripletCommand() = default;
    };

    FullPackageSpec check_and_get_full_package_spec(std::string&& spec_string,
                                                    DefaultTripletProvider& default_triplet,
                                                    ZStringView example_text,
                                                    const VcpkgPaths& paths);

include/vcpkg/triplet.h

src/vcpkg/build.cpp

ras0219-msft · 2023-02-13T17:58:52Z

src/vcpkg/commands.upgrade.cpp

@@ -96,6 +96,7 @@ namespace vcpkg::Commands::Upgrade
        const std::vector<PackageSpec> specs = Util::fmap(args.command_arguments, [&](auto&& arg) {
            return check_and_get_package_spec(std::string(arg), default_triplet, COMMAND_STRUCTURE.example_text, paths);
        });
+        print_default_triplet_warning(args, args.command_arguments);


vcpkg upgrade shouldn't emit a warning -- it doesn't use the target triplet.

Sorry, while I was making this comment before I made a mistake; vcpkg upgrade alone shouldn't emit a warning. However, vcpkg upgrade zlib does use the target triplet.

So, specifically, when args.command_arguments is empty, we should not emit a warning.

BillyONeal

Thanks!

include/vcpkg/triplet.h

BillyONeal · 2023-02-13T19:19:04Z

src/vcpkg/commands.dependinfo.cpp

@@ -305,6 +305,7 @@ namespace vcpkg::Commands::DependInfo
            return check_and_get_full_package_spec(
                std::string{arg}, default_triplet, COMMAND_STRUCTURE.example_text, paths);
        });
+        print_default_triplet_warning(args, args.command_arguments);


I still think it would be better to do this as part of check_and_get_full_package_spec but that's nitpick territory.

ras0219-msft · 2023-02-13T22:29:12Z

I made a mistake in my previous comment on commands.upgrade.cpp -- please take a look again.

# Conflicts: # include/vcpkg/base/messages.h # locales/messages.json # src/vcpkg/base/messages.cpp

BillyONeal · 2023-02-14T00:13:55Z

Thanks a bunch!

dan-shaw and others added 4 commits February 1, 2023 15:45

add warning for default triplet

34a2197

fix

d07d2d6

Merge remote-tracking branch 'origin/main' into default-triplet

71c239e

Format

e713f24

ras0219-msft reviewed Feb 7, 2023

View reviewed changes

fix

cfd4d2c

dan-shaw changed the title ~~Add warning message for change in default triplet to x64-windows~~ Add warning message for change in default triplet to host triplet Feb 8, 2023

BillyONeal requested changes Feb 8, 2023

View reviewed changes

dan-shaw added 4 commits February 10, 2023 19:17

fix

e5d105e

fix

9887875

fix

d251b0e

fix

6eb8890

ras0219-msft approved these changes Feb 13, 2023

View reviewed changes

fix

c839688

BillyONeal approved these changes Feb 13, 2023

View reviewed changes

dan-shaw and others added 3 commits February 13, 2023 11:58

fix

bda99c8

Merge remote-tracking branch 'origin/main' into default-triplet

f7bab33

format

40c178f

dan-shaw and others added 2 commits February 13, 2023 15:15

fix

e624b00

Merge remote-tracking branch 'origin/main' into default-triplet

feb5960

# Conflicts: # include/vcpkg/base/messages.h # locales/messages.json # src/vcpkg/base/messages.cpp

BillyONeal merged commit 3e20dcc into microsoft:main Feb 14, 2023

autoantwort mentioned this pull request Feb 16, 2023

switch to host triplet as the default. #640

Closed

SunBlack mentioned this pull request Mar 14, 2023

[vcpkg] Passing --dgml should suppress warnings (on stdout) microsoft/vcpkg#30180

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add warning message for change in default triplet to host triplet #881

Add warning message for change in default triplet to host triplet #881

dan-shaw commented Feb 1, 2023

autoantwort commented Feb 2, 2023

dan-shaw commented Feb 3, 2023

dg0yt commented Feb 3, 2023

Neumann-A commented Feb 4, 2023

BillyONeal commented Feb 6, 2023

ras0219-msft Feb 7, 2023

dan-shaw Feb 8, 2023

ras0219-msft Feb 8, 2023

ras0219-msft Feb 8, 2023

BillyONeal Feb 8, 2023

BillyONeal Feb 8, 2023 •

edited

Loading

BillyONeal Feb 8, 2023

BillyONeal commented Feb 8, 2023

ras0219-msft Feb 13, 2023

ras0219-msft Feb 13, 2023

BillyONeal left a comment

BillyONeal Feb 13, 2023

ras0219-msft commented Feb 13, 2023

BillyONeal commented Feb 14, 2023

Add warning message for change in default triplet to host triplet #881

Add warning message for change in default triplet to host triplet #881

Conversation

dan-shaw commented Feb 1, 2023

autoantwort commented Feb 2, 2023

dan-shaw commented Feb 3, 2023

dg0yt commented Feb 3, 2023

Neumann-A commented Feb 4, 2023

BillyONeal commented Feb 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BillyONeal Feb 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BillyONeal commented Feb 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BillyONeal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ras0219-msft commented Feb 13, 2023

BillyONeal commented Feb 14, 2023

BillyONeal Feb 8, 2023 •

edited

Loading