-
Notifications
You must be signed in to change notification settings - Fork 339
Remove optionality in ActionPlan and Summary #1894
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
Open
BillyONeal
wants to merge
38
commits into
microsoft:main
Choose a base branch
from
BillyONeal:remove-optionality
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Broken: ``` PS C:\Dev\test> C:\Dev\vcpkg-tool\out\build\Win-x64-Debug-WithArtifacts\vcpkg.exe --vcpkg-root C:\Dev\vcpkg install warning: configuration contains the following unrecognized fields: $.overridesIf these are documented fields that should be recognized try updating the vcpkg tool. Detecting compiler hash for triplet x64-windows... ``` Fixed: ``` PS C:\Dev\test> C:\Dev\vcpkg-tool\out\build\Win-x64-Debug-WithArtifacts\vcpkg.exe --vcpkg-root C:\Dev\vcpkg install warning: configuration contains the following unrecognized fields: $.overrides If these are documented fields that should be recognized try updating the vcpkg tool. Detecting compiler hash for triplet x64-windows... ```
… aren't actually build failure messages.
Undoes microsoft/vcpkg#7305 ; we have no tests, documentation, or usage of this as far as I can tell. There were very few callers of public_abi(). With that, none of the 'ActionType' enums are necessary any longer as they are inferable from something else.
…clude-deep # Conflicts: # azure-pipelines/e2e-assets/ci-skipped-ports/baseline.fail.txt # azure-pipelines/e2e-assets/ci-skipped-ports/baseline.skip.txt # azure-pipelines/end-to-end-tests-dir/ci.ps1 # include/vcpkg/dependencies.h # include/vcpkg/fwd/dependencies.h # src/vcpkg-test/dependencies.cpp # src/vcpkg-test/plan.cpp # src/vcpkg/cmakevars.cpp # src/vcpkg/commands.ci.cpp # src/vcpkg/commands.install.cpp # src/vcpkg/commands.set-installed.cpp # src/vcpkg/commands.test-features.cpp # src/vcpkg/dependencies.cpp
# Conflicts: # include/vcpkg/dependencies.h # src/vcpkg-test/plan.cpp # src/vcpkg/commands.ci.cpp # src/vcpkg/commands.install.cpp # src/vcpkg/commands.set-installed.cpp # src/vcpkg/dependencies.cpp
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is some cleanup I did while trying to better understand how the dependency planner works trying to fix microsoft/vcpkg#48797 (comment) et al.
ci.ps1tests.CMakeVarProvider::load_tag_varswhich wanted a wholeActionPlandespite only needing the install actionsPackageSpectoExtendedBuildResultto avoid cases where reporters were trying to re-parse theSourceControlFileet al. to figure out the package nameInstallSpecSummaryfromSpecSummaryto avoidvalue_or_exits when reporting about install actions. (Those values were only empty for removals which continue to useSpecSummary)InstallPlanActionintoAlreadyInstalledPlanActionandInstallPlanAction, where the former has theInstalledPackageViewfor already installed things, and the latter has theSourceControlFileAndLocationfor to-be-installed things. This avoids a lot of optionality in the rest of the product needing to deal with these two cases, particularly in parts that only ever cared about to-be-installed parts.DiagnosticLinerather thanLocalizedString(fixes the print color of those)create_binary_control_file, loop through thefeature_dependenciesdirectly rather than trying to look each of them up by name. Note thatfeature_listisfeature_dependenciesprojected to only the names.commands.depend-info.cpp, don't form a vector of pointers when the real vector will do. Note that it callscreate_feature_install_planwith an empty/blankstatus_db, soalready_installedis known to be empty.perform_install_plan_actionto record build time directly rather than trying to write during destructors. (This seems easier to reason about to me than the relatively non-local behavior ofTrackedPackageInstallGuard, which has been deleted)Other observations:
InstallPlanActionagain for the "pre-Build::compute_all_abis" and "post-Build::compute_all_abis" state. I'm not sure if that's a good change anymore though because unknown ABIs are a "normal" state of affairs, e.g. when--editableis used. I think that makes all callers ofpackage_abi_or_emptyorpackage_abi_or_exitsuspect and those probably need to be fixed, but this PR is big enough.