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.

Sign up for GitHub

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

Jump to bottom

Git object hashing in libutil #9294

Merged
merged 3 commits into from
Nov 10, 2023
Merged

Git object hashing in libutil #9294

merged 3 commits into from
Nov 10, 2023

Conversation

Ericson2314
Copy link
Member

Motivation

This is the core functionality but just unit-tested and not yet made part of the store layer.

Context

The integration is not yet done because there is some tech debt around (a) repeated boilerplate hashing objects (b) better integration of the new SourceAccessor type that needs to be cleaned up first.

depends on #9283

Priorities

Add 👍 to pull requests you find important.

@Ericson2314 Ericson2314 requested a review from edolstra as a code owner November 4, 2023 19:42
@github-actions github-actions bot added with-tests Issues related to testing. PRs with tests have some priority fetching Networking with the outside (non-Nix) world, input locking labels Nov 4, 2023
@Ericson2314 Ericson2314 force-pushed the minimal-git branch 2 times, most recently from 02a54e0 to c837e61 Compare November 4, 2023 20:04
This was referenced Nov 4, 2023
Merged
Open
@Ericson2314 Ericson2314 added the RFC Related to an accepted RFC label Nov 6, 2023
@dpulls dpulls
Copy link

dpulls bot commented Nov 6, 2023

🎉 All dependencies have been resolved !

@Ericson2314 Ericson2314 changed the base branch from master to 2.18-maintenance November 6, 2023 18:52
@Ericson2314 Ericson2314 changed the base branch from 2.18-maintenance to master November 6, 2023 18:52
@github-actions github-actions bot added documentation new-cli Relating to the "nix" command store Issues and pull requests concerning the Nix store repl The Read Eval Print Loop, "nix repl" command and debugger and removed documentation new-cli Relating to the "nix" command store Issues and pull requests concerning the Nix store repl The Read Eval Print Loop, "nix repl" command and debugger fetching Networking with the outside (non-Nix) world, input locking labels Nov 6, 2023
roberth
roberth reviewed Nov 6, 2023
View reviewed changes
Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

Haven't found an issue, other than that it would be nice to see that the .bin files correspond to what the real git produces. Perhaps a shell script that produces the same tree using the git command, and then compares that tree hash to a hash of a .bin file? (or did I overlook something?)

@Ericson2314
Copy link
Member Author

@roberth I have the additional now yay, but ugh feel weird adding unit-test-data to the fileset whitelist for "functional tests only" that feels weird (which would fix the CI failure). Have any ideas?

@roberth
Copy link
Member

roberth commented Nov 7, 2023

feels weird

In terms of its purpose it's not part of the functional test suite, because it's testing the unit tests, not Nix.
However, from a practical point of view it doesn't matter much. I'm ok with not creating a new checks attribute just for this little test. We can split it out if/when we have more examples of this.

@Ericson2314
Copy link
Member Author

Oh actually an ad-hoc one-off

check:
    $(bash) ./script

seems to like the right way to solve this problem. Then no changes to the file filters are needed.

Ericson2314
Ericson2314 commented Nov 8, 2023
View reviewed changes
mk/common-test.sh
init_test () {
cd "$test_dir" && env "${TESTS_ENVIRONMENT[@]}" $BASH -e init.sh 2>/dev/null > /dev/null
Copy link
Member Author

Choose a reason for hiding this comment

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

Old init.sh layer violation

Ericson2314
Ericson2314 commented Nov 8, 2023
View reviewed changes
mk/lib.mk
@@ -122,14 +122,15 @@ $(foreach script, $(bin-scripts), $(eval $(call install-program-in,$(script),$(b
$(foreach script, $(bin-scripts), $(eval programs-list += $(script)))
$(foreach script, $(noinst-scripts), $(eval programs-list += $(script)))
$(foreach template, $(template-files), $(eval $(call instantiate-template,$(template))))
install_test_init=tests/functional/init.sh
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a layer violation, but it is not a new layer violation. mk previously also new about init.sh. The violation is in fact "shallower" (closer to the entry point) then before. That seems like an improvement to me.

@Ericson2314
Copy link
Member Author

Ericson2314 commented Nov 8, 2023 •
edited
Loading

I like the way having this bash test in make check forced me to think about layering in the functional test suite. I hope that gets us one small bit closer to fixing other issues with it --- e.g. experimental with different ways of initialization/setup.

roberth
roberth reviewed Nov 10, 2023
View reviewed changes
Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

LGTM after suggestions

mk/common-test.sh Outdated Show resolved Hide resolved
mk/run-test.sh Outdated Show resolved Hide resolved
Ericson2314 and others added 3 commits November 10, 2023 11:02
@Ericson2314
Create MemorySink
3d9d5dc 
This is for writing to a `MemorySourceAccessor`.
@Ericson2314 @roberth
Refactor bash test build system a bit
9afa697 
The basic idea here is to separate a few intertwined notions:

1. Not all "run bash tests" are "install tests"

2. Not all "run bash tests" use `tests/functional/init.sh`, or any
   pre-test initialization at all.

This will used in the next commit when we have a test that check unit
test golden master data.

Also, move our custom `PS4` from the test to the test runner, as it is
part of how we want to display the tests, not the test themselves.

Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>
@Ericson2314 @matthewbauer
@meditans @roberth @flokli
Git object hashing in libutil
20b95d6 
This is the core functionality but just unit-tested and not yet made
part of the store layer. This is because there is some tech debt around
(a) repeated boilerplate hashing objects (b) better integration of the
new `SourceAccessor` type that needs to be cleaned up first.

Part of RFC 133

Co-Authored-By: Matthew Bauer <mjbauer95@gmail.com>
Co-Authored-By: Carlo Nucera <carlo.nucera@protonmail.com>
Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>
Co-authored-by: Florian Klink <flokli@flokli.de>
@Ericson2314 Ericson2314 merged commit 458e511 into NixOS:master Nov 10, 2023
7 checks passed
@Ericson2314 Ericson2314 deleted the minimal-git branch November 10, 2023 16:33
@Ericson2314 Ericson2314 added this to the git-hashing stabilisation milestone Mar 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roberth roberth roberth approved these changes

@edolstra edolstra Awaiting requested review from edolstra edolstra is a code owner

@thufschmitt thufschmitt Awaiting requested review from thufschmitt

@fricklerhandwerk fricklerhandwerk Awaiting requested review from fricklerhandwerk fricklerhandwerk is a code owner

Assignees
No one assigned
Labels
RFC Related to an accepted RFC with-tests Issues related to testing. PRs with tests have some priority
Projects
None yet
Milestone
git-hashing stabilisation
Development

Successfully merging this pull request may close these issues.
2 participants
@Ericson2314 @roberth