-
-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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: support builtin-baseline without git #329939
Conversation
pkgs/by-name/vc/vcpkg/package.nix
Outdated
@@ -41,6 +41,7 @@ stdenvNoCC.mkDerivation (finalAttrs: { | |||
''} | |||
|
|||
ln -s "$out/bin/vcpkg" "$out/share/vcpkg/vcpkg" | |||
echo '{ "readonly": true, "usegitregistry": true, "deployment": "Git", "embeddedsha": "f7423ee180c4b7f40d43402c2feb3859161ef625" }' > "$out/share/vcpkg/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.
Is there a way to fetch the git hash of the current version automatically?
pkgs/by-name/vc/vcpkg/package.nix
Outdated
@@ -41,6 +41,7 @@ stdenvNoCC.mkDerivation (finalAttrs: { | |||
''} | |||
|
|||
ln -s "$out/bin/vcpkg" "$out/share/vcpkg/vcpkg" | |||
echo '{ "readonly": true, "usegitregistry": true, "deployment": "Git", "embeddedsha": "f7423ee180c4b7f40d43402c2feb3859161ef625" }' > "$out/share/vcpkg/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.
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.currentGitHash
or 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 postInstall
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 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
leaveDotGit
usesbuiltin.fetchGit
internally. 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. |
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
@@ -41,6 +41,7 @@ stdenvNoCC.mkDerivation (finalAttrs: { | |||
''} | |||
|
|||
ln -s "$out/bin/vcpkg" "$out/share/vcpkg/vcpkg" | |||
echo '{ "readonly": true, "usegitregistry": true, "deployment": "Git", "embeddedsha": "f7423ee180c4b7f40d43402c2feb3859161ef625" }' > "$out/share/vcpkg/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.
Nix can't do
src.currentGitHash
or 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 postInstall
244126c
to
033f635
Compare
1f6d32d
to
e7149c5
Compare
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
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.json
file. 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.json
as 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.json
file 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 = relaxed
sandbox = true
nix-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.