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

Move merging original_cwd into overlay directories into VcpkgPaths. #1522

Merged
merged 1 commit into from
Nov 1, 2024

Conversation

BillyONeal
Copy link
Member

This work is part of resolving microsoft/vcpkg#30942 / https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1979597

When overlay directories from the config were added in #743 this added a condition where VcpkgPaths took responsibility to tack on the relative path, transforming those paths that come from the config into absolute paths. I did not realize at the time that this block was repeating machinery already present in IOverlayProvider family. The difference is that, there, it always assumed the prefix would be original_cwd.

As part of adding overlay-port-dirs, I needed to add the same kind of prefix handling as overlay-ports get, thus pointing this out to me. It doesn't make sense to keep two independent ways to do this, leaving two options:

  • Move the prefix stapling into VcpkgPaths (as done in this PR)
  • Move the prefix stapling related to configs down into OverlayPortProviderImpl

I chose to move into VcpkgPaths for several reasons:

  • Plumbing the information about how to handle config paths correctly into OverlayPortProviderImpl would have added many many function parameters to large parts of the product that are only passing paths information around.
  • The decision to chdir in VcpkgPaths is a consideration it makes, and OverlayPortProviderImpl would be just as happy if the relative paths were left alone. (And in fact I think the right behavior in the future would be to remove this harmful chdir but that is not proposed right now)
  • Despite making VcpkgPaths do "more work" here, I actually argue that it makes the "VcpkgPaths big ball of mud" problem better, by removing parts of the code that need to consider original_cwd.

This work is part of resolving microsoft/vcpkg#30942 / https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1979597

When overlay directories from the config were added in microsoft#743 this added a condition where VcpkgPaths took responsibility to tack on the relative path, transforming those paths that come from the config into absolute paths. I did not realize at the time that this block was repeating machinery already present in IOverlayProvider family. The difference is that, there, it always assumed the prefix would be original_cwd.

As part of adding overlay-port-dirs, I needed to add the same kind of prefix handling as overlay-ports get, thus pointing this out to me. It doesn't make sense to keep two independent ways to do this, leaving two options:

* Move the prefix stapling into VcpkgPaths (as done in this PR)
* Move the prefix stapling related to configs down into OverlayPortProviderImpl

I chose to move into VcpkgPaths for several reasons:

* Plumbing the information about how to handle config paths correctly into OverlayPortProviderImpl would have added many many function parameters to large parts of the product that are only passing paths information around.
* The decision to chdir in VcpkgPaths is a consideration *it* makes, and OverlayPortProviderImpl would be just as happy if the relative paths were left alone. (And in fact I think the right behavior in the future would be to remove this harmful chdir but that is not proposed right now)
* Despite making VcpkgPaths do "more work" here, I actually argue that it makes the "VcpkgPaths big ball of mud" problem better, by removing parts of the code that need to consider original_cwd.
@BillyONeal BillyONeal merged commit ce99f4c into microsoft:main Nov 1, 2024
6 checks passed
@BillyONeal BillyONeal deleted the overlay-relative-paths branch November 1, 2024 07:37
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