Skip to content

Remove baseline warning#27

Merged
strega-nil merged 7 commits intomicrosoft:mainfrom
strega-nil:remove-baseline-warning
Apr 14, 2021
Merged

Remove baseline warning#27
strega-nil merged 7 commits intomicrosoft:mainfrom
strega-nil:remove-baseline-warning

Conversation

@strega-nil
Copy link
Contributor

we don't want people to use baseline necessarily; builtin-baseline is also completely valid.

" Instead, use the \"baseline\" field of the registry.\n");
if (builtin_registry->m_baseline_identifier.empty())
{
builtin_registry->m_baseline_identifier.assign(baseline.begin(), baseline.end());
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should warn if m_baseline_identifier.empty() && !baseline.empty()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hrm, okay.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure about this; for example:

vcpkg.json:

{
  ...,
  "builtin-baseline": "b1"

vcpkg-configuration.json:

{
  "registries": [
    {
      "kind": "builtin",
      "baseline": "b2",
      "packages": [ "a", "b", "c" ]
    }
  ]
}

Copy link
Collaborator

@ras0219-msft ras0219-msft Mar 11, 2021

Choose a reason for hiding this comment

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

Right; so in this case we should warn to the user about which of the two baselines they've specified will be used (I assume b2?). I could see an argument for not warning iff they are the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean, they'll both be used: b2 will be used for packages a, b, and c, and b1 will be used for everything else.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the syntax for changing the default registry's baseline? I suspect this means we should warn in the case that the user has specified "builtin-baseline" AND specifies "default-registry". If that is already the case in this PR, then this comment is resolved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The syntax for changing the default registry's baseline is:

{
  "default-registry": {
    "kind": "...",
    ...,
    "baseline": "<baseline>"
  }
}

This is not warned against in this PR; what should the warning look like, do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

"Warning: Specifying the "default-registry" overrides the "builtin-baseline" field. Pass to suppress this warning message."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we only want builtin-baseline to do something to a registry that is both builtin, and default? Currently it does something to registries that are builtin, even if they're non-default.

Copy link
Collaborator

@ras0219-msft ras0219-msft Mar 30, 2021

Choose a reason for hiding this comment

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

It should only affect the default builtin baseline in my opinion. I see the following scenarios:

  1. No vcpkg-configuration.json (no warn)
  2. adds other registries but doesn't change the default (no warn)
  3. doesn't change the default, but does re-add the builtin registry to get packages with a different baseline (no warn)
  4. re-defines the default to builtin, but at a different baseline (warn)

I think the most straightforward UX is all registries defined in the configuration file must have a baseline specified. Therefore, the only case that builtin-baseline could possibly apply would be in the undefined default case.

@strega-nil strega-nil added requires:discussion This PR requires discussion of the correct way forward and removed requires:discussion This PR requires discussion of the correct way forward labels Mar 30, 2021
@strega-nil strega-nil force-pushed the remove-baseline-warning branch from 0c41780 to 096e402 Compare March 31, 2021 16:39
@strega-nil strega-nil merged commit 0d21b3e into microsoft:main Apr 14, 2021
strega-nil added a commit that referenced this pull request Apr 15, 2021
The following PRs are included:

* hopefully fix crash in constraints (#60)
* [vcpkg] allow --version to check the version (#50)
* Remove baseline warning (#27)
* [git] always pass autocrlf=false (#58)
* ignore QtCreator CMake project files (#54)
* ignore .DS_store files (#53)
* [vcpkg] x-add-version now also checks if the manifest file is properly formatted (#43)
* hopefully fix ci issue #16773 (#34)
* Add docs to set VCPKG_ROOT to run tests (#45)
* [vcpkg] x-add-version improve speed by calling get_builtin_baseline only once (#44)
* add clang-format version to format-cxxcode (#41)
* [vcpkg] Introduce experimental workaround X_VCPKG_NUGET_ID_PREFIX (#40)
* [supports] Add `native` identifier expression and x-check-support command (#29)
* [metrics] Split reporting of installs into name:triplet (#39)
* [vcpkg] Improve error when accessing missing feature (#38)
* [vcpkg] Allow shallow git registries (#37)
* Disable git autocrlf when archiving tree (#36)
* Use only named packages from extra registries (#35)
* [registries] add metrics (#30)
* Add vcpkg policy cmake helper port support (#17)
* [osx] add support for rosetta (#23)
* don't build tls12-download unless it's needed (#33)
* Add new telemetry points for versioning (#21)
* add cmake_minimum_required to vcpkg_tags (#25)
* [x-add-versions] Perform atomic replacement of versioning files (#28)
* [tools] support gsutil (#19)
* add CUDA 11.1 and 11.2 to KEEP_ENV_VARS defaults (#26)
* Add finite timeout on CURL metrics endpoint. (#22)
* fix UB in make_error_code(utf8_errc) (#18)
@strega-nil strega-nil deleted the remove-baseline-warning branch May 5, 2021 15:32
vicroms added a commit to vicroms/vcpkg-tool that referenced this pull request Aug 28, 2025
# This is the 1st commit message:

WIP

# The commit message #2 will be skipped:

# Fix build error

# The commit message microsoft#3 will be skipped:

# Implement bulk operation

# The commit message microsoft#4 will be skipped:

# Remove unused struct

# The commit message microsoft#5 will be skipped:

# Follow redirects

# The commit message microsoft#6 will be skipped:

# Get HTTP response code

# The commit message microsoft#7 will be skipped:

# Map URL response code to the correct index

# The commit message microsoft#8 will be skipped:

# Map URL response code to the correct index

# The commit message microsoft#9 will be skipped:

# Undo changes to workflows

# The commit message microsoft#10 will be skipped:

# Implement store to asset cache

# The commit message microsoft#11 will be skipped:

# handle file:// protocol

# The commit message microsoft#12 will be skipped:

# Implement try_download_file

# The commit message microsoft#13 will be skipped:

# Add missing calls to curl_easy_cleanup

# The commit message microsoft#14 will be skipped:

# Fix global init

# The commit message microsoft#15 will be skipped:

# Replace all calls in downloads.cpp and fix unit tests

# The commit message microsoft#16 will be skipped:

# use libcurl to submit metrics payload

# The commit message microsoft#17 will be skipped:

# Use Schannel on Windows

# The commit message microsoft#18 will be skipped:

# Apply suggestions from code review
#
# Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

# The commit message microsoft#19 will be skipped:

# Fix unit test

# The commit message microsoft#20 will be skipped:

# Change curl error messages

# The commit message microsoft#21 will be skipped:

# generate message map

# The commit message microsoft#22 will be skipped:

# Always call multi_remove_handle

# The commit message microsoft#23 will be skipped:

# Fix end-to-end tests

# The commit message microsoft#24 will be skipped:

# Cleanup header lists

# The commit message microsoft#25 will be skipped:

# curl global init order

# The commit message microsoft#26 will be skipped:

# Use external libcurl on non-Windows

# The commit message microsoft#27 will be skipped:

# Install libcurl4 dev package on Linux

# The commit message microsoft#28 will be skipped:

# Disable metrics by default in debug builds

# The commit message microsoft#29 will be skipped:

# Set user agent using CURLOPT_USERAGENT

# The commit message microsoft#30 will be skipped:

# curl initialization again
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.

4 participants