Skip to content

Comments

Git object hashing in libutil#9294

Merged
Ericson2314 merged 3 commits intoNixOS:masterfrom
obsidiansystems:minimal-git
Nov 10, 2023
Merged

Git object hashing in libutil#9294
Ericson2314 merged 3 commits intoNixOS:masterfrom
obsidiansystems:minimal-git

Conversation

@Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Nov 4, 2023

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
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
Ericson2314
Ericson2314 commented Nov 6, 2023
View reviewed changes
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

Ericson2314 commented Nov 7, 2023

@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

Ericson2314 commented Nov 7, 2023

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

@Ericson2314 Ericson2314 Nov 8, 2023

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
$(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

@Ericson2314 Ericson2314 Nov 8, 2023

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

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 <[email protected]>
@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 <[email protected]>
Co-Authored-By: Carlo Nucera <[email protected]>
Co-authored-by: Robert Hensing <[email protected]>
Co-authored-by: Florian Klink <[email protected]>
@Ericson2314 Ericson2314 merged commit 458e511 into NixOS:master Nov 10, 2023
@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

@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

1 more reviewer

@roberth roberth roberth approved these changes

Reviewers whose approvals may not affect merge requirements

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