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

vcpkg: support builtin-baseline without git #329939

Merged
merged 2 commits into from
Aug 16, 2024

Conversation

gracicot
Copy link
Contributor

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 the vcpkg-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

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@@ -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"
Copy link
Contributor Author

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?

@ofborg ofborg bot requested review from Guekka and h7x4 July 25, 2024 17:11
@ofborg ofborg bot added 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 labels Jul 25, 2024
@@ -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"
Copy link
Contributor

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?

Copy link
Contributor Author

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:

I will add links to the relevant place in the source code right above the echo

Copy link
Contributor

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?

Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@gracicot

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

Copy link
Contributor Author

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

Copy link
Contributor Author

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

Copy link
Contributor

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 uses builtin.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.

Copy link
Contributor Author

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

@gracicot gracicot force-pushed the gracicot/vcpkg-bundle branch 2 times, most recently from 589bab5 to 244126c Compare July 26, 2024 13:24
@ofborg ofborg bot requested a review from Guekka July 26, 2024 14:01
@h7x4
Copy link
Member

h7x4 commented Jul 28, 2024

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

@gracicot
Copy link
Contributor Author

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

Copy link
Contributor

@Jayman2000 Jayman2000 left a comment

Choose a reason for hiding this comment

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

@h7x4

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 Show resolved Hide resolved
@@ -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"
Copy link
Contributor

Choose a reason for hiding this comment

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

@gracicot

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

@gracicot gracicot force-pushed the gracicot/vcpkg-bundle branch from 244126c to 033f635 Compare July 30, 2024 14:35
@gracicot gracicot marked this pull request as ready for review July 30, 2024 14:35
@gracicot gracicot force-pushed the gracicot/vcpkg-bundle branch 2 times, most recently from 1f6d32d to e7149c5 Compare August 13, 2024 19:55
Copy link
Contributor

@Jayman2000 Jayman2000 left a 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.

pkgs/by-name/vc/vcpkg/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/vc/vcpkg/package.nix Outdated Show resolved Hide resolved
@smancill
Copy link
Contributor

smancill commented Aug 14, 2024

Is this a blocker for #327830 / #328476 / #327790 ?

@gracicot
Copy link
Contributor Author

@smancill no, it's just that this PR took time away from me.

@gracicot gracicot force-pushed the gracicot/vcpkg-bundle branch from e7149c5 to 8844f15 Compare August 14, 2024 17:44
@gracicot
Copy link
Contributor Author

@smancill I rebased after the update merge

@gracicot gracicot force-pushed the gracicot/vcpkg-bundle branch from c4ffccf to e4c87ba Compare August 14, 2024 17:56
@gracicot gracicot force-pushed the gracicot/vcpkg-bundle branch from e4c87ba to 9443df5 Compare August 14, 2024 17:56
Copy link
Contributor

@Jayman2000 Jayman2000 left a 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.

@smancill smancill merged commit 0da03f4 into NixOS:master Aug 16, 2024
25 of 26 checks passed
@smancill
Copy link
Contributor

Merged because it worked for me on Darwin, and it also was already approved.

@smancill smancill mentioned this pull request Aug 16, 2024
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants