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

Configuring overlay ports and overlay triplets through the manifest #743

Merged
merged 30 commits into from
Oct 20, 2022

Conversation

valeriaconde
Copy link
Contributor

@valeriaconde valeriaconde commented Oct 10, 2022

@valeriaconde
Copy link
Contributor Author

@microsoft-github-policy-service agree [company="microsoft"]

@valeriaconde
Copy link
Contributor Author

@microsoft-github-policy-service agree company="microsoft"

@autoantwort
Copy link
Contributor

Currently relative paths are relative to the CWD and not to the manifest root. And I guess e2e test will come to test relative paths in manifest file and manifest root != CWD.

I really like this change 👍

@Neumann-A
Copy link
Contributor

The only issue I have with adding this stuff into the manifest is that the manifest becomes kind of a configuration file for vcpkg instead of a clean dependency definition. Maybe vcpkg should add a new differentiation and add a vcpkg-project.json instead of calling everything vcpkg.json

@BillyONeal
Copy link
Member

The only issue I have with adding this stuff into the manifest is that the manifest becomes kind of a configuration file for vcpkg instead of a clean dependency definition. Maybe vcpkg should add a new differentiation and add a vcpkg-project.json instead of calling everything vcpkg.json

That is, vcpkg-configuration.json, where the registries definitions already live. That makes sense to me...

@BillyONeal
Copy link
Member

That is, vcpkg-configuration.json, where the registries definitions already live. That makes sense to me...

Wait, that's what this PR already does. @Neumann-A I think the "vcpkg.json is the durable/transitive declaration of dependencies" is unaffected here.

@Neumann-A
Copy link
Contributor

Wait, that's what this PR already does. @Neumann-A I think the "vcpkg.json is the durable/transitive declaration of dependencies" is unaffected here.

Maybe the title of the PR should be changed? Or is vcpkg-configuration.json also considered to be the manifest?
Furthermore can i use the manifest overlays without defining a registry?

@valeriaconde
Copy link
Contributor Author

We consider vcpkg-configuration.json as part of what composes "the manifest" , although this is not clear in the docs for sure.

Furthermore can i use the manifest overlays without defining a registry?

Yes, and just like registries you could also configure overlays in vcpkg.json using the configuration { } tag

@Neumann-A
Copy link
Contributor

Yes, and just like registries you could also configure overlays in vcpkg.json using the configuration { } tag

Is this documented somewhere? I always thought you need to have two different files (vcpkg.json; vcpkg-configuration.json).
https://github.com/microsoft/vcpkg/blob/master/docs/users/manifests.md doesn't mention that.
https://github.com/microsoft/vcpkg/blob/master/docs/users/registries.md also seems to not mention it.
https://raw.githubusercontent.com/microsoft/vcpkg/master/scripts/vcpkg.schema.json also doesn't have that if I read it correctly.

@vicroms
Copy link
Member

vicroms commented Oct 12, 2022

Yes, and just like registries you could also configure overlays in vcpkg.json using the configuration { } tag

Is this documented somewhere? I always thought you need to have two different files (vcpkg.json; vcpkg-configuration.json). https://github.com/microsoft/vcpkg/blob/master/docs/users/manifests.md doesn't mention that. https://github.com/microsoft/vcpkg/blob/master/docs/users/registries.md also seems to not mention it. https://raw.githubusercontent.com/microsoft/vcpkg/master/scripts/vcpkg.schema.json also doesn't have that if I read it correctly.

That would be my fault, I don't think it is documented other than the comment I made on the PR #239.

@valeriaconde
Copy link
Contributor Author

valeriaconde commented Oct 12, 2022

That would be my fault, I don't think it is documented other than the comment I made on the PR #239.

I added this to the docs PR I have open (currently on the description of this PR)

src/vcpkg/vcpkgpaths.cpp Outdated Show resolved Hide resolved
src/vcpkg/vcpkgpaths.cpp Outdated Show resolved Hide resolved
src/vcpkg/vcpkgpaths.cpp Outdated Show resolved Hide resolved
src/vcpkg/vcpkgpaths.cpp Outdated Show resolved Hide resolved
@valeriaconde valeriaconde requested a review from vicroms October 18, 2022 18:16
.gitignore Outdated Show resolved Hide resolved
azure-pipelines/end-to-end-tests-dir/overlays.ps1 Outdated Show resolved Hide resolved
src/vcpkg-test/configmetadata.cpp Outdated Show resolved Hide resolved
include/vcpkg/vcpkgpaths.h Outdated Show resolved Hide resolved
src/vcpkg/vcpkgpaths.cpp Outdated Show resolved Hide resolved
src/vcpkg/vcpkgpaths.cpp Outdated Show resolved Hide resolved
src/vcpkg/vcpkgpaths.cpp Show resolved Hide resolved
@BillyONeal BillyONeal merged commit a1e5a75 into microsoft:main Oct 20, 2022
@BillyONeal
Copy link
Member

Thanks!

@valeriaconde valeriaconde deleted the val-port-overlays branch October 20, 2022 18:21
BillyONeal added a commit to BillyONeal/vcpkg-tool that referenced this pull request Oct 22, 2024
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 added a commit to BillyONeal/vcpkg-tool that referenced this pull request Oct 24, 2024
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 added a commit to BillyONeal/vcpkg-tool that referenced this pull request Oct 29, 2024
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 added a commit to BillyONeal/vcpkg-tool that referenced this pull request Oct 31, 2024
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 added a commit that referenced this pull request Nov 1, 2024
…1522)

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.
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.

7 participants