-
-
Notifications
You must be signed in to change notification settings - Fork 17.2k
vcpkg: support builtin-baseline without git #329939
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
Conversation
pkgs/by-name/vc/vcpkg/package.nix
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to fetch the git hash of the current version automatically?
pkgs/by-name/vc/vcpkg/package.nix
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add an explanation or link to documentation of vcpkg-bundle.json?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When asking Billy O'Neal about documentation for that file, he answered me that there's no documentation for it. Given that this file is intended to be written by those who ship vcpkg, and that the only official distributions of vcpkg right now is by microsoff, I could see why they haven't taken the time to document it.
He gave me a link of those two source file as help for implementing this PR though:
- https://github.com/microsoft/vcpkg-tool/blob/f098d3e0aaa7e46ea84a1f7079586e1ec5af8ab5/vcpkg-init/mint-standalone-bundle.ps1#L21
- https://github.com/microsoft/vcpkg-tool/blob/f098d3e0aaa7e46ea84a1f7079586e1ec5af8ab5/src/vcpkg/bundlesettings.cpp#L87
I will add links to the relevant place in the source code right above the echo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand what the sha represents. Is it the sha of vcpkg.exe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I found it https://github.com/microsoft/vcpkg-tool/blob/d272c0d4f5175b26bd56c6109d4c4935b791a157/src/vcpkg/commands.new.cpp#L190
So it's the sha of the current vcpkg library, if I understand correctly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sha of the current registry, which is simply the current git hash of the vcpkg repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nix can't do
src.currentGitHashor something like that, right?
It looks like it can:
diff --git a/pkgs/by-name/vc/vcpkg/package.nix b/pkgs/by-name/vc/vcpkg/package.nix
index b9e51af8bc8b..8e1a668139f1 100644
--- a/pkgs/by-name/vc/vcpkg/package.nix
+++ b/pkgs/by-name/vc/vcpkg/package.nix
@@ -1,5 +1,4 @@
-{ fetchFromGitHub
-, stdenvNoCC
+{ stdenvNoCC
, lib
, vcpkg-tool
, makeWrapper
@@ -10,11 +9,9 @@ stdenvNoCC.mkDerivation (finalAttrs: {
pname = "vcpkg";
version = "2024.06.15";
- src = fetchFromGitHub {
- owner = "microsoft";
- repo = "vcpkg";
- rev = finalAttrs.version;
- hash = "sha256-eDpMGDtC44eh0elLWV0r1H/WbpVdZ5qMedKh7Ct50Cs=";
+ src = builtins.fetchGit {
+ url = "https://github.com/microsoft/vcpkg";
+ ref = "refs/tags/${finalAttrs.version}";
};
nativeBuildInputs = [ makeWrapper ];
@@ -50,7 +47,7 @@ stdenvNoCC.mkDerivation (finalAttrs: {
# will checkout a remote instead of trying to do git operation in the vcpkg root
# https://github.com/microsoft/vcpkg-tool/blob/d272c0d4f5175b26bd56c6109d4c4935b791a157/src/vcpkg/vcpkgpaths.cpp#L920
# https://github.com/microsoft/vcpkg-tool/blob/d272c0d4f5175b26bd56c6109d4c4935b791a157/src/vcpkg/configuration.cpp#L718
- echo '{ "readonly": true, "usegitregistry": true, "deployment": "Git", "embeddedsha": "f7423ee180c4b7f40d43402c2feb3859161ef625" }' > "$out/share/vcpkg/vcpkg-bundle.json"
+ echo '{ "readonly": true, "usegitregistry": true, "deployment": "Git", "embeddedsha": "${finalAttrs.src.rev}" }' > "$out/share/vcpkg/vcpkg-bundle.json"
touch "$out/share/vcpkg/vcpkg.disable-metrics"
runHook postInstallThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw that leaveDotGit uses builtin.fetchGit internally. I changed it to use git to get the value of embeddedsha
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Jayman2000 Let me know if you prefer using fetchGit directly instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Jayman2000 Will the script that generate the automatic update still work if we switch to
builtin.fetchGit? Such as these: #327790
I don’t really know. Pull requests like that one are generated by nixpkgs-update. Normally, I would just run nixpkgs-update on my local system in order to see if it works, but I can’t because of nix-community/nixpkgs-update#335. That being said, there is another tool with a confusingly similar name: nix-update. As you saw in NixOS/nix#11222, nix-update doesn’t work if you use builtins.fetchGit. If nix-update doesn’t work, then I doubt that nixpkgs-update will work.
I saw that
leaveDotGitusesbuiltin.fetchGitinternally. I changed it to use git to get the value of embeddedsha@Jayman2000 Let me know if you prefer using fetchGit directly instead
Here’s my concern: if we use leaveDotGit, then won’t we run into #8567 again?
Here’s a potential fixup for this PR. I kept leaveDotGit = true, but I delete the .git folder before the fetcher calculates the hash. Hopefully, that will prevent the hash from changing under our feet without breaking nixpkgs-update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Jayman2000 I applied the fixup, seems to work great
589bab5 to
244126c
Compare
|
@gracicot: Do you know how well this solution fits with any future plans to build projects with manifests and external repositories as nix derivations? If you're unsure I'll do some research of my own, but I was hoping you had some more insider knowledge :) Also, @Jayman2000: do you have any input on this solution? I'm guessing you've tested the solution from #328937 on a project of some sort since you opened two consecutive PRs for vcpkg earlier. Does this break anything for you? |
|
@h7x4 sadly my only "insider knowledge" I have is the few comments I got from their lead developer, and the few conversations I participated in their repo. For projects that have a manifest, I guess we could try building derivations from libraries packaged by vcpkg using vcpkg ports, then making cmake aware of the install prefix. I don't think this patch impede with this future. On the contrary, I think it's gonna help. We could also try to follow what cargo does. I'm not exactly sure how it works, but I'm sure there's a way to make it just as convenient. |
Jayman2000
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, @Jayman2000: do you have any input on this solution?
I think that this solution is better than the one from #328937.
I'm guessing you've tested the solution from #328937 on a project of some sort since you opened two consecutive PRs for vcpkg earlier. Does this break anything for you?
I started submitting vcpkg PRs because I was having trouble testing out DescentDevelopers/Descent3#469 on NixOS. I just merged the libstdc++ PR (#329033) and this PR into Nixpkg’s master branch locally so that I could try building Descent 3 using vcpkg again. I was able to build Descent 3 using vcpkg successfully.
pkgs/by-name/vc/vcpkg/package.nix
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nix can't do
src.currentGitHashor something like that, right?
It looks like it can:
diff --git a/pkgs/by-name/vc/vcpkg/package.nix b/pkgs/by-name/vc/vcpkg/package.nix
index b9e51af8bc8b..8e1a668139f1 100644
--- a/pkgs/by-name/vc/vcpkg/package.nix
+++ b/pkgs/by-name/vc/vcpkg/package.nix
@@ -1,5 +1,4 @@
-{ fetchFromGitHub
-, stdenvNoCC
+{ stdenvNoCC
, lib
, vcpkg-tool
, makeWrapper
@@ -10,11 +9,9 @@ stdenvNoCC.mkDerivation (finalAttrs: {
pname = "vcpkg";
version = "2024.06.15";
- src = fetchFromGitHub {
- owner = "microsoft";
- repo = "vcpkg";
- rev = finalAttrs.version;
- hash = "sha256-eDpMGDtC44eh0elLWV0r1H/WbpVdZ5qMedKh7Ct50Cs=";
+ src = builtins.fetchGit {
+ url = "https://github.com/microsoft/vcpkg";
+ ref = "refs/tags/${finalAttrs.version}";
};
nativeBuildInputs = [ makeWrapper ];
@@ -50,7 +47,7 @@ stdenvNoCC.mkDerivation (finalAttrs: {
# will checkout a remote instead of trying to do git operation in the vcpkg root
# https://github.com/microsoft/vcpkg-tool/blob/d272c0d4f5175b26bd56c6109d4c4935b791a157/src/vcpkg/vcpkgpaths.cpp#L920
# https://github.com/microsoft/vcpkg-tool/blob/d272c0d4f5175b26bd56c6109d4c4935b791a157/src/vcpkg/configuration.cpp#L718
- echo '{ "readonly": true, "usegitregistry": true, "deployment": "Git", "embeddedsha": "f7423ee180c4b7f40d43402c2feb3859161ef625" }' > "$out/share/vcpkg/vcpkg-bundle.json"
+ echo '{ "readonly": true, "usegitregistry": true, "deployment": "Git", "embeddedsha": "${finalAttrs.src.rev}" }' > "$out/share/vcpkg/vcpkg-bundle.json"
touch "$out/share/vcpkg/vcpkg.disable-metrics"
runHook postInstall244126c to
033f635
Compare
1f6d32d to
e7149c5
Compare
Jayman2000
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested this PR locally, and it worked.
|
@smancill no, it's just that this PR took time away from me. |
e7149c5 to
8844f15
Compare
|
@smancill I rebased after the update merge |
c4ffccf to
e4c87ba
Compare
e4c87ba to
9443df5
Compare
Jayman2000
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just tested this pull request on NixOS 24.05, and it worked. Specifically, I tried running vcpkg install with this package.json file…
{
"dependencies": [ "fmt", "zlib" ],
"builtin-baseline": "101ae1f63a22d262f80a68034a1550da5dfdd05f"
}…and it completed successfully.
|
Merged because it worked for me on Darwin, and it also was already approved. |
Description of changes
This change is another take on #328937.
This PR is taking a different approach. Instead of adding the git directory in the install folder, I use the
vcpkg-bundle.jsonfile. That file can instruct vcpkg of the current git revision without shipping the actual git history in the repo. It can also instruct vcpkg that the filesystem is immutable, so it won't attempt to fetch a different revisions.This as for effect of supporting arbitrary baselines, since vcpkg will use the mutable tree instead as it's download location instead of its own git root.
There was one problem implementing this PR is that vcpkg expects to read
vcpkg-bundle.jsonas a sibling file to the executable file. Since the vcpkg root is using a wrapper script, it tries to read at the wrong location. I don't think we can move thevcpkg-bundle.jsonfile to vcpkg-tool, since it contains the git revision of vcpkg. I instead opted to patch vcpkg to read the bundle json file from the root directory instead.Bonus, it fixes the disable metrics, which suffered from the same problem as the bundle file.
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.