| Message ID | cover.1586309211.git.congdanhqx@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | ci: replace our Azure Pipeline by GitHub Actions | expand |
Đoàn Trần Công Danh <congdanhqx@gmail.com> writes: > Our Azure Pipeline has served us well over the course of the past year or > so, steadily catching issues before the respective patches hit the next > branch. > > There is a GitHub-native CI system now, though, called "GitHub Actions" > [https://github.com/features/actions] which is essentially on par with Azure > Pipelines as far as our needs are concerned, and it brings a couple of > advantages: > > * It is substantially easier to set up than Azure Pipelines: all you need > is to add the YAML-based build definition, push to your fork on GitHub, > and that's it. > * The syntax is a bit easier to read than Azure Pipelines'. > * We get more concurrent jobs (Azure Pipelines is limited to 10 concurrent > jobs). > > With this change, users also no longer need to open a PR at > https://github.com/git/git or at https://github.com/gitgitgadget/git just to > get the benefit of a CI build. > > Sample run on top of dd/ci-musl-libc with dd/test-with-busybox merged: > https://github.com/sgn/git/actions/runs/73179413 > > Sample run when this series applied into git-for-windows > https://github.com/git-for-windows/git/runs/568625109 > > Change from v3: > - Use build matrix > - All dependencies are install by scripts > - stop repeating environment variables > - build failure's artifacts will be uploaded I did not see any particular thing that is bad in any of the three series involved; do people have further comments? I am not exactly happy that these had to be queued on top of a merge of two topics in flight, which makes it cumbersome to deal with a breakage in these two other topics, though, but that would be a pain only until these two topics prove to be stable enough to build on. Judging from two CI runs for 'pu' ([*1*] and [*2*]), among the topics that are cooking, there are only a few topics that these tests are unhappy about. Perhaps those on Windows can help these topics to pass these tests? [References] *1* https://github.com/git/git/actions/runs/74687673 is 'pu' with all cooking topics. *2* https://github.com/git/git/actions/runs/74741625 is 'pu' with some topics excluded.
Hi Junio, On Thu, 9 Apr 2020, Junio C Hamano wrote: > Đoàn Trần Công Danh <congdanhqx@gmail.com> writes: > > > Our Azure Pipeline has served us well over the course of the past year or > > so, steadily catching issues before the respective patches hit the next > > branch. > > > > There is a GitHub-native CI system now, though, called "GitHub Actions" > > [https://github.com/features/actions] which is essentially on par with Azure > > Pipelines as far as our needs are concerned, and it brings a couple of > > advantages: > > > > * It is substantially easier to set up than Azure Pipelines: all you need > > is to add the YAML-based build definition, push to your fork on GitHub, > > and that's it. > > * The syntax is a bit easier to read than Azure Pipelines'. > > * We get more concurrent jobs (Azure Pipelines is limited to 10 concurrent > > jobs). > > > > With this change, users also no longer need to open a PR at > > https://github.com/git/git or at https://github.com/gitgitgadget/git just to > > get the benefit of a CI build. > > > > Sample run on top of dd/ci-musl-libc with dd/test-with-busybox merged: > > https://github.com/sgn/git/actions/runs/73179413 > > > > Sample run when this series applied into git-for-windows > > https://github.com/git-for-windows/git/runs/568625109 > > > > Change from v3: > > - Use build matrix > > - All dependencies are install by scripts > > - stop repeating environment variables > > - build failure's artifacts will be uploaded > > I did not see any particular thing that is bad in any of the three > series involved; do people have further comments? FWIW I consider this work good enough that I already merged it into Git for Windows. It should make it easier for contributors to test their branches "privately", in their own forks, before opening a PR (most people do not like to have relatively trivial issues pointed out by fellow human beings, but are much more okay with machines telling them what needs to be improved). Please mark this up as a vote of confidence from my side. > I am not exactly happy that these had to be queued on top of a merge > of two topics in flight, which makes it cumbersome to deal with a > breakage in these two other topics, though, but that would be a pain > only until these two topics prove to be stable enough to build on. Yes, and the fact that `ci-musl-libc` was _not_ based on top of `test-with-busybox` makes it a bit more awkward. I, for one, had totally missed that the latter patch series is _required_ in order to make the former work correctly. Hunting for the cause for almost an hour until Danh kindly pointed out that he had fixed all the issues in `test-with-busybox` already. > Judging from two CI runs for 'pu' ([*1*] and [*2*]), among the > topics that are cooking, there are only a few topics that these > tests are unhappy about. Perhaps those on Windows can help these > topics to pass these tests? I would like to point out that there is only one single topic that is cause for sorrow here, and it is the reftable one. Before going further, let me point out that the `pu` branch has been broken for quite a long time now, primarily because of `bugreport` and... of course because of `reftable`. Whenever `pu` included `reftable`, the CI builds failed. So these `reftable` problems are not a good reason, in my mind, to hold up the GitHub workflow patches from advancing. Seeing the short stat `35 files changed, 6719 insertions(+)` of even a single patch in the `reftable` patch series _really_ does not entice me to spend time even looking at it, certainly not at a time when I am short on time, let alone to try to find time to fix it. However, since you asked so nicely, I did start to look into it. First, let me present you the less controversial of two patches I want to show you: -- snip -- From 5f42a3f6ef9cf7d90bd274e55539b145cae40e28 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin <johannes.schindelin@gmx.de> Date: Fri, 10 Apr 2020 14:23:40 +0200 Subject: [PATCH] fixup??? Reftable support for git-core As I had already pointed out over a month ago in https://github.com/gitgitgadget/git/pull/539#issuecomment-589157008 this C code violates the C standard, and MS Visual C is not as lenient as GCC/clang on it: `struct`s cannot be initialized with `= {}`. Compile errors aside, while this code conforms to the C syntax, it feels more like Java when it initializes e.g. `struct object_id` only to _immediately_ overwrite the contents. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> --- refs/reftable-backend.c | 52 ++++++++++++++++++++--------------------- 1 file changed, 26 insertions(+), 26 deletions(-) diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index 6e845e9c649..20c94bb403b 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -31,7 +31,7 @@ static void clear_reftable_log_record(struct reftable_log_record *log) static void fill_reftable_log_record(struct reftable_log_record *log) { const char *info = git_committer_info(0); - struct ident_split split = {}; + struct ident_split split = { NULL }; int result = split_ident_line(&split, info, strlen(info)); int sign = 1; assert(0 == result); @@ -230,7 +230,7 @@ static int reftable_transaction_abort(struct ref_store *ref_store, static int reftable_check_old_oid(struct ref_store *refs, const char *refname, struct object_id *want_oid) { - struct object_id out_oid = {}; + struct object_id out_oid; int out_flags = 0; const char *resolved = refs_resolve_ref_unsafe( refs, refname, RESOLVE_REF_READING, &out_oid, &out_flags); @@ -287,14 +287,14 @@ static int write_transaction_table(struct reftable_writer *writer, void *arg) log->message = u->msg; if (u->flags & REF_HAVE_NEW) { - struct object_id out_oid = {}; + struct object_id out_oid; int out_flags = 0; /* Memory owned by refs_resolve_ref_unsafe, no need to * free(). */ const char *resolved = refs_resolve_ref_unsafe( transaction->ref_store, u->refname, 0, &out_oid, &out_flags); - struct reftable_ref_record ref = {}; + struct reftable_ref_record ref = { NULL }; ref.ref_name = (char *)(resolved ? resolved : u->refname); log->ref_name = ref.ref_name; @@ -376,8 +376,8 @@ static int write_delete_refs_table(struct reftable_writer *writer, void *argv) } for (int i = 0; i < arg->refnames->nr; i++) { - struct reftable_log_record log = {}; - struct reftable_ref_record current = {}; + struct reftable_log_record log = { NULL }; + struct reftable_ref_record current = { NULL }; fill_reftable_log_record(&log); log.message = xstrdup(arg->logmsg); log.new_hash = NULL; @@ -455,10 +455,10 @@ static int write_create_symref_table(struct reftable_writer *writer, void *arg) } { - struct reftable_log_record log = {}; - struct object_id new_oid = {}; - struct object_id old_oid = {}; - struct reftable_ref_record current = {}; + struct reftable_log_record log = { NULL }; + struct object_id new_oid; + struct object_id old_oid; + struct reftable_ref_record current = { NULL }; reftable_stack_read_ref(create->refs->stack, create->refname, ¤t); fill_reftable_log_record(&log); @@ -513,7 +513,7 @@ static int write_rename_table(struct reftable_writer *writer, void *argv) { struct write_rename_arg *arg = (struct write_rename_arg *)argv; uint64_t ts = reftable_stack_next_update_index(arg->stack); - struct reftable_ref_record ref = {}; + struct reftable_ref_record ref = { NULL }; int err = reftable_stack_read_ref(arg->stack, arg->oldname, &ref); if (err) { @@ -531,7 +531,7 @@ static int write_rename_table(struct reftable_writer *writer, void *argv) ref.update_index = ts; { - struct reftable_ref_record todo[2] = {}; + struct reftable_ref_record todo[2] = { { NULL } }; todo[0].ref_name = (char *)arg->oldname; todo[0].update_index = ts; /* leave todo[0] empty */ @@ -545,7 +545,7 @@ static int write_rename_table(struct reftable_writer *writer, void *argv) } if (ref.value != NULL) { - struct reftable_log_record todo[2] = {}; + struct reftable_log_record todo[2] = { { NULL } }; fill_reftable_log_record(&todo[0]); fill_reftable_log_record(&todo[1]); @@ -688,12 +688,12 @@ reftable_for_each_reflog_ent_newest_first(struct ref_store *ref_store, const char *refname, each_reflog_ent_fn fn, void *cb_data) { - struct reftable_iterator it = {}; + struct reftable_iterator it = { NULL }; struct git_reftable_ref_store *refs = (struct git_reftable_ref_store *)ref_store; struct reftable_merged_table *mt = NULL; int err = 0; - struct reftable_log_record log = {}; + struct reftable_log_record log = { NULL }; if (refs->err < 0) { return refs->err; @@ -712,8 +712,8 @@ reftable_for_each_reflog_ent_newest_first(struct ref_store *ref_store, } { - struct object_id old_oid = {}; - struct object_id new_oid = {}; + struct object_id old_oid; + struct object_id new_oid; const char *full_committer = ""; hashcpy(old_oid.hash, log.old_hash); @@ -744,7 +744,7 @@ reftable_for_each_reflog_ent_oldest_first(struct ref_store *ref_store, const char *refname, each_reflog_ent_fn fn, void *cb_data) { - struct reftable_iterator it = {}; + struct reftable_iterator it = { NULL }; struct git_reftable_ref_store *refs = (struct git_reftable_ref_store *)ref_store; struct reftable_merged_table *mt = NULL; @@ -760,7 +760,7 @@ reftable_for_each_reflog_ent_oldest_first(struct ref_store *ref_store, err = reftable_merged_table_seek_log(mt, &it, refname); while (err == 0) { - struct reftable_log_record log = {}; + struct reftable_log_record log = { NULL }; err = reftable_iterator_next_log(it, &log); if (err != 0) { break; @@ -780,8 +780,8 @@ reftable_for_each_reflog_ent_oldest_first(struct ref_store *ref_store, for (int i = len; i--;) { struct reftable_log_record *log = &logs[i]; - struct object_id old_oid = {}; - struct object_id new_oid = {}; + struct object_id old_oid; + struct object_id new_oid; const char *full_committer = ""; hashcpy(old_oid.hash, log->old_hash); @@ -903,8 +903,8 @@ static int reftable_reflog_expire(struct ref_store *ref_store, struct reflog_expiry_arg arg = { .refs = refs, }; - struct reftable_log_record log = {}; - struct reftable_iterator it = {}; + struct reftable_log_record log = { NULL }; + struct reftable_iterator it = { NULL }; int err = 0; if (refs->err < 0) { return refs->err; @@ -917,8 +917,8 @@ static int reftable_reflog_expire(struct ref_store *ref_store, } while (1) { - struct object_id ooid = {}; - struct object_id noid = {}; + struct object_id ooid; + struct object_id noid; int err = reftable_iterator_next_log(it, &log); if (err < 0) { @@ -950,7 +950,7 @@ static int reftable_read_raw_ref(struct ref_store *ref_store, { struct git_reftable_ref_store *refs = (struct git_reftable_ref_store *)ref_store; - struct reftable_ref_record ref = {}; + struct reftable_ref_record ref = { NULL }; int err = 0; if (refs->err < 0) { return refs->err; -- snap -- This patch should fix the `vs-build` job in the Azure Pipeline as well as in the GitHub workflow. However, it does _not_ fix the test failure on Windows. When I tried to investigate this, I wanted to compare the results between Windows and Linux (WSL, of course, it is a major time saver for me these days because I don't have to power up a VM, and I can access WSL files from Windows and vice versa), and it turns out that the `000000000002-000000000002.ref` file is different, it even has different sizes (242 bytes on Windows instead of 268 bytes on Linux), and notably it contains the string `HEAD` on Windows and `refs/heads/master` on Linux, but not vice versa. So I dug a bit deeper and was stopped rudely by the fact that the t0031-reftable.sh script produces different output every time it runs. Because it does not even use `test_commit`. Therefore, let me present you with this patch (whose commit message conveys a rather alarming indication that this endeavor of fixing `reftable` could become a major time sink): -- snip - From 6ba47e70a2eb8efe2116c12eb950ddb90c473d11 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin <johannes.schindelin@gmx.de> Date: Fri, 10 Apr 2020 16:10:53 +0200 Subject: [PATCH] fixup??? Reftable support for git-core The test for the reftable functionality should use the convenience functions we provide for test scripts. Using `test_commit` in particular does help with reproducible output (otherwise the SHA-1s will change together with the time the tests were run). Currently, this here seemingly innocuous change causes quite a few warnings throughout the test, though, e.g. this slur of warnings when committing the last commit in the test script: warning: bad replace ref name: e warning: bad replace ref name: ber-1 warning: bad replace ref name: ber-2 warning: bad replace ref name: ber-3 warning: bad replace ref name: ber-4 warning: bad replace ref name: ber-5 warning: bad replace ref name: ber-6 warning: bad replace ref name: ber-7 warning: bad replace ref name: ber-8 warning: bad replace ref name: ber-9 This is notably _not_ a problem that was introduced by this here patch, of course, but a very real concern about the reftable patches, most likely the big one that introduces the reftable library in one fell swoop. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> --- t/t0031-reftable.sh | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/t/t0031-reftable.sh b/t/t0031-reftable.sh index 3ebf13c2f42..4bc7bd8167f 100755 --- a/t/t0031-reftable.sh +++ b/t/t0031-reftable.sh @@ -8,28 +8,26 @@ test_description='reftable basics' . ./test-lib.sh test_expect_success 'basic operation of reftable storage' ' - git init --ref-storage=reftable repo && ( - cd repo && - echo "hello" >world.txt && - git add world.txt && - git commit -m "first post" && - test_write_lines HEAD refs/heads/master >expect && + rm -rf .git && + git init --ref-storage=reftable && + mv .git/hooks .git/hooks-disabled && + test_commit file && + test_write_lines HEAD refs/heads/master refs/tags/file >expect && git show-ref && git show-ref | cut -f2 -d" " > actual && test_cmp actual expect && for count in $(test_seq 1 10) do - echo "hello" >>world.txt - git commit -m "number ${count}" world.txt || + test_commit "number $count" file.t $count number-$count || return 1 done && git gc && - nfiles=$(ls -1 .git/reftable | wc -l ) && - test ${nfiles} = "2" && + ls -1 .git/reftable >table-files && + test_line_count = 2 table-files && git reflog refs/heads/master >output && test_line_count = 11 output && grep "commit (initial): first post" output && - grep "commit: number 10" output ) + grep "commit: number 10" output ' test_done -- snap -- While I am very happy with the post-image of this diff, I am super unhappy about the output of it. It makes me believe that this `reftable` patch series is in serious need of being "incrementalized" _after the fact_. Otherwise it will be simply impossible to build enough confidence in the correctness of it, especially given the fact that it obviously does some incorrect things right now (see the "bad replace ref name" warning mentioned above). I'll take a break from this now, but I would like to encourage you to apply both patches as `SQUASH???` on top of `hn/reftable` for the time being. Ciao, Dscho > > > [References] > > *1* https://github.com/git/git/actions/runs/74687673 is 'pu' with > all cooking topics. > > *2* https://github.com/git/git/actions/runs/74741625 is 'pu' with > some topics excluded. > >
Hi Junio,
me again, just quickly, because the `t0031-reftable.sh --valgrind` run
just came back with this:
-- snip --
[...]
+ git gc
==28394== error calling PR_SET_PTRACER, vgdb might block
==28399== error calling PR_SET_PTRACER, vgdb might block
==28399== error calling PR_SET_PTRACER, vgdb might block
==28404== error calling PR_SET_PTRACER, vgdb might block
==28404== Invalid read of size 1
==28404== at 0x4C32CF2: strlen (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==28404== by 0x551D9AD: strdup (strdup.c:41)
==28404== by 0x39D15A: xstrdup (wrapper.c:29)
==28404== by 0x3DA9CA: reftable_log_record_copy_from (record.c:605)
==28404== by 0x3DB844: record_copy_from (record.c:968)
==28404== by 0x3D64B3: merged_iter_next (merged.c:117)
==28404== by 0x3D656B: merged_iter_next_void (merged.c:131)
==28404== by 0x3D597D: iterator_next (iter.c:45)
==28404== by 0x3D5AD2: reftable_iterator_next_log (iter.c:71)
==28404== by 0x3DE037: stack_write_compact (stack.c:718)
==28404== by 0x3DDBEA: stack_compact_locked (stack.c:632)
==28404== by 0x3DE5AD: stack_compact_range (stack.c:847)
==28404== Address 0x0 is not stack'd, malloc'd or (recently) free'd
==28404==
{
<insert_a_suppression_name_here>
Memcheck:Addr1
fun:strlen
fun:strdup
fun:xstrdup
fun:reftable_log_record_copy_from
fun:record_copy_from
fun:merged_iter_next
fun:merged_iter_next_void
fun:iterator_next
fun:reftable_iterator_next_log
fun:stack_write_compact
fun:stack_compact_locked
fun:stack_compact_range
}
==28404==
==28404== Process terminating with default action of signal 11 (SIGSEGV)
==28404== Access not within mapped region at address 0x0
==28404== at 0x4C32CF2: strlen (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==28404== by 0x551D9AD: strdup (strdup.c:41)
==28404== by 0x39D15A: xstrdup (wrapper.c:29)
==28404== by 0x3DA9CA: reftable_log_record_copy_from (record.c:605)
==28404== by 0x3DB844: record_copy_from (record.c:968)
==28404== by 0x3D64B3: merged_iter_next (merged.c:117)
==28404== by 0x3D656B: merged_iter_next_void (merged.c:131)
==28404== by 0x3D597D: iterator_next (iter.c:45)
==28404== by 0x3D5AD2: reftable_iterator_next_log (iter.c:71)
==28404== by 0x3DE037: stack_write_compact (stack.c:718)
==28404== by 0x3DDBEA: stack_compact_locked (stack.c:632)
==28404== by 0x3DE5AD: stack_compact_range (stack.c:847)
==28404== If you believe this happened as a result of a stack
==28404== overflow in your program's main thread (unlikely but
==28404== possible), you can try to increase the size of the
==28404== main thread stack using the --main-stacksize= flag.
==28404== The main thread stack size used in this run was 8388608.
error: reflog died of signal 11
fatal: failed to run reflog
error: last command exited with $?=128
-- snap --
But now _really_ want to take a break from this,
Dscho
On Fri, 10 Apr 2020, Johannes Schindelin wrote:
> Hi Junio,
>
> On Thu, 9 Apr 2020, Junio C Hamano wrote:
>
> > Đoàn Trần Công Danh <congdanhqx@gmail.com> writes:
> >
> > > Our Azure Pipeline has served us well over the course of the past year or
> > > so, steadily catching issues before the respective patches hit the next
> > > branch.
> > >
> > > There is a GitHub-native CI system now, though, called "GitHub Actions"
> > > [https://github.com/features/actions] which is essentially on par with Azure
> > > Pipelines as far as our needs are concerned, and it brings a couple of
> > > advantages:
> > >
> > > * It is substantially easier to set up than Azure Pipelines: all you need
> > > is to add the YAML-based build definition, push to your fork on GitHub,
> > > and that's it.
> > > * The syntax is a bit easier to read than Azure Pipelines'.
> > > * We get more concurrent jobs (Azure Pipelines is limited to 10 concurrent
> > > jobs).
> > >
> > > With this change, users also no longer need to open a PR at
> > > https://github.com/git/git or at https://github.com/gitgitgadget/git just to
> > > get the benefit of a CI build.
> > >
> > > Sample run on top of dd/ci-musl-libc with dd/test-with-busybox merged:
> > > https://github.com/sgn/git/actions/runs/73179413
> > >
> > > Sample run when this series applied into git-for-windows
> > > https://github.com/git-for-windows/git/runs/568625109
> > >
> > > Change from v3:
> > > - Use build matrix
> > > - All dependencies are install by scripts
> > > - stop repeating environment variables
> > > - build failure's artifacts will be uploaded
> >
> > I did not see any particular thing that is bad in any of the three
> > series involved; do people have further comments?
>
> FWIW I consider this work good enough that I already merged it into Git
> for Windows. It should make it easier for contributors to test their
> branches "privately", in their own forks, before opening a PR (most people
> do not like to have relatively trivial issues pointed out by fellow human
> beings, but are much more okay with machines telling them what needs to be
> improved).
>
> Please mark this up as a vote of confidence from my side.
>
> > I am not exactly happy that these had to be queued on top of a merge
> > of two topics in flight, which makes it cumbersome to deal with a
> > breakage in these two other topics, though, but that would be a pain
> > only until these two topics prove to be stable enough to build on.
>
> Yes, and the fact that `ci-musl-libc` was _not_ based on top of
> `test-with-busybox` makes it a bit more awkward. I, for one, had totally
> missed that the latter patch series is _required_ in order to make the
> former work correctly. Hunting for the cause for almost an hour until Danh
> kindly pointed out that he had fixed all the issues in `test-with-busybox`
> already.
>
> > Judging from two CI runs for 'pu' ([*1*] and [*2*]), among the
> > topics that are cooking, there are only a few topics that these
> > tests are unhappy about. Perhaps those on Windows can help these
> > topics to pass these tests?
>
> I would like to point out that there is only one single topic that is
> cause for sorrow here, and it is the reftable one.
>
> Before going further, let me point out that the `pu` branch has been
> broken for quite a long time now, primarily because of `bugreport` and...
> of course because of `reftable`. Whenever `pu` included `reftable`, the CI
> builds failed. So these `reftable` problems are not a good reason, in my
> mind, to hold up the GitHub workflow patches from advancing.
>
> Seeing the short stat `35 files changed, 6719 insertions(+)` of even a
> single patch in the `reftable` patch series _really_ does not entice me to
> spend time even looking at it, certainly not at a time when I am short on
> time, let alone to try to find time to fix it.
>
> However, since you asked so nicely, I did start to look into it. First,
> let me present you the less controversial of two patches I want to show
> you:
>
> -- snip --
> From 5f42a3f6ef9cf7d90bd274e55539b145cae40e28 Mon Sep 17 00:00:00 2001
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> Date: Fri, 10 Apr 2020 14:23:40 +0200
> Subject: [PATCH] fixup??? Reftable support for git-core
>
> As I had already pointed out over a month ago in
> https://github.com/gitgitgadget/git/pull/539#issuecomment-589157008 this
> C code violates the C standard, and MS Visual C is not as lenient as
> GCC/clang on it: `struct`s cannot be initialized with `= {}`.
>
> Compile errors aside, while this code conforms to the C syntax, it feels
> more like Java when it initializes e.g. `struct object_id` only to
> _immediately_ overwrite the contents.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> refs/reftable-backend.c | 52 ++++++++++++++++++++---------------------
> 1 file changed, 26 insertions(+), 26 deletions(-)
>
> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
> index 6e845e9c649..20c94bb403b 100644
> --- a/refs/reftable-backend.c
> +++ b/refs/reftable-backend.c
> @@ -31,7 +31,7 @@ static void clear_reftable_log_record(struct reftable_log_record *log)
> static void fill_reftable_log_record(struct reftable_log_record *log)
> {
> const char *info = git_committer_info(0);
> - struct ident_split split = {};
> + struct ident_split split = { NULL };
> int result = split_ident_line(&split, info, strlen(info));
> int sign = 1;
> assert(0 == result);
> @@ -230,7 +230,7 @@ static int reftable_transaction_abort(struct ref_store *ref_store,
> static int reftable_check_old_oid(struct ref_store *refs, const char *refname,
> struct object_id *want_oid)
> {
> - struct object_id out_oid = {};
> + struct object_id out_oid;
> int out_flags = 0;
> const char *resolved = refs_resolve_ref_unsafe(
> refs, refname, RESOLVE_REF_READING, &out_oid, &out_flags);
> @@ -287,14 +287,14 @@ static int write_transaction_table(struct reftable_writer *writer, void *arg)
> log->message = u->msg;
>
> if (u->flags & REF_HAVE_NEW) {
> - struct object_id out_oid = {};
> + struct object_id out_oid;
> int out_flags = 0;
> /* Memory owned by refs_resolve_ref_unsafe, no need to
> * free(). */
> const char *resolved = refs_resolve_ref_unsafe(
> transaction->ref_store, u->refname, 0, &out_oid,
> &out_flags);
> - struct reftable_ref_record ref = {};
> + struct reftable_ref_record ref = { NULL };
> ref.ref_name =
> (char *)(resolved ? resolved : u->refname);
> log->ref_name = ref.ref_name;
> @@ -376,8 +376,8 @@ static int write_delete_refs_table(struct reftable_writer *writer, void *argv)
> }
>
> for (int i = 0; i < arg->refnames->nr; i++) {
> - struct reftable_log_record log = {};
> - struct reftable_ref_record current = {};
> + struct reftable_log_record log = { NULL };
> + struct reftable_ref_record current = { NULL };
> fill_reftable_log_record(&log);
> log.message = xstrdup(arg->logmsg);
> log.new_hash = NULL;
> @@ -455,10 +455,10 @@ static int write_create_symref_table(struct reftable_writer *writer, void *arg)
> }
>
> {
> - struct reftable_log_record log = {};
> - struct object_id new_oid = {};
> - struct object_id old_oid = {};
> - struct reftable_ref_record current = {};
> + struct reftable_log_record log = { NULL };
> + struct object_id new_oid;
> + struct object_id old_oid;
> + struct reftable_ref_record current = { NULL };
> reftable_stack_read_ref(create->refs->stack, create->refname, ¤t);
>
> fill_reftable_log_record(&log);
> @@ -513,7 +513,7 @@ static int write_rename_table(struct reftable_writer *writer, void *argv)
> {
> struct write_rename_arg *arg = (struct write_rename_arg *)argv;
> uint64_t ts = reftable_stack_next_update_index(arg->stack);
> - struct reftable_ref_record ref = {};
> + struct reftable_ref_record ref = { NULL };
> int err = reftable_stack_read_ref(arg->stack, arg->oldname, &ref);
>
> if (err) {
> @@ -531,7 +531,7 @@ static int write_rename_table(struct reftable_writer *writer, void *argv)
> ref.update_index = ts;
>
> {
> - struct reftable_ref_record todo[2] = {};
> + struct reftable_ref_record todo[2] = { { NULL } };
> todo[0].ref_name = (char *)arg->oldname;
> todo[0].update_index = ts;
> /* leave todo[0] empty */
> @@ -545,7 +545,7 @@ static int write_rename_table(struct reftable_writer *writer, void *argv)
> }
>
> if (ref.value != NULL) {
> - struct reftable_log_record todo[2] = {};
> + struct reftable_log_record todo[2] = { { NULL } };
> fill_reftable_log_record(&todo[0]);
> fill_reftable_log_record(&todo[1]);
>
> @@ -688,12 +688,12 @@ reftable_for_each_reflog_ent_newest_first(struct ref_store *ref_store,
> const char *refname,
> each_reflog_ent_fn fn, void *cb_data)
> {
> - struct reftable_iterator it = {};
> + struct reftable_iterator it = { NULL };
> struct git_reftable_ref_store *refs =
> (struct git_reftable_ref_store *)ref_store;
> struct reftable_merged_table *mt = NULL;
> int err = 0;
> - struct reftable_log_record log = {};
> + struct reftable_log_record log = { NULL };
>
> if (refs->err < 0) {
> return refs->err;
> @@ -712,8 +712,8 @@ reftable_for_each_reflog_ent_newest_first(struct ref_store *ref_store,
> }
>
> {
> - struct object_id old_oid = {};
> - struct object_id new_oid = {};
> + struct object_id old_oid;
> + struct object_id new_oid;
> const char *full_committer = "";
>
> hashcpy(old_oid.hash, log.old_hash);
> @@ -744,7 +744,7 @@ reftable_for_each_reflog_ent_oldest_first(struct ref_store *ref_store,
> const char *refname,
> each_reflog_ent_fn fn, void *cb_data)
> {
> - struct reftable_iterator it = {};
> + struct reftable_iterator it = { NULL };
> struct git_reftable_ref_store *refs =
> (struct git_reftable_ref_store *)ref_store;
> struct reftable_merged_table *mt = NULL;
> @@ -760,7 +760,7 @@ reftable_for_each_reflog_ent_oldest_first(struct ref_store *ref_store,
> err = reftable_merged_table_seek_log(mt, &it, refname);
>
> while (err == 0) {
> - struct reftable_log_record log = {};
> + struct reftable_log_record log = { NULL };
> err = reftable_iterator_next_log(it, &log);
> if (err != 0) {
> break;
> @@ -780,8 +780,8 @@ reftable_for_each_reflog_ent_oldest_first(struct ref_store *ref_store,
>
> for (int i = len; i--;) {
> struct reftable_log_record *log = &logs[i];
> - struct object_id old_oid = {};
> - struct object_id new_oid = {};
> + struct object_id old_oid;
> + struct object_id new_oid;
> const char *full_committer = "";
>
> hashcpy(old_oid.hash, log->old_hash);
> @@ -903,8 +903,8 @@ static int reftable_reflog_expire(struct ref_store *ref_store,
> struct reflog_expiry_arg arg = {
> .refs = refs,
> };
> - struct reftable_log_record log = {};
> - struct reftable_iterator it = {};
> + struct reftable_log_record log = { NULL };
> + struct reftable_iterator it = { NULL };
> int err = 0;
> if (refs->err < 0) {
> return refs->err;
> @@ -917,8 +917,8 @@ static int reftable_reflog_expire(struct ref_store *ref_store,
> }
>
> while (1) {
> - struct object_id ooid = {};
> - struct object_id noid = {};
> + struct object_id ooid;
> + struct object_id noid;
>
> int err = reftable_iterator_next_log(it, &log);
> if (err < 0) {
> @@ -950,7 +950,7 @@ static int reftable_read_raw_ref(struct ref_store *ref_store,
> {
> struct git_reftable_ref_store *refs =
> (struct git_reftable_ref_store *)ref_store;
> - struct reftable_ref_record ref = {};
> + struct reftable_ref_record ref = { NULL };
> int err = 0;
> if (refs->err < 0) {
> return refs->err;
> -- snap --
>
> This patch should fix the `vs-build` job in the Azure Pipeline as well as
> in the GitHub workflow.
>
> However, it does _not_ fix the test failure on Windows. When I tried to
> investigate this, I wanted to compare the results between Windows and
> Linux (WSL, of course, it is a major time saver for me these days because
> I don't have to power up a VM, and I can access WSL files from Windows and
> vice versa), and it turns out that the `000000000002-000000000002.ref`
> file is different, it even has different sizes (242 bytes on Windows
> instead of 268 bytes on Linux), and notably it contains the string `HEAD`
> on Windows and `refs/heads/master` on Linux, but not vice versa.
>
> So I dug a bit deeper and was stopped rudely by the fact that the
> t0031-reftable.sh script produces different output every time it runs.
> Because it does not even use `test_commit`.
>
> Therefore, let me present you with this patch (whose commit message
> conveys a rather alarming indication that this endeavor of fixing
> `reftable` could become a major time sink):
>
> -- snip -
> From 6ba47e70a2eb8efe2116c12eb950ddb90c473d11 Mon Sep 17 00:00:00 2001
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> Date: Fri, 10 Apr 2020 16:10:53 +0200
> Subject: [PATCH] fixup??? Reftable support for git-core
>
> The test for the reftable functionality should use the convenience
> functions we provide for test scripts. Using `test_commit` in particular
> does help with reproducible output (otherwise the SHA-1s will change
> together with the time the tests were run).
>
> Currently, this here seemingly innocuous change causes quite a few
> warnings throughout the test, though, e.g. this slur of warnings when
> committing the last commit in the test script:
>
> warning: bad replace ref name: e
> warning: bad replace ref name: ber-1
> warning: bad replace ref name: ber-2
> warning: bad replace ref name: ber-3
> warning: bad replace ref name: ber-4
> warning: bad replace ref name: ber-5
> warning: bad replace ref name: ber-6
> warning: bad replace ref name: ber-7
> warning: bad replace ref name: ber-8
> warning: bad replace ref name: ber-9
>
> This is notably _not_ a problem that was introduced by this here patch,
> of course, but a very real concern about the reftable patches, most
> likely the big one that introduces the reftable library in one fell
> swoop.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> t/t0031-reftable.sh | 20 +++++++++-----------
> 1 file changed, 9 insertions(+), 11 deletions(-)
>
> diff --git a/t/t0031-reftable.sh b/t/t0031-reftable.sh
> index 3ebf13c2f42..4bc7bd8167f 100755
> --- a/t/t0031-reftable.sh
> +++ b/t/t0031-reftable.sh
> @@ -8,28 +8,26 @@ test_description='reftable basics'
> . ./test-lib.sh
>
> test_expect_success 'basic operation of reftable storage' '
> - git init --ref-storage=reftable repo && (
> - cd repo &&
> - echo "hello" >world.txt &&
> - git add world.txt &&
> - git commit -m "first post" &&
> - test_write_lines HEAD refs/heads/master >expect &&
> + rm -rf .git &&
> + git init --ref-storage=reftable &&
> + mv .git/hooks .git/hooks-disabled &&
> + test_commit file &&
> + test_write_lines HEAD refs/heads/master refs/tags/file >expect &&
> git show-ref &&
> git show-ref | cut -f2 -d" " > actual &&
> test_cmp actual expect &&
> for count in $(test_seq 1 10)
> do
> - echo "hello" >>world.txt
> - git commit -m "number ${count}" world.txt ||
> + test_commit "number $count" file.t $count number-$count ||
> return 1
> done &&
> git gc &&
> - nfiles=$(ls -1 .git/reftable | wc -l ) &&
> - test ${nfiles} = "2" &&
> + ls -1 .git/reftable >table-files &&
> + test_line_count = 2 table-files &&
> git reflog refs/heads/master >output &&
> test_line_count = 11 output &&
> grep "commit (initial): first post" output &&
> - grep "commit: number 10" output )
> + grep "commit: number 10" output
> '
>
> test_done
> -- snap --
>
> While I am very happy with the post-image of this diff, I am super unhappy
> about the output of it. It makes me believe that this `reftable` patch
> series is in serious need of being "incrementalized" _after the fact_.
> Otherwise it will be simply impossible to build enough confidence in the
> correctness of it, especially given the fact that it obviously does some
> incorrect things right now (see the "bad replace ref name" warning
> mentioned above).
>
> I'll take a break from this now, but I would like to encourage you to
> apply both patches as `SQUASH???` on top of `hn/reftable` for the time
> being.
>
> Ciao,
> Dscho
>
> >
> >
> > [References]
> >
> > *1* https://github.com/git/git/actions/runs/74687673 is 'pu' with
> > all cooking topics.
> >
> > *2* https://github.com/git/git/actions/runs/74741625 is 'pu' with
> > some topics excluded.
> >
> >
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: >> Judging from two CI runs for 'pu' ([*1*] and [*2*]), among the >> topics that are cooking, there are only a few topics that these >> tests are unhappy about. Perhaps those on Windows can help these >> topics to pass these tests? > > I would like to point out that there is only one single topic that is > cause for sorrow here, and it is the reftable one. I first thought so, too, but vsbuild failures like the one in https://github.com/git/git/runs/575116793 do not appear to be caused by that topic (6a8c1d17b8 excludes reftable), so there must be somebody else that is broken. An all green build https://github.com/git/git/actions/runs/74741625 was my attempt to see how ready these tests are (not 'how ready other topics are to be tested by this topic) by moving the swap-azure-pipelines-with-github-actions topic early in 'pu', temporarily discarding many other topics, and pushing it out, by the way.
On 2020-04-10 16:37:27+0200, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote: > Hi Junio, > > me again, just quickly, because the `t0031-reftable.sh --valgrind` run > just came back with this: > > -- snip -- > [...] > + git gc > ==28394== error calling PR_SET_PTRACER, vgdb might block > ==28399== error calling PR_SET_PTRACER, vgdb might block > ==28399== error calling PR_SET_PTRACER, vgdb might block > ==28404== error calling PR_SET_PTRACER, vgdb might block > ==28404== Invalid read of size 1 > ==28404== at 0x4C32CF2: strlen (in > /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) > ==28404== by 0x551D9AD: strdup (strdup.c:41) > ==28404== by 0x39D15A: xstrdup (wrapper.c:29) > ==28404== by 0x3DA9CA: reftable_log_record_copy_from (record.c:605) > ==28404== by 0x3DB844: record_copy_from (record.c:968) > ==28404== by 0x3D64B3: merged_iter_next (merged.c:117) > ==28404== by 0x3D656B: merged_iter_next_void (merged.c:131) > ==28404== by 0x3D597D: iterator_next (iter.c:45) > ==28404== by 0x3D5AD2: reftable_iterator_next_log (iter.c:71) > ==28404== by 0x3DE037: stack_write_compact (stack.c:718) > ==28404== by 0x3DDBEA: stack_compact_locked (stack.c:632) > ==28404== by 0x3DE5AD: stack_compact_range (stack.c:847) > ==28404== Address 0x0 is not stack'd, malloc'd or (recently) free'd > ==28404== > { > <insert_a_suppression_name_here> > Memcheck:Addr1 > fun:strlen > fun:strdup > fun:xstrdup > fun:reftable_log_record_copy_from > fun:record_copy_from > fun:merged_iter_next > fun:merged_iter_next_void > fun:iterator_next > fun:reftable_iterator_next_log > fun:stack_write_compact > fun:stack_compact_locked > fun:stack_compact_range > } > ==28404== > ==28404== Process terminating with default action of signal 11 (SIGSEGV) > ==28404== Access not within mapped region at address 0x0 > ==28404== at 0x4C32CF2: strlen (in > /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) > ==28404== by 0x551D9AD: strdup (strdup.c:41) > ==28404== by 0x39D15A: xstrdup (wrapper.c:29) > ==28404== by 0x3DA9CA: reftable_log_record_copy_from (record.c:605) > ==28404== by 0x3DB844: record_copy_from (record.c:968) > ==28404== by 0x3D64B3: merged_iter_next (merged.c:117) > ==28404== by 0x3D656B: merged_iter_next_void (merged.c:131) > ==28404== by 0x3D597D: iterator_next (iter.c:45) > ==28404== by 0x3D5AD2: reftable_iterator_next_log (iter.c:71) > ==28404== by 0x3DE037: stack_write_compact (stack.c:718) > ==28404== by 0x3DDBEA: stack_compact_locked (stack.c:632) > ==28404== by 0x3DE5AD: stack_compact_range (stack.c:847) > ==28404== If you believe this happened as a result of a stack > ==28404== overflow in your program's main thread (unlikely but > ==28404== possible), you can try to increase the size of the > ==28404== main thread stack using the --main-stacksize= flag. > ==28404== The main thread stack size used in this run was 8388608. > error: reflog died of signal 11 > fatal: failed to run reflog > error: last command exited with $?=128 > -- snap -- The second patch from Dscho (which is only test code in sh) trigger segfault with and without the first patch on top of pu. Git was built with DEVELOPER=1 I merely run "./t0031-reftable.sh -d -v -i"
On 2020-04-10 08:42:10-0700, Junio C Hamano <gitster@pobox.com> wrote: > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > > >> Judging from two CI runs for 'pu' ([*1*] and [*2*]), among the > >> topics that are cooking, there are only a few topics that these > >> tests are unhappy about. Perhaps those on Windows can help these > >> topics to pass these tests? > > > > I would like to point out that there is only one single topic that is > > cause for sorrow here, and it is the reftable one. > > I first thought so, too, but vsbuild failures like the one in > https://github.com/git/git/runs/575116793 do not appear to be > caused by that topic (6a8c1d17b8 excludes reftable), so there > must be somebody else that is broken. Excerpt from build log: > fatal error C1083: Cannot open include file: 'config-list.h' It's from bugreport topic. I've seen this failure in the past (when testing with pu), then I saw it disappear. I thought it was fixed during my testing for v4.
Danh Doan <congdanhqx@gmail.com> writes: > On 2020-04-10 08:42:10-0700, Junio C Hamano <gitster@pobox.com> wrote: >> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: >> ... >> > I would like to point out that there is only one single topic that is >> > cause for sorrow here, and it is the reftable one. >> >> I first thought so, too, but vsbuild failures like the one in >> https://github.com/git/git/runs/575116793 do not appear to be >> caused by that topic (6a8c1d17b8 excludes reftable), so there >> must be somebody else that is broken. > > Excerpt from build log: > >> fatal error C1083: Cannot open include file: 'config-list.h' > > It's from bugreport topic. > I've seen this failure in the past (when testing with pu), > then I saw it disappear. > > I thought it was fixed during my testing for v4. Is the issue something similar to 976aaedc (msvc: add a Makefile target to pre-generate the Visual Studio solution, 2019-07-29)? If that is the case, perhaps something like this would help? I'll tentatively queue it on top of es/bugreport and merge the result to 'pu' to see what happens. -- >8 -- Subject: msvc: the bugreport topic depends on a generated config-list.h file For reasons explained in 976aaedc (msvc: add a Makefile target to pre-generate the Visual Studio solution, 2019-07-29), some build artifacts we consider non-source files cannot be generated in the Visual Studio environment, and we already have some Makefile tweaks to help Visual Studio to use generated command-list.h header file. As this topic starts to depend on another such generated header file, config-list.h, let's do the same to it. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- compat/vcbuild/README | 4 ++-- config.mak.uname | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/compat/vcbuild/README b/compat/vcbuild/README index 1b6dabf5a2..42292e7c09 100644 --- a/compat/vcbuild/README +++ b/compat/vcbuild/README @@ -92,8 +92,8 @@ The Steps of Build Git with VS2008 the git operations. 3. Inside Git's directory run the command: - make command-list.h - to generate the command-list.h file needed to compile git. + make command-list.h config-list.h + to generate the header file needed to compile git. 4. Then either build Git with the GNU Make Makefile in the Git projects root diff --git a/config.mak.uname b/config.mak.uname index 0ab8e00938..f880cc2792 100644 --- a/config.mak.uname +++ b/config.mak.uname @@ -721,9 +721,9 @@ vcxproj: echo '</Project>') >git-remote-http/LinkOrCopyRemoteHttp.targets git add -f git/LinkOrCopyBuiltins.targets git-remote-http/LinkOrCopyRemoteHttp.targets - # Add command-list.h - $(MAKE) MSVC=1 SKIP_VCPKG=1 prefix=/mingw64 command-list.h - git add -f command-list.h + # Add command-list.h and config-list.h + $(MAKE) MSVC=1 SKIP_VCPKG=1 prefix=/mingw64 config-list.h command-list.h + git add -f config-list.h command-list.h # Add scripts rm -f perl/perl.mak
Junio C Hamano <gitster@pobox.com> writes: > Danh Doan <congdanhqx@gmail.com> writes: > >> Excerpt from build log: >> >>> fatal error C1083: Cannot open include file: 'config-list.h' >> >> It's from bugreport topic. >> I've seen this failure in the past (when testing with pu), >> then I saw it disappear. >> >> I thought it was fixed during my testing for v4. > > Is the issue something similar to 976aaedc (msvc: add a Makefile > target to pre-generate the Visual Studio solution, 2019-07-29)? > > If that is the case, perhaps something like this would help? I'll > tentatively queue it on top of es/bugreport and merge the result to > 'pu' to see what happens. The build just passed: https://github.com/git/git/runs/590781044 Emily, you may need to squash in something along the line of this change to the commit in your series that starts building and using the config-list.h file (was it the first one?). I've queued mine as a follow-up "oops, it was wrong" patch, but that would not be kosher from bisectability's point of view. But before we see a full reroll of the topic, it would be good to have a quick "looks OK" from somebody who does Windows (Dscho CC'ed). Thanks. > -- >8 -- > Subject: msvc: the bugreport topic depends on a generated config-list.h file > > For reasons explained in 976aaedc (msvc: add a Makefile target to > pre-generate the Visual Studio solution, 2019-07-29), some build > artifacts we consider non-source files cannot be generated in the > Visual Studio environment, and we already have some Makefile tweaks > to help Visual Studio to use generated command-list.h header file. > > As this topic starts to depend on another such generated header file, > config-list.h, let's do the same to it. > > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- > compat/vcbuild/README | 4 ++-- > config.mak.uname | 6 +++--- > 2 files changed, 5 insertions(+), 5 deletions(-) > > diff --git a/compat/vcbuild/README b/compat/vcbuild/README > index 1b6dabf5a2..42292e7c09 100644 > --- a/compat/vcbuild/README > +++ b/compat/vcbuild/README > @@ -92,8 +92,8 @@ The Steps of Build Git with VS2008 > the git operations. > > 3. Inside Git's directory run the command: > - make command-list.h > - to generate the command-list.h file needed to compile git. > + make command-list.h config-list.h > + to generate the header file needed to compile git. > > 4. Then either build Git with the GNU Make Makefile in the Git projects > root > diff --git a/config.mak.uname b/config.mak.uname > index 0ab8e00938..f880cc2792 100644 > --- a/config.mak.uname > +++ b/config.mak.uname > @@ -721,9 +721,9 @@ vcxproj: > echo '</Project>') >git-remote-http/LinkOrCopyRemoteHttp.targets > git add -f git/LinkOrCopyBuiltins.targets git-remote-http/LinkOrCopyRemoteHttp.targets > > - # Add command-list.h > - $(MAKE) MSVC=1 SKIP_VCPKG=1 prefix=/mingw64 command-list.h > - git add -f command-list.h > + # Add command-list.h and config-list.h > + $(MAKE) MSVC=1 SKIP_VCPKG=1 prefix=/mingw64 config-list.h command-list.h > + git add -f config-list.h command-list.h > > # Add scripts > rm -f perl/perl.mak
On Wed, Apr 15, 2020 at 6:28 PM Junio C Hamano <gitster@pobox.com> wrote: > > Junio C Hamano <gitster@pobox.com> writes: > > > Danh Doan <congdanhqx@gmail.com> writes: > > > >> Excerpt from build log: > >> > >>> fatal error C1083: Cannot open include file: 'config-list.h' > >> > >> It's from bugreport topic. > >> I've seen this failure in the past (when testing with pu), > >> then I saw it disappear. > >> > >> I thought it was fixed during my testing for v4. > > > > Is the issue something similar to 976aaedc (msvc: add a Makefile > > target to pre-generate the Visual Studio solution, 2019-07-29)? > > > > If that is the case, perhaps something like this would help? I'll > > tentatively queue it on top of es/bugreport and merge the result to > > 'pu' to see what happens. > > The build just passed: https://github.com/git/git/runs/590781044 > > Emily, you may need to squash in something along the line of this > change to the commit in your series that starts building and using > the config-list.h file (was it the first one?). I've queued mine > as a follow-up "oops, it was wrong" patch, but that would not be > kosher from bisectability's point of view. Hm, ok. I'll send a reroll squashing this in verbatim tomorrow unless I hear otherwise from Dscho? Looks like it's indeed the first one (dd763e). I'm curious to know how I can check this build method for myself for next time. (If I misunderstood and I should send a reroll ASAP, please let me know; otherwise I already switched off for the evening.) - Emily
Emily Shaffer <emilyshaffer@google.com> writes: > Hm, ok. I'll send a reroll squashing this in verbatim tomorrow unless > I hear otherwise from Dscho? Looks like it's indeed the first one > (dd763e). > I'm curious to know how I can check this build method for myself for next time. > > (If I misunderstood and I should send a reroll ASAP, please let me > know; otherwise I already switched off for the evening.) Nah, I do not think it is all that urgent. I'd rather wait until we hear positive "yup, that's the right way to do it" (or "no, you'd do it this way instead" guidance) to waste an extra round. Having said that, the topic won't touch 'next' with a known CI glitch whose fix ought to be straight-forward especially with help/nod from experts, and as far as I recall, there wasn't any other outstanding issues for this round, even though we may already have plans for possible follow-up enhancements, so let's not keep it hanging unnecessarily longer. Perhaps we'd all be happy if we can resolve it before the end of this week or early next week? Thanks.
Hi Emily, On Wed, Apr 15, 2020 at 7:01 PM Emily Shaffer <emilyshaffer@google.com> wrote: > > On Wed, Apr 15, 2020 at 6:28 PM Junio C Hamano <gitster@pobox.com> wrote: > > > > Junio C Hamano <gitster@pobox.com> writes: > > > > > Danh Doan <congdanhqx@gmail.com> writes: > > > > > >> Excerpt from build log: > > >> > > >>> fatal error C1083: Cannot open include file: 'config-list.h' > > >> > > >> It's from bugreport topic. > > >> I've seen this failure in the past (when testing with pu), > > >> then I saw it disappear. > > >> > > >> I thought it was fixed during my testing for v4. > > > > > > Is the issue something similar to 976aaedc (msvc: add a Makefile > > > target to pre-generate the Visual Studio solution, 2019-07-29)? > > > > > > If that is the case, perhaps something like this would help? I'll > > > tentatively queue it on top of es/bugreport and merge the result to > > > 'pu' to see what happens. > > > > The build just passed: https://github.com/git/git/runs/590781044 > > > > Emily, you may need to squash in something along the line of this > > change to the commit in your series that starts building and using > > the config-list.h file (was it the first one?). I've queued mine > > as a follow-up "oops, it was wrong" patch, but that would not be > > kosher from bisectability's point of view. > > Hm, ok. I'll send a reroll squashing this in verbatim tomorrow unless > I hear otherwise from Dscho? Looks like it's indeed the first one > (dd763e). > I'm curious to know how I can check this build method for myself for next time. Create a fork of github.com/git/git and open a pull request against it. (I believe you could also fork github.com/gitgitgadget/git and do a pull request against it, but I switched over to /git/git a while ago.) Immediately upon opening the pull request, a bunch of linux, mac, windows, and freebsd builds will be triggered with various runs of the testsuite. Has been very useful for catching issues for me before I sent them off to the list. You can also make use of Dscho's gitgitgadget magic at that point to take care of the git send-email step for you too by commenting '/submit' in the PR, though you don't have to do that. It's perfectly fine to just open a PR for the automated testing and then close the PR and do the rest yourself. But if you leave it open and have it submit the patch emails for you, it'll track their status. I kinda like being able to go to https://github.com/git/git/pulls/newren and have it track the state of where all my open pull requests are. (You might even be able to click through some of those to see example build results) Dscho has done some great work with his gitgitgadget work and azure builds. SZEDER Gábor also has a few builds triggered via Travis that check a few more things off the same PRs (static anlysis and whatnot). I've been very happily using them all by just opening PRs, and have appreciated the heads up of potential issues I would've otherwise caused on various platforms otherwise. Hope that helps, Elijah
On Wed, Apr 15, 2020 at 08:45:05PM -0700, Elijah Newren wrote: > > Hi Emily, > > On Wed, Apr 15, 2020 at 7:01 PM Emily Shaffer <emilyshaffer@google.com> wrote: > > > > On Wed, Apr 15, 2020 at 6:28 PM Junio C Hamano <gitster@pobox.com> wrote: > > > > > > Junio C Hamano <gitster@pobox.com> writes: > > > > > > > Danh Doan <congdanhqx@gmail.com> writes: > > > > > > > >> Excerpt from build log: > > > >> > > > >>> fatal error C1083: Cannot open include file: 'config-list.h' > > > >> > > > >> It's from bugreport topic. > > > >> I've seen this failure in the past (when testing with pu), > > > >> then I saw it disappear. > > > >> > > > >> I thought it was fixed during my testing for v4. > > > > > > > > Is the issue something similar to 976aaedc (msvc: add a Makefile > > > > target to pre-generate the Visual Studio solution, 2019-07-29)? > > > > > > > > If that is the case, perhaps something like this would help? I'll > > > > tentatively queue it on top of es/bugreport and merge the result to > > > > 'pu' to see what happens. > > > > > > The build just passed: https://github.com/git/git/runs/590781044 > > > > > > Emily, you may need to squash in something along the line of this > > > change to the commit in your series that starts building and using > > > the config-list.h file (was it the first one?). I've queued mine > > > as a follow-up "oops, it was wrong" patch, but that would not be > > > kosher from bisectability's point of view. > > > > Hm, ok. I'll send a reroll squashing this in verbatim tomorrow unless > > I hear otherwise from Dscho? Looks like it's indeed the first one > > (dd763e). > > I'm curious to know how I can check this build method for myself for next time. > > Create a fork of github.com/git/git and open a pull request against > it. (I believe you could also fork github.com/gitgitgadget/git and do > a pull request against it, but I switched over to /git/git a while > ago.) Immediately upon opening the pull request, a bunch of linux, > mac, windows, and freebsd builds will be triggered with various runs > of the testsuite. Has been very useful for catching issues for me > before I sent them off to the list. I did before I sent this iteration, and it passed: https://github.com/gitgitgadget/git/pull/573 That's why I'm confused :) Did I do something differently? I don't use GGG to send the emails, but I do use it to run CI checks. - Emily
Emily Shaffer <emilyshaffer@google.com> writes: > On Wed, Apr 15, 2020 at 08:45:05PM -0700, Elijah Newren wrote: >> >> Create a fork of github.com/git/git and open a pull request against >> it. (I believe you could also fork github.com/gitgitgadget/git and do >> a pull request against it, but I switched over to /git/git a while >> ago.) Immediately upon opening the pull request, a bunch of linux, >> mac, windows, and freebsd builds will be triggered with various runs >> of the testsuite. Has been very useful for catching issues for me >> before I sent them off to the list. > > I did before I sent this iteration, and it passed: > https://github.com/gitgitgadget/git/pull/573 > > That's why I'm confused :) Did I do something differently? I don't use > GGG to send the emails, but I do use it to run CI checks. Comparing the list of "checks" revealed by clicking "Show all checks" there, with the list of "Actions" with recent tip of 'pu', say, https://github.com/git/git/actions/runs/79416884, I notice that the former does not have vs-build. Also, the former seems to be on "Azure Pipelines" (e.g. [*1*] which is "Windows build" among the "checks" on the list), while the latter is "Github Actions" (e.g. [*2*], among which exists "VS-build" that seems to be missing from the former). The latter is coming from having the dd/ci-swap-azure-pipelines-with-github-actions topic and other two topics that it depends on, which are right now only in 'pu'. As we'd like to advance the Github Actions CI support to 'next', I've been looking at the failures to it caused by individual topics (and right now, we know of two topics, one is the bugreport and the other is reftable) to make sure these other topics can enter 'next'. Thanks. [References] *1* https://github.com/gitgitgadget/git/pull/573/checks?check_run_id=565642291 *2* https://github.com/git/git/runs/590781044?check_suite_focus=true
On 2020-04-15 20:45:05-0700, Elijah Newren <newren@gmail.com> wrote: > Hi Emily, > > On Wed, Apr 15, 2020 at 7:01 PM Emily Shaffer <emilyshaffer@google.com> wrote: > > > > On Wed, Apr 15, 2020 at 6:28 PM Junio C Hamano <gitster@pobox.com> wrote: > > > > > > Junio C Hamano <gitster@pobox.com> writes: > > > > > > > Danh Doan <congdanhqx@gmail.com> writes: > > > > > > > >> Excerpt from build log: > > > >> > > > >>> fatal error C1083: Cannot open include file: 'config-list.h' > > > >> > > > >> It's from bugreport topic. > > > >> I've seen this failure in the past (when testing with pu), > > > >> then I saw it disappear. > > > >> > > > >> I thought it was fixed during my testing for v4. > > > > > > > > Is the issue something similar to 976aaedc (msvc: add a Makefile > > > > target to pre-generate the Visual Studio solution, 2019-07-29)? > > > > > > > > If that is the case, perhaps something like this would help? I'll > > > > tentatively queue it on top of es/bugreport and merge the result to > > > > 'pu' to see what happens. > > > > > > The build just passed: https://github.com/git/git/runs/590781044 > > > > > > Emily, you may need to squash in something along the line of this > > > change to the commit in your series that starts building and using > > > the config-list.h file (was it the first one?). I've queued mine > > > as a follow-up "oops, it was wrong" patch, but that would not be > > > kosher from bisectability's point of view. > > > > Hm, ok. I'll send a reroll squashing this in verbatim tomorrow unless > > I hear otherwise from Dscho? Looks like it's indeed the first one > > (dd763e). > > I'm curious to know how I can check this build method for myself for next time. > > Create a fork of github.com/git/git and open a pull request against > it. (I believe you could also fork github.com/gitgitgadget/git and do > a pull request against it, but I switched over to /git/git a while > ago.) Immediately upon opening the pull request, a bunch of linux, > mac, windows, and freebsd builds will be triggered with various runs > of the testsuite. Has been very useful for catching issues for me > before I sent them off to the list. For the time being, open a Github PR will trigger Azure Pipelines to check various things with both Linux, macOS, and Windows. This Azure thing doesn't have that vs-build target, yet. We're moving to Github Actions. When that topic graduate to master, we can simply branch out from master and push to our fork in GitHub, it will run automatically. No need to create a PR on git.git anymore To check that vs-build target for the time being by merging dd/ci-swap-azure-pipelines-with-github-actions and push to your GitHub fork.
Hi Emily, On Thu, 16 Apr 2020, Danh Doan wrote: > On 2020-04-15 20:45:05-0700, Elijah Newren <newren@gmail.com> wrote: > > > On Wed, Apr 15, 2020 at 7:01 PM Emily Shaffer > > <emilyshaffer@google.com> wrote: > > > > > I'm curious to know how I can check this build method for myself for > > > next time. > > > > Create a fork of github.com/git/git and open a pull request against > > it. (I believe you could also fork github.com/gitgitgadget/git and do > > a pull request against it, but I switched over to /git/git a while > > ago.) Immediately upon opening the pull request, a bunch of linux, > > mac, windows, and freebsd builds will be triggered with various runs > > of the testsuite. Has been very useful for catching issues for me > > before I sent them off to the list. > > For the time being, open a Github PR will trigger Azure Pipelines to Please spell it with an upper-case `H`: there is no `th` sound in GitHub. > check various things with both Linux, macOS, and Windows. > This Azure thing doesn't have that vs-build target, yet. More concretely, you will want to open a PR at https://github.com/git/git, not at https://github.com/gitgitgadget/git. For reasons (having to do with Junio's practice to base branches on older commits, where `azure-pipelines.yml` either does not exist, or needs changes to pass), the latter runs an Azure Pipeline on Pull Requests which is based _not_ on `azure-pipelines.yml`, but is essentially a manual re-implementation of it that does _not_ use YAML (but is revisioned separately), with manual patches for all kinds of issues on top that have made it into core Git's `master`. However, in your case I would strongly advise to simply use a throw-away branch, merge in the GitHub workflow patches by Danh and myself (as described in the quoted text below), and push it to your fork on GitHub. That will execute a workflow run that will show up at https://github.com/nasamuffin/git/actions/new. Ciao, Dscho > > We're moving to Github Actions. When that topic graduate to master, > we can simply branch out from master and push to our fork in GitHub, > it will run automatically. No need to create a PR on git.git anymore > > To check that vs-build target for the time being by merging > dd/ci-swap-azure-pipelines-with-github-actions > and push to your GitHub fork. > > -- > Danh > >
Hi Junio, On Wed, 15 Apr 2020, Junio C Hamano wrote: > Danh Doan <congdanhqx@gmail.com> writes: > > > On 2020-04-10 08:42:10-0700, Junio C Hamano <gitster@pobox.com> wrote: > >> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > >> ... > >> > I would like to point out that there is only one single topic that is > >> > cause for sorrow here, and it is the reftable one. > >> > >> I first thought so, too, but vsbuild failures like the one in > >> https://github.com/git/git/runs/575116793 do not appear to be > >> caused by that topic (6a8c1d17b8 excludes reftable), so there > >> must be somebody else that is broken. > > > > Excerpt from build log: > > > >> fatal error C1083: Cannot open include file: 'config-list.h' > > > > It's from bugreport topic. > > I've seen this failure in the past (when testing with pu), > > then I saw it disappear. > > > > I thought it was fixed during my testing for v4. > > Is the issue something similar to 976aaedc (msvc: add a Makefile > target to pre-generate the Visual Studio solution, 2019-07-29)? > > If that is the case, perhaps something like this would help? I'll > tentatively queue it on top of es/bugreport and merge the result to > 'pu' to see what happens. This patch is morally equivalent to (albeit a bit more complete than) the patch I suggested in my mail to Emily that I sent on February 26th: https://lore.kernel.org/git/nycvar.QRO.7.76.6.2002261649550.46@tvgsbejvaqbjf.bet/ So yes, I am very much in favor of that patch. Thanks, Dscho > > -- >8 -- > Subject: msvc: the bugreport topic depends on a generated config-list.h file > > For reasons explained in 976aaedc (msvc: add a Makefile target to > pre-generate the Visual Studio solution, 2019-07-29), some build > artifacts we consider non-source files cannot be generated in the > Visual Studio environment, and we already have some Makefile tweaks > to help Visual Studio to use generated command-list.h header file. > > As this topic starts to depend on another such generated header file, > config-list.h, let's do the same to it. > > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- > compat/vcbuild/README | 4 ++-- > config.mak.uname | 6 +++--- > 2 files changed, 5 insertions(+), 5 deletions(-) > > diff --git a/compat/vcbuild/README b/compat/vcbuild/README > index 1b6dabf5a2..42292e7c09 100644 > --- a/compat/vcbuild/README > +++ b/compat/vcbuild/README > @@ -92,8 +92,8 @@ The Steps of Build Git with VS2008 > the git operations. > > 3. Inside Git's directory run the command: > - make command-list.h > - to generate the command-list.h file needed to compile git. > + make command-list.h config-list.h > + to generate the header file needed to compile git. > > 4. Then either build Git with the GNU Make Makefile in the Git projects > root > diff --git a/config.mak.uname b/config.mak.uname > index 0ab8e00938..f880cc2792 100644 > --- a/config.mak.uname > +++ b/config.mak.uname > @@ -721,9 +721,9 @@ vcxproj: > echo '</Project>') >git-remote-http/LinkOrCopyRemoteHttp.targets > git add -f git/LinkOrCopyBuiltins.targets git-remote-http/LinkOrCopyRemoteHttp.targets > > - # Add command-list.h > - $(MAKE) MSVC=1 SKIP_VCPKG=1 prefix=/mingw64 command-list.h > - git add -f command-list.h > + # Add command-list.h and config-list.h > + $(MAKE) MSVC=1 SKIP_VCPKG=1 prefix=/mingw64 config-list.h command-list.h > + git add -f config-list.h command-list.h > > # Add scripts > rm -f perl/perl.mak > >
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > This patch is morally equivalent to (albeit a bit more complete than) the > patch I suggested in my mail to Emily that I sent on February 26th: > https://lore.kernel.org/git/nycvar.QRO.7.76.6.2002261649550.46@tvgsbejvaqbjf.bet/ Ah, that was why it looked vaguely familiar. Figures. > So yes, I am very much in favor of that patch. Thanks.
Our Azure Pipeline has served us well over the course of the past year or so, steadily catching issues before the respective patches hit the next branch. There is a GitHub-native CI system now, though, called "GitHub Actions" [https://github.com/features/actions] which is essentially on par with Azure Pipelines as far as our needs are concerned, and it brings a couple of advantages: * It is substantially easier to set up than Azure Pipelines: all you need is to add the YAML-based build definition, push to your fork on GitHub, and that's it. * The syntax is a bit easier to read than Azure Pipelines'. * We get more concurrent jobs (Azure Pipelines is limited to 10 concurrent jobs). With this change, users also no longer need to open a PR at https://github.com/git/git or at https://github.com/gitgitgadget/git just to get the benefit of a CI build. Sample run on top of dd/ci-musl-libc with dd/test-with-busybox merged: https://github.com/sgn/git/actions/runs/73179413 Sample run when this series applied into git-for-windows https://github.com/git-for-windows/git/runs/568625109 Change from v3: - Use build matrix - All dependencies are install by scripts - stop repeating environment variables - build failure's artifacts will be uploaded Johannes Schindelin (9): ci/lib: if CI type is unknown, show the environment variables ci/lib: allow running in GitHub Actions ci: fix the `jobname` of the `GETTEXT_POISON` job ci: run gem with sudo to install asciidoctor README: add a build badge for the GitHub Actions runs ci: retire the Azure Pipelines definition tests: when run in Bash, annotate test failures with file name/line number ci: add a problem matcher for GitHub Actions ci: let GitHub Actions upload failed tests' directories Đoàn Trần Công Danh (3): ci/lib: set TERM environment variable if not exist ci: explicit install all required packages ci: configure GitHub Actions for CI/PR .github/workflows/main.yml | 230 +++++++++++++++ .travis.yml | 2 +- README.md | 2 +- azure-pipelines.yml | 558 ------------------------------------ ci/git-problem-matcher.json | 16 ++ ci/install-dependencies.sh | 16 +- ci/lib.sh | 31 +- ci/print-test-failures.sh | 7 + t/test-lib.sh | 14 +- 9 files changed, 310 insertions(+), 566 deletions(-) create mode 100644 .github/workflows/main.yml delete mode 100644 azure-pipelines.yml create mode 100644 ci/git-problem-matcher.json Range-diff against v3: 1: 3f9f1c6335 = 1: 2219bf3db9 ci/lib: if CI type is unknown, show the environment variables 2: 7a4f646bc1 = 2: 2818799a4b ci/lib: allow running in GitHub Actions 3: 9a03c0844c = 3: b88586c2c5 ci/lib: set TERM environment variable if not exist 4: 7308199e24 < -: ---------- ci: configure GitHub Actions for CI/PR -: ---------- > 4: 1df60e677c ci: fix the `jobname` of the `GETTEXT_POISON` job -: ---------- > 5: 4f80724641 ci: explicit install all required packages -: ---------- > 6: 795ec656c6 ci: run gem with sudo to install asciidoctor -: ---------- > 7: ec0aa20119 ci: configure GitHub Actions for CI/PR 5: 365ba5e831 = 8: 46f2b6bce6 README: add a build badge for the GitHub Actions runs 6: 53094612d3 ! 9: 92f2623dc7 ci: retire the Azure Pipelines definition @@ Commit message Pipelines would be redundant, and a waste of energy. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> - [Danh: fix apply conflicts] Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com> ## README.md ## @@ azure-pipelines.yml (deleted) - PathtoPublish: t/failed-test-artifacts - ArtifactName: failed-test-artifacts - --- job: linux_musl -- displayName: linux-musl -- condition: succeeded() -- pool: -- vmImage: ubuntu-latest -- steps: -- - bash: | -- test "$GITFILESHAREPWD" = '$(gitfileshare.pwd)' || ci/mount-fileshare.sh //gitfileshare.file.core.windows.net/test-cache gitfileshare "$GITFILESHAREPWD" "$HOME/test-cache" || exit 1 -- -- res=0 -- sudo AGENT_OS="$AGENT_OS" BUILD_BUILDNUMBER="$BUILD_BUILDNUMBER" BUILD_REPOSITORY_URI="$BUILD_REPOSITORY_URI" BUILD_SOURCEBRANCH="$BUILD_SOURCEBRANCH" BUILD_SOURCEVERSION="$BUILD_SOURCEVERSION" SYSTEM_PHASENAME="$SYSTEM_PHASENAME" SYSTEM_TASKDEFINITIONSURI="$SYSTEM_TASKDEFINITIONSURI" SYSTEM_TEAMPROJECT="$SYSTEM_TEAMPROJECT" CC=$CC MAKEFLAGS="$MAKEFLAGS" jobname=linux-musl bash -lxc ci/run-docker.sh || res=1 -- -- sudo chmod a+r t/out/TEST-*.xml -- test ! -d t/failed-test-artifacts || sudo chmod a+r t/failed-test-artifacts -- -- test "$GITFILESHAREPWD" = '$(gitfileshare.pwd)' || sudo umount "$HOME/test-cache" || res=1 -- exit $res -- displayName: 'jobname=linux-musl ci/run-docker.sh' -- env: -- GITFILESHAREPWD: $(gitfileshare.pwd) -- - task: PublishTestResults@2 -- displayName: 'Publish Test Results **/TEST-*.xml' -- inputs: -- mergeTestResults: true -- testRunTitle: 'musl' -- platform: Linux -- publishRunAttachments: false -- condition: succeededOrFailed() -- - task: PublishBuildArtifacts@1 -- displayName: 'Publish trash directories of failed tests' -- condition: failed() -- inputs: -- PathtoPublish: t/failed-test-artifacts -- ArtifactName: failed-test-artifacts -- -- job: static_analysis - displayName: StaticAnalysis - condition: succeeded() -: ---------- > 10: f688fa50d3 tests: when run in Bash, annotate test failures with file name/line number -: ---------- > 11: 715d1f732f ci: add a problem matcher for GitHub Actions -: ---------- > 12: 0908d5ab9b ci: let GitHub Actions upload failed tests' directories