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

Make x-add-version not require changes be committed first. #1616

Merged
merged 8 commits into from
Mar 20, 2025

Conversation

BillyONeal
Copy link
Member

@BillyONeal BillyONeal commented Mar 18, 2025

I was trying to fix a bug / assumption that builtin_ports_dir is under vcpkg_root elsewhere and realized it might be easier to just fix this problem for good. Note that we no longer need to know where the .git is and the only real input is the target directory. Git finds the .git directory if necessary; we want it to do that because it respects e.g. GIT_CEILING_DIRECTORIES and friends.

@BillyONeal
Copy link
Member Author

image

In further testing I realize that we mishandle the chmod +x bit on Windows. I need to somehow incorporate that part from the existing git repo, possibly by starting with the existing repo

@BillyONeal BillyONeal marked this pull request as draft March 18, 2025 22:13
…ectory rather than the whole ports tree. Also extract some git ops to git.h.
@BillyONeal
Copy link
Member Author

Perf impact of not needing to SHA the whole ports directory as in 2cf695a :

image

…preserve chmod +x which doesn't exist on the real filesystem.
…ions test.

I made the registry database edits with 2025-03-13-7699e411ea11543de6abc0c7d5fd11cfe0039ae5 , so I know they aren't affected by my changes.
@BillyONeal BillyONeal marked this pull request as ready for review March 19, 2025 04:02
@BillyONeal
Copy link
Member Author

Perf impact of not needing to SHA the whole ports directory as in 2cf695a

The effect of this change is reverted; it might be worth bringing back in the form of using a smaller set of git add commands but I believe it would have required a lot more child process invocations, and given that the perf when reusing the index seems better anyway I decided it wasn't worth bringing back.

@BillyONeal BillyONeal requested a review from JavierMatosD March 19, 2025 04:06
@@ -2,14 +2,12 @@

Copy-Item -Recurse "$PSScriptRoot/../e2e-assets/ci-verify-versions-registry" "$TestingRoot/ci-verify-versions-registry"
git -C "$TestingRoot/ci-verify-versions-registry" @gitConfigOptions init
git -C "$TestingRoot/ci-verify-versions-registry" @gitConfigOptions add --chmod=+x 'ports/executable-bit/some-script.sh'
Copy link
Member Author

@BillyONeal BillyONeal Mar 19, 2025

Choose a reason for hiding this comment

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

This additional test was added with the change to reuse the existing index done in response to x-ci-verify-versions failing on the real github.com/microsoft/vcpkg. Because I was always making new trees that don't have the git-only-on-Windows-anyway +x bit, the new trees had different SHAs than our version database says, resulting in failures.

In the following message to the team I sent when I just discovered this problem I'm using git diff-tree on some of the openssl trees and ports\openssl\unix\configure shows up because it's got a +x.

(Sorry it's just a screenshot; now that I fixed the problem it'd be annoying to try to recreate the broken behavior and it isn't super important now that it is fixed and has a regression test anyway)

image

@BillyONeal BillyONeal force-pushed the add-version-no-commit branch 7 times, most recently from e853eef to 7fd1cd7 Compare March 19, 2025 19:30
@BillyONeal BillyONeal force-pushed the add-version-no-commit branch from 7fd1cd7 to 3b8092d Compare March 19, 2025 19:43
Copy link
Member Author

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

Copy link
Member

@vicroms vicroms left a comment

Choose a reason for hiding this comment

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

LGTM. Some comments regarding the error messages, but not blocking over this.

@BillyONeal BillyONeal enabled auto-merge (squash) March 20, 2025 19:35
@BillyONeal BillyONeal merged commit b38768e into microsoft:main Mar 20, 2025
6 checks passed
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.

3 participants