{"thread":{"id":"58241","subject":"[PATCH 0/2] ci: fix the FreeBSD build","startedAt":"2022-07-29T12:28:51Z","lastAt":"2022-07-29T21:25:03Z","messageCount":8,"participants":["Johannes Schindelin via GitGitGadget","Derrick Stolee","Junio C Hamano","Carlo Marcelo Arenas Belón","brian m. carlson"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"460205","messageId":"pull.1308.git.1659097724.gitgitgadget@gmail.com","threadId":"58241","inReplyTo":null,"subject":"[PATCH 0/2] ci: fix the FreeBSD build","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-07-29T12:28:42Z","receivedAt":"2022-07-29T12:28:51Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Since 3a251bac0d1a (trace2: only include \"fsync\" events if we git_fsync(),\n2022-07-18), the FreeBSD builds are failing in t5351.6. See\nhttps://cirrus-ci.com/task/4577761405698048 for an example. The run at\nhttps://cirrus-ci.com/task/6004115347079168 shows that this patch fixes the\nbug.\n\nWhile verifying the fix on Windows, I noticed a recent, rather terrible\nperformance regression: t5351 all of a sudden takes almost half an hour\n[https://github.com/git/git/runs/7398490747?check_suite_focus=true#step:5:171]\nto run on Windows. I found a fix, and it now passes in less than half a\nminute\n[https://github.com/gitgitgadget/git/runs/7578071365?check_suite_focus=true#step:5:125]\nagain.\n\nJohannes Schindelin (2):\n t5351: avoid relying on `core.fsyncMethod = batch` to be supported\n t5351: avoid using `test_cmp` for binary data\n\n bulk-checkin.c | 2 ++\n t/t5351-unpack-large-objects.sh | 12 +++++++++---\n 2 files changed, 11 insertions(+), 3 deletions(-)\n\n\nbase-commit: 23b219f8e3f2adfb0441e135f0a880e6124f766c\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1308%2Fdscho%2Ffix-t5351-on-freebsd-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1308/dscho/fix-t5351-on-freebsd-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1308\n-- \ngitgitgadget\n"},{"id":"460206","messageId":"501a89aa0b2de396b0c7b82b2e24046b9c98c382.1659097724.git.gitgitgadget@gmail.com","threadId":"58241","inReplyTo":"pull.1308.git.1659097724.gitgitgadget@gmail.com","subject":"[PATCH 1/2] t5351: avoid relying on `core.fsyncMethod = batch` to be supported","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-07-29T12:28:43Z","receivedAt":"2022-07-29T12:28:54Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nOn FreeBSD, this mode is not supported. But since 3a251bac0d1a (trace2:\nonly include \"fsync\" events if we git_fsync(), 2022-07-18) t5351 will\nfail if this mode is unsupported.\n\nLet's address this in the minimal fashion, by detecting that that mode\nis unsupported and expecting a different count of hardware flushes in\nthat case.\n\nThis fixes the CI/PR builds on FreeBSD again.\n\nNote: A better way would be to test only what is relevant in t5351.6\n\"unpack big object in stream (core.fsyncmethod=batch)\" again instead of\nblindly comparing the output against some exact text. But that would\npretty much revert the idea of above-mentioned commit, and that commit\nwas _just_ accepted into Git's main branch so one must assume that it\nwas intentional.\n\nSigned-off-by: Johannes Schindelin \n---\n bulk-checkin.c | 2 ++\n t/t5351-unpack-large-objects.sh | 10 ++++++++--\n 2 files changed, 10 insertions(+), 2 deletions(-)\n\ndiff --git a/bulk-checkin.c b/bulk-checkin.c\nindex 98ec8938424..855b68ec23b 100644\n--- a/bulk-checkin.c\n+++ b/bulk-checkin.c\n@@ -340,6 +340,8 @@ void fsync_loose_object_bulk_checkin(int fd, const char *filename)\n \t */\n \tif (!bulk_fsync_objdir ||\n \t git_fsync(fd, FSYNC_WRITEOUT_ONLY) < 0) {\n+\t\tif (errno == ENOSYS)\n+\t\t\twarning(_(\"core.fsyncMethod = batch is unsupported on this platform\"));\n \t\tfsync_or_die(fd, filename);\n \t}\n }\ndiff --git a/t/t5351-unpack-large-objects.sh b/t/t5351-unpack-large-objects.sh\nindex f785cb06173..dd7ebc40072 100755\n--- a/t/t5351-unpack-large-objects.sh\n+++ b/t/t5351-unpack-large-objects.sh\n@@ -70,9 +70,15 @@ test_expect_success 'unpack big object in stream (core.fsyncmethod=batch)' '\n \tGIT_TRACE2_EVENT=\"$(pwd)/trace2.txt\" \\\n \tGIT_TEST_FSYNC=true \\\n \t\tgit -C dest.git $BATCH_CONFIGURATION unpack-objects \n\nThe `test_cmp` function is meant to provide nicer output than `cmp` when\nexpected and actual output of Git commands disagree. The implicit\nassumption is that the output is line-based and human readable.\n\nHowever, aaf81223f48 (unpack-objects: use stream_loose_object() to\nunpack large objects, 2022-06-11) introduced a call that compares the\ncontents of pack files, which are distinctly not line-based nor human\nreadable.\n\nThis causes problems because on Windows, we hand off to the Bash\nfunction `mingw_test_cmp` that compares the lines while ignoring line\nending differences. And this Bash function spends an insane amount of\ncycles trying to read in that binary pack file, so that it is almost\nindistinguishable from an infinite loop.\n\nFor example, t5351 took 1486 seconds in the CI run at\nhttps://github.com/git/git/runs/7398490747?check_suite_focus=true#step:5:171,\nto complete. And yes, that is almost half an hour.\n\nSince Git's tests already use `cmp` consistently when comparing pack\nfiles, let's change this instance to use `cmp` instead of `test_cmp`,\ntoo, and fix that performance problem.\n\nNow t5351 takes all of 22 seconds.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t5351-unpack-large-objects.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t5351-unpack-large-objects.sh b/t/t5351-unpack-large-objects.sh\nindex dd7ebc40072..e936f91c3ba 100755\n--- a/t/t5351-unpack-large-objects.sh\n+++ b/t/t5351-unpack-large-objects.sh\n@@ -93,7 +93,7 @@ test_expect_success 'do not unpack existing large objects' '\n \n \t# The destination came up with the exact same pack...\n \tDEST_PACK=$(echo dest.git/objects/pack/pack-*.pack) &&\n-\ttest_cmp pack-$PACK.pack $DEST_PACK &&\n+\tcmp pack-$PACK.pack $DEST_PACK &&\n \n \t# ...and wrote no loose objects\n \ttest_stdout_line_count = 0 find dest.git/objects -type f ! -name \"pack-*\"\n-- \ngitgitgadget\n"},{"id":"460210","messageId":"985e4bb8-0cb1-3980-aaf7-bd3c793f7627@github.com","threadId":"58241","inReplyTo":"pull.1308.git.1659097724.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/2] ci: fix the FreeBSD build","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-07-29T12:58:46Z","receivedAt":"2022-07-29T12:58:54Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 7/29/2022 8:28 AM, Johannes Schindelin via GitGitGadget wrote:\n> Since 3a251bac0d1a (trace2: only include \"fsync\" events if we git_fsync(),\n> 2022-07-18), the FreeBSD builds are failing in t5351.6. See\n> https://cirrus-ci.com/task/4577761405698048 for an example. The run at\n> https://cirrus-ci.com/task/6004115347079168 shows that this patch fixes the\n> bug.\n\nThanks for noticing and fixing this bug. The FreeBSD build is slow\nand flaky enough that I sometimes ignore its output before submitting\na series. Good that it will be green again.\n \n> While verifying the fix on Windows, I noticed a recent, rather terrible\n> performance regression: t5351 all of a sudden takes almost half an hour\n> [https://github.com/git/git/runs/7398490747?check_suite_focus=true#step:5:171]\n> to run on Windows. I found a fix, and it now passes in less than half a\n> minute\n> [https://github.com/gitgitgadget/git/runs/7578071365?check_suite_focus=true#step:5:125]\n> again.\n\nThanks for speeding this up!\n\n-Stolee\n"},{"id":"460231","messageId":"xmqq7d3vaovk.fsf@gitster.g","threadId":"58241","inReplyTo":"pull.1308.git.1659097724.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/2] ci: fix the FreeBSD build","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-29T16:02:23Z","receivedAt":"2022-07-29T16:02:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> performance regression: t5351 all of a sudden takes almost half an hour\n> [https://github.com/git/git/runs/7398490747?check_suite_focus=true#step:5:171]\n> to run on Windows. I found a fix, and it now passes in less than half a\n> minute\n\n;-) That's impressive.\n\n"},{"id":"460232","messageId":"xmqq1qu3aoml.fsf@gitster.g","threadId":"58241","inReplyTo":"501a89aa0b2de396b0c7b82b2e24046b9c98c382.1659097724.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] t5351: avoid relying on `core.fsyncMethod = batch` to be supported","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-29T16:07:46Z","receivedAt":"2022-07-29T16:07:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> On FreeBSD, this mode is not supported. But since 3a251bac0d1a (trace2:\n> only include \"fsync\" events if we git_fsync(), 2022-07-18) t5351 will\n> fail if this mode is unsupported.\n>\n> Let's address this in the minimal fashion, by detecting that that mode\n> is unsupported and expecting a different count of hardware flushes in\n> that case.\n>\n> This fixes the CI/PR builds on FreeBSD again.\n>\n> Note: A better way would be to test only what is relevant in t5351.6\n> \"unpack big object in stream (core.fsyncmethod=batch)\" again instead of\n> blindly comparing the output against some exact text. But that would\n> pretty much revert the idea of above-mentioned commit, and that commit\n> was _just_ accepted into Git's main branch so one must assume that it\n> was intentional.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> bulk-checkin.c | 2 ++\n> t/t5351-unpack-large-objects.sh | 10 ++++++++--\n> 2 files changed, 10 insertions(+), 2 deletions(-)\n\nI am inclined to take this as-is for now.\n\nBut it inherits from 3a251bac0d1a the general \"we should be able to\ncount the value\" expectation, which may not be the best approach to\nrun this test; asking Acks from those originally involved in this\narea and possibly ideas to test this in a more robust way.\n\nThanks.\n\n\n> diff --git a/bulk-checkin.c b/bulk-checkin.c\n> index 98ec8938424..855b68ec23b 100644\n> --- a/bulk-checkin.c\n> +++ b/bulk-checkin.c\n> @@ -340,6 +340,8 @@ void fsync_loose_object_bulk_checkin(int fd, const char *filename)\n> \t */\n> \tif (!bulk_fsync_objdir ||\n> \t git_fsync(fd, FSYNC_WRITEOUT_ONLY) < 0) {\n> +\t\tif (errno == ENOSYS)\n> +\t\t\twarning(_(\"core.fsyncMethod = batch is unsupported on this platform\"));\n> \t\tfsync_or_die(fd, filename);\n> \t}\n> }\n> diff --git a/t/t5351-unpack-large-objects.sh b/t/t5351-unpack-large-objects.sh\n> index f785cb06173..dd7ebc40072 100755\n> --- a/t/t5351-unpack-large-objects.sh\n> +++ b/t/t5351-unpack-large-objects.sh\n> @@ -70,9 +70,15 @@ test_expect_success 'unpack big object in stream (core.fsyncmethod=batch)' '\n> \tGIT_TRACE2_EVENT=\"$(pwd)/trace2.txt\" \\\n> \tGIT_TEST_FSYNC=true \\\n> \t\tgit -C dest.git $BATCH_CONFIGURATION unpack-objects -\tcheck_fsync_events trace2.txt <<-\\EOF &&\n> +\tif grep \"core.fsyncMethod = batch is unsupported\" trace2.txt\n> +\tthen\n> +\t\tflush_count=7\n> +\telse\n> +\t\tflush_count=1\n> +\tfi &&\n> +\tcheck_fsync_events trace2.txt <<-EOF &&\n> \t\"key\":\"fsync/writeout-only\",\"value\":\"6\"\n> -\t\"key\":\"fsync/hardware-flush\",\"value\":\"1\"\n> +\t\"key\":\"fsync/hardware-flush\",\"value\":\"$flush_count\"\n> \tEOF\n> \n> \ttest_dir_is_empty dest.git/objects/pack &&\n"},{"id":"460239","messageId":"20220729175108.zk63qonchmrvprsl@Carlos-MacBook-Pro-2.local","threadId":"58241","inReplyTo":"985e4bb8-0cb1-3980-aaf7-bd3c793f7627@github.com","subject":"Re: [PATCH 0/2] ci: fix the FreeBSD build","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2022-07-29T17:51:08Z","receivedAt":"2022-07-29T17:51:17Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Fri, Jul 29, 2022 at 08:58:46AM -0400, Derrick Stolee wrote:\n> On 7/29/2022 8:28 AM, Johannes Schindelin via GitGitGadget wrote:\n> > Since 3a251bac0d1a (trace2: only include \"fsync\" events if we git_fsync(),\n> > 2022-07-18), the FreeBSD builds are failing in t5351.6. See\n> > https://cirrus-ci.com/task/4577761405698048 for an example. The run at\n> > https://cirrus-ci.com/task/6004115347079168 shows that this patch fixes the\n> > bug.\n> \n> Thanks for noticing and fixing this bug. The FreeBSD build is slow\n\nIt usually takes a little more than 7 minutes for a full run, which is IMHO\nless (at least wall time) than the whole CI does; could you elaborate on\nwhy being \"slow\" would warrant ignoring its failures?\n\n> and flaky enough that I sometimes ignore its output before submitting\n> a series. Good that it will be green again.\n\nI'd noticed that because it runs outside GitHub actions it sometimes has\nsynchronization(ex [1]) issues, but that might be some bug on the integration\nwith Cirrus which is easily avoided by looking instead directly to their\nstatus page:\n\n https://cirrus-ci.com/github/git/git\n\nCarlo\n"},{"id":"460276","messageId":"YuRQJTzA1EtDUFSv@tapette.crustytoothpaste.net","threadId":"58241","inReplyTo":"xmqq1qu3aoml.fsf@gitster.g","subject":"Re: [PATCH 1/2] t5351: avoid relying on `core.fsyncMethod = batch` to be supported","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2022-07-29T21:24:53Z","receivedAt":"2022-07-29T21:25:03Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2022-07-29 at 16:07:46, Junio C Hamano wrote:\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n> \n> > From: Johannes Schindelin \n> >\n> > On FreeBSD, this mode is not supported. But since 3a251bac0d1a (trace2:\n> > only include \"fsync\" events if we git_fsync(), 2022-07-18) t5351 will\n> > fail if this mode is unsupported.\n> >\n> > Let's address this in the minimal fashion, by detecting that that mode\n> > is unsupported and expecting a different count of hardware flushes in\n> > that case.\n> >\n> > This fixes the CI/PR builds on FreeBSD again.\n> >\n> > Note: A better way would be to test only what is relevant in t5351.6\n> > \"unpack big object in stream (core.fsyncmethod=batch)\" again instead of\n> > blindly comparing the output against some exact text. But that would\n> > pretty much revert the idea of above-mentioned commit, and that commit\n> > was _just_ accepted into Git's main branch so one must assume that it\n> > was intentional.\n> >\n> > Signed-off-by: Johannes Schindelin \n> > ---\n> > bulk-checkin.c | 2 ++\n> > t/t5351-unpack-large-objects.sh | 10 ++++++++--\n> > 2 files changed, 10 insertions(+), 2 deletions(-)\n> \n> I am inclined to take this as-is for now.\n\nI think this patch is a strict improvement over the status quo, despite\nwhat I'm going to mention below.\n\n> But it inherits from 3a251bac0d1a the general \"we should be able to\n> count the value\" expectation, which may not be the best approach to\n> run this test; asking Acks from those originally involved in this\n> area and possibly ideas to test this in a more robust way.\n\nWhile it may not matter in this test, I noticed that the metrics we use\nneed not be accurate in multi-threaded programs. We use integers of\nstatic intmax_t which isn't necessarily atomic across threads. Even if\nwe assumed word-sized increments were atomic[0], this type might be 64\nbits on a 32-bit system.\n\nI am aware that we don't use threads in many places, but in general we\nshould be safely able to assume that we can perform an fsync on any\nthread without thread safety problems because that's the assumption a\nreasonable Unix programmer would make. Even if it's not a problem now,\nit could well be in the future, and we should either fix this (say, with\nC11 atomic integers[1]) or put a note on the metrics functions that they\nmay be wrong and stop using them in tests.\n\nI would, in general, prefer that if we add wrappers that wrap common\nUnix functions we ensure that they provide the same guarantees as the\ncommon Unix functions we're wrapping. I realize I may sound fussy, but\nI've been fixing giant thread-safety problems recently and it's not fun.\n\n[0] This is not, in general, a safe assumption to make, since most RISC\narchitectures are load-store.\n[1] This would necessitate moving to C11, which is fine with me (and\nalready required on Windows), but others may have objections to.\n-- \nbrian m. carlson (he/him or they/them)\nToronto, Ontario, CA\n"}]}