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

ci: fix the FreeBSD build #1308

Closed
wants to merge 2 commits into from
Closed

ci: fix the FreeBSD build #1308

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
501a89a
t5351: avoid relying on `core.fsyncMethod = batch` to be supported
dscho Jul 29, 2022
b9203ea
t5351: avoid using `test_cmp` for binary data
dscho Jul 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions bulk-checkin.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,8 @@ void fsync_loose_object_bulk_checkin(int fd, const char *filename)
*/
Copy link

Choose a reason for hiding this comment

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

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> On FreeBSD, this mode is not supported. But since 3a251bac0d1a (trace2:
> only include "fsync" events if we git_fsync(), 2022-07-18) t5351 will
> fail if this mode is unsupported.
>
> Let's address this in the minimal fashion, by detecting that that mode
> is unsupported and expecting a different count of hardware flushes in
> that case.
>
> This fixes the CI/PR builds on FreeBSD again.
>
> Note: A better way would be to test only what is relevant in t5351.6
> "unpack big object in stream (core.fsyncmethod=batch)" again instead of
> blindly comparing the output against some exact text. But that would
> pretty much revert the idea of above-mentioned commit, and that commit
> was _just_ accepted into Git's main branch so one must assume that it
> was intentional.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  bulk-checkin.c                  |  2 ++
>  t/t5351-unpack-large-objects.sh | 10 ++++++++--
>  2 files changed, 10 insertions(+), 2 deletions(-)

I am inclined to take this as-is for now.

But it inherits from 3a251bac0d1a the general "we should be able to
count the value" expectation, which may not be the best approach to
run this test; asking Acks from those originally involved in this
area and possibly ideas to test this in a more robust way.

Thanks.


> diff --git a/bulk-checkin.c b/bulk-checkin.c
> index 98ec8938424..855b68ec23b 100644
> --- a/bulk-checkin.c
> +++ b/bulk-checkin.c
> @@ -340,6 +340,8 @@ void fsync_loose_object_bulk_checkin(int fd, const char *filename)
>  	 */
>  	if (!bulk_fsync_objdir ||
>  	    git_fsync(fd, FSYNC_WRITEOUT_ONLY) < 0) {
> +		if (errno == ENOSYS)
> +			warning(_("core.fsyncMethod = batch is unsupported on this platform"));
>  		fsync_or_die(fd, filename);
>  	}
>  }
> diff --git a/t/t5351-unpack-large-objects.sh b/t/t5351-unpack-large-objects.sh
> index f785cb06173..dd7ebc40072 100755
> --- a/t/t5351-unpack-large-objects.sh
> +++ b/t/t5351-unpack-large-objects.sh
> @@ -70,9 +70,15 @@ test_expect_success 'unpack big object in stream (core.fsyncmethod=batch)' '
>  	GIT_TRACE2_EVENT="$(pwd)/trace2.txt" \
>  	GIT_TEST_FSYNC=true \
>  		git -C dest.git $BATCH_CONFIGURATION unpack-objects <pack-$PACK.pack &&
> -	check_fsync_events trace2.txt <<-\EOF &&
> +	if grep "core.fsyncMethod = batch is unsupported" trace2.txt
> +	then
> +		flush_count=7
> +	else
> +		flush_count=1
> +	fi &&
> +	check_fsync_events trace2.txt <<-EOF &&
>  	"key":"fsync/writeout-only","value":"6"
> -	"key":"fsync/hardware-flush","value":"1"
> +	"key":"fsync/hardware-flush","value":"$flush_count"
>  	EOF
>  
>  	test_dir_is_empty dest.git/objects/pack &&

Copy link

Choose a reason for hiding this comment

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

On the Git mailing list, "brian m. carlson" wrote (reply to this):

On 2022-07-29 at 16:07:46, Junio C Hamano wrote:
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
> 
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > On FreeBSD, this mode is not supported. But since 3a251bac0d1a (trace2:
> > only include "fsync" events if we git_fsync(), 2022-07-18) t5351 will
> > fail if this mode is unsupported.
> >
> > Let's address this in the minimal fashion, by detecting that that mode
> > is unsupported and expecting a different count of hardware flushes in
> > that case.
> >
> > This fixes the CI/PR builds on FreeBSD again.
> >
> > Note: A better way would be to test only what is relevant in t5351.6
> > "unpack big object in stream (core.fsyncmethod=batch)" again instead of
> > blindly comparing the output against some exact text. But that would
> > pretty much revert the idea of above-mentioned commit, and that commit
> > was _just_ accepted into Git's main branch so one must assume that it
> > was intentional.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  bulk-checkin.c                  |  2 ++
> >  t/t5351-unpack-large-objects.sh | 10 ++++++++--
> >  2 files changed, 10 insertions(+), 2 deletions(-)
> 
> I am inclined to take this as-is for now.

I think this patch is a strict improvement over the status quo, despite
what I'm going to mention below.

> But it inherits from 3a251bac0d1a the general "we should be able to
> count the value" expectation, which may not be the best approach to
> run this test; asking Acks from those originally involved in this
> area and possibly ideas to test this in a more robust way.

While it may not matter in this test, I noticed that the metrics we use
need not be accurate in multi-threaded programs.  We use integers of
static intmax_t which isn't necessarily atomic across threads.  Even if
we assumed word-sized increments were atomic[0], this type might be 64
bits on a 32-bit system.

I am aware that we don't use threads in many places, but in general we
should be safely able to assume that we can perform an fsync on any
thread without thread safety problems because that's the assumption a
reasonable Unix programmer would make.  Even if it's not a problem now,
it could well be in the future, and we should either fix this (say, with
C11 atomic integers[1]) or put a note on the metrics functions that they
may be wrong and stop using them in tests.

I would, in general, prefer that if we add wrappers that wrap common
Unix functions we ensure that they provide the same guarantees as the
common Unix functions we're wrapping.  I realize I may sound fussy, but
I've been fixing giant thread-safety problems recently and it's not fun.

[0] This is not, in general, a safe assumption to make, since most RISC
architectures are load-store.
[1] This would necessitate moving to C11, which is fine with me (and
already required on Windows), but others may have objections to.
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

if (!bulk_fsync_objdir ||
git_fsync(fd, FSYNC_WRITEOUT_ONLY) < 0) {
if (errno == ENOSYS)
warning(_("core.fsyncMethod = batch is unsupported on this platform"));
fsync_or_die(fd, filename);
}
}
Expand Down
12 changes: 9 additions & 3 deletions t/t5351-unpack-large-objects.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,9 +70,15 @@ test_expect_success 'unpack big object in stream (core.fsyncmethod=batch)' '
GIT_TRACE2_EVENT="$(pwd)/trace2.txt" \
GIT_TEST_FSYNC=true \
git -C dest.git $BATCH_CONFIGURATION unpack-objects <pack-$PACK.pack &&
check_fsync_events trace2.txt <<-\EOF &&
if grep "core.fsyncMethod = batch is unsupported" trace2.txt
then
flush_count=7
else
flush_count=1
fi &&
check_fsync_events trace2.txt <<-EOF &&
"key":"fsync/writeout-only","value":"6"
"key":"fsync/hardware-flush","value":"1"
"key":"fsync/hardware-flush","value":"$flush_count"
EOF
test_dir_is_empty dest.git/objects/pack &&
Expand All @@ -87,7 +93,7 @@ test_expect_success 'do not unpack existing large objects' '
# The destination came up with the exact same pack...
DEST_PACK=$(echo dest.git/objects/pack/pack-*.pack) &&
test_cmp pack-$PACK.pack $DEST_PACK &&
cmp pack-$PACK.pack $DEST_PACK &&
# ...and wrote no loose objects
test_stdout_line_count = 0 find dest.git/objects -type f ! -name "pack-*"
Expand Down