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

x-add-version: add option to automatically commit result #94

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

autoantwort
Copy link
Contributor

@autoantwort autoantwort commented Jun 15, 2021

No description provided.

@autoantwort autoantwort changed the title x-addversion: add option to automatically commit result x-add-version: add option to automatically commit result Jun 15, 2021
@autoantwort autoantwort force-pushed the x-add-version-commit-result branch from d25433c to 24aac98 Compare June 15, 2021 00:35
@autoantwort autoantwort force-pushed the x-add-version-commit-result branch from 24aac98 to fb34ea7 Compare July 2, 2021 21:38
@strega-nil-ms strega-nil-ms added the depends:different-pr This PR depends on a different PR which has been filed label Jul 6, 2021
@autoantwort autoantwort force-pushed the x-add-version-commit-result branch from fb34ea7 to f20a2a0 Compare September 13, 2021 21:28
@autoantwort autoantwort force-pushed the x-add-version-commit-result branch from f20a2a0 to b8018c0 Compare October 21, 2021 13:29
@autoantwort
Copy link
Contributor Author

@BillyONeal can you review the code for Filesystem::relative? :)

@autoantwort autoantwort force-pushed the x-add-version-commit-result branch from b8018c0 to b9baa13 Compare November 7, 2021 03:53
# Conflicts:
#	include/vcpkg/vcpkgpaths.h
#	src/vcpkg/vcpkgpaths.cpp
@BillyONeal
Copy link
Member

@BillyONeal can you review the code for Filesystem::relative? :)

Sorry I missed this message before. I generally don't think code review is very effective at finding bugs in unit level tests like this; I would prefer to see lexically_relative extracted with unit tests added, then change this function to one that combines with almost_canonical in a way where it need not be unit tested because there are no "interesting conditions", if that makes sense?

@BillyONeal BillyONeal added requires:author-response and removed depends:different-pr This PR depends on a different PR which has been filed labels Mar 31, 2022
@autoantwort
Copy link
Contributor Author

Should lexically_relative check if the given paths are absolute? Or is this simply undefined behavior?

# Conflicts:
#	include/vcpkg/base/util.h
#	src/vcpkg/commands.add-version.cpp
# Conflicts:
#	src/vcpkg/commands.add-version.cpp
#	src/vcpkg/vcpkgpaths.cpp
src/vcpkg/commands.add-version.cpp Outdated Show resolved Hide resolved
src/vcpkg/commands.add-version.cpp Outdated Show resolved Hide resolved
@BillyONeal
Copy link
Member

Should lexically_relative check if the given paths are absolute? Or is this simply undefined behavior?

The standard says that if absoluteness is different or similar that the result is path() (that is, an empty path). https://en.cppreference.com/w/cpp/filesystem/path/lexically_normal

@autoantwort
Copy link
Contributor Author

Hm I don't want to reimplement the function lexically_relative with standard semantics. Especially when the only reason is that we want to run the tests on windows.

@BillyONeal
Copy link
Member

Hm I don't want to reimplement the function lexically_relative with standard semantics. Especially when the only reason is that we want to run the tests on windows.

It's OK to implement your own function but if we are naming the function the same name as the standard I think it should do what the standard says unless we can't do that for some reason. (There's a notable exception with some of the copy/copy_file family enums, but in that case we just didn't implement some of the cases we didn't need, we didn't implement something different than what the standard said)

# Conflicts:
#	include/vcpkg/base/messages.h
#	locales/messages.en.json
#	locales/messages.json
#	src/vcpkg/commands.add-version.cpp
#	src/vcpkg/vcpkgpaths.cpp
@BillyONeal
Copy link
Member

I think this PR has been in a holding pattern because

  1. We don't have a great understanding of what we want registry manipulation commands to be / do. This is part of why x-add-version still has the x-. I'm hoping to address this (as we also discussed in vcpkg command port-name should be consistent across commands vcpkg#18967 ) as part of going into Visual Studio slated to be ready in March.
  2. We aren't sure we want to be creating commits. It's "out of character" for us to make 'durable' changes like that in any automated fashion.

If you're still interested in investigating here before we can give you a concrete answer, I can say something that would be uncontroversial would be fixing x-add-version to work with uncommitted changes. (By setting GIT_INDEX_FILE to a throwaway index, 'committing' into that, and using that to figure out what the tree SHA would be)

Sorry you've been in a holding pattern for so long

@autoantwort
Copy link
Contributor Author

autoantwort commented Nov 28, 2022

I thought it would be helpful because you currently always have to do a

  1. git add versions
  2. git commit --amend --no-edit or git commit -m "Add versions"

after running vcpkg x-add-versions no matter what. So it seemed logical to me to automate this.

@JavierMatosD JavierMatosD added the requires:vcpkg-team-review This PR or issue requires someone from the vcpkg team to take a further look. label Oct 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
depends:registry-editing requires:vcpkg-team-review This PR or issue requires someone from the vcpkg team to take a further look.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants