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

vcpkg add port: Handle feature core. #1163

Merged
merged 4 commits into from
Oct 31, 2023

Conversation

autoantwort
Copy link
Contributor

Don't crash at vcpkg add port sqlite3[core]

Now sets "default-featues": false

src/vcpkg/commands.add.cpp Show resolved Hide resolved
"dependencies": [
{
"name": "sqlite3",
"default-features": false,
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be clarified that this is still here from the previous run, but it would also be better to have an independent test with [core,somethingelse].

Copy link
Member

Choose a reason for hiding this comment

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

On second thought I think this should make default features be true because that's what sqlite3[zlib] requests.

@ras0219-msft also points out that we should have tests for multiple references to the same name

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 now have the test vcpkg add port "sqlite3" "sqlite3[core]"

@BillyONeal
Copy link
Member

@AugP @vicroms @dan-shaw @ras0219-msft @JavierMatosD and I discussed this today and it gets design approval modulo the nitpicks above

Copy link
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

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

I pushed a merge with main for you; do you want me to fix the behavior bit I pointed out for you here and merge?

azure-pipelines/end-to-end-tests-dir/add.ps1 Outdated Show resolved Hide resolved
@autoantwort
Copy link
Contributor Author

I pushed a merge with main for you; do you want me to fix the behavior bit I pointed out for you here and merge?

Sorry for the late answer. Yeah you could have done that 😅 I have not implemented this

@BillyONeal BillyONeal merged commit f07df71 into microsoft:main Oct 31, 2023
5 checks passed
@BillyONeal
Copy link
Member

Thanks!

@autoantwort autoantwort deleted the feature/add-port branch October 31, 2023 01:13
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