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

Fix and localize x-add-version messages #489

Merged
merged 9 commits into from
Apr 20, 2022

Conversation

vicroms
Copy link
Member

@vicroms vicroms commented Apr 6, 2022

This PR adds the following behavior changes:

  • Print error message when given a non-existing port name
  • Print a warning when there are uncommitted changes for a port
  • Print a message when no files were updated at all
  • Localize x-add-version messages

Copy link
Contributor

@strega-nil-ms strega-nil-ms left a comment

Choose a reason for hiding this comment

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

minor nit

src/vcpkg/commands.add-version.cpp Outdated Show resolved Hide resolved
@strega-nil-ms
Copy link
Contributor

strega-nil-ms commented Apr 7, 2022

Example messages:

PS C:\src\vcpkg> &"$env:HOMEPATH/projects/vcpkg-tool/build/vcpkg" x-add-version dbghelp
No files were updated for dbghelp
PS C:\src\vcpkg> &"$env:HOMEPATH/projects/vcpkg-tool/build/vcpkg" x-add-version dbghelp # modify portfile, don't modify vcpkg.json
No files were updated for dbghelp
PS C:\src\vcpkg> &"$env:HOMEPATH/projects/vcpkg-tool/build/vcpkg" x-add-version dbghelp # modify version
warning: local port files SHA is the same as version 0#2 in C:/src/vcpkg/versions/d-/dbghelp.json
-- SHA: c90a3ad39f6dc1e0558acf89de26f220d0fd0c1f
-- Did you remember to commit your changes?
***No files were updated***
PS C:\src\vcpkg> &"$env:HOMEPATH/projects/vcpkg-tool/build/vcpkg" x-add-version dbghelp
error: local changes detected for dbghelp but no changes to version or port version
version: 0#2
old SHA: c90a3ad39f6dc1e0558acf89de26f220d0fd0c1f
new SHA: 5ef14d2fb94b9e30b91e74a77c4808f7eeddb771
Did you remember to update the version or port version?
***No files were updated***
PS C:\src\vcpkg> &"$env:HOMEPATH/projects/vcpkg-tool/build/vcpkg" x-add-version dbghelp # vcpkg.json incorrect
error: couldn't load port dbghelp
PS C:\src\vcpkg> &"$env:HOMEPATH/projects/vcpkg-tool/build/vcpkg" x-add-version dbghelpasdf # non-existent port
error: port dbghelpasdf does not exist
PS C:\src\vcpkg> &"$env:HOMEPATH/projects/vcpkg-tool/build/vcpkg" x-add-version dbghelp # update version (no message)
PS C:\src\vcpkg> &"$env:HOMEPATH/projects/vcpkg-tool/build/vcpkg" x-add-version dbghelp --overwrite-version # update without updating version (no message)

Some comments:

  • I don't love that we still don't print anything in the success case
  • I appreciate that the non-existent port message != vcpkg.json incorrect now
  • I really don't like that we don't print the actual error for parsing an incorrect vcpkg.json
  • I don't see where print_success is used? it seems to be called, but I don't know when it's supposed to print?
    • I also don't really like print_success

src/vcpkg/commands.add-version.cpp Outdated Show resolved Hide resolved
src/vcpkg/commands.add-version.cpp Outdated Show resolved Hide resolved
src/vcpkg/commands.add-version.cpp Outdated Show resolved Hide resolved
src/vcpkg/commands.add-version.cpp Outdated Show resolved Hide resolved
src/vcpkg/commands.add-version.cpp Outdated Show resolved Hide resolved
src/vcpkg/commands.add-version.cpp Outdated Show resolved Hide resolved
@vicroms vicroms merged commit 8b3f164 into microsoft:main Apr 20, 2022
@autoantwort
Copy link
Contributor

This change resulted in a huge performance regression. vcpkg x-add-version --all now needs 25.5 seconds instead of 0.7 seconds on my machine. So it is now 36 times slower.

@BillyONeal
Copy link
Member

Hmmm... probably because we're now doing git status a zillion times?

@autoantwort
Copy link
Contributor

Probably. Running it > 1900 times is not the fastest. Maybe disable this additional check when passing --all

@ras0219-msft
Copy link
Contributor

I think the fix is to run status a single time on the whole "ports" directory, then use that single output to judge each port.

@vicroms
Copy link
Member Author

vicroms commented Apr 25, 2022

Should be fixed in #526

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.

5 participants