| Message ID | 20190614100059.13540-1-szeder.dev@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | compat/obstack: update from upstream | expand |
On Fri, Jun 14, 2019 at 12:00:55PM +0200, SZEDER Gábor wrote: > Update 'compat/obstack.{c,h}' from upstream, because they already use > 'size_t' instead of 'long' in places that might eventually end up as > an argument to malloc(), which might solve build errors with GCC 8 on > Windows. > > The first patch just imports from upstream and doesn't modify anything > at all, and, consequently, it can't be compiled because of a screenful > or two of errors. This is bad for future bisects, of course. > > OTOH, adding all the necessary build fixes right away makes review > harder... One thing about your approach that makes it hard to review is that the first commit obliterates all of our local changes, and then you have to re-apply them individually. Looking at "git log" there aren't very many in this case, so it's pretty easy to be sure you got them all (in some cases this can be particularly nasty if the changes were themselves part of conflict resolution, and so you have to pick them out of a merge). I think a flow that better matches "what really happened" is to do more of a vendor-branch approach: have a line of history that represents the upstream changes (each one obliterating the last), and then periodically merge that into our fork. That can even retain bisectability as long as the commits along the vendor branch don't actually try to build the code. Unfortunately our initial import does try to build, so I think it already wrecks bisectability, but we can undo that now. So e.g.,: # start at e831171d67 (Add obstack.[ch] from EGLIBC 2.10, 2011-08-21) git checkout -b upstream-obstack e831171d67 # undo build changes to restore bisection; ideally this would have # been done back then, but it's too late now sed -i /obstack/d Makefile git commit -am 'strip out obstack building' # but of course in our merged version we want that back, so let's # do a noop merge to represent that. git checkout master ;# or whatever feature branch you're working on git merge -s ours upstream-obstack # and now with a sane vendor branch established, we can proceed to do # a real update there git checkout upstream-obstack cp /path/to/obstack.[ch] compat/ git commit -am 'update obstack' # and now we are free to merge that in, getting a real 3-way merge # between our changes and what happened upstream. git checkout master git merge upstream-obstack Now, if you try this you may find that the conflicts are pretty horrid. And the result may end up way less readable than your cherry-picks (and harder to resolve in the first place). I claim only that: 1. This represents in the history graph more clearly the actual sequence of events. 2. Its saves you from digging up the set of changes that have been applied since our last upstream import. So in this case the way you did it may well be the best way. But I offer it as an alternative. :) -Peff
SZEDER Gábor <szeder.dev@gmail.com> writes: > And here is an all-green build of these patches on Travis CI: > > https://travis-ci.org/szeder/git/builds/545645247 > > (and one bonus patch on top to deal with some Homebrew nonsense) Is this the one that making all of the jobs pass in the above output, including the mac gcc one. It would be wonderful to have it separately and fast-tracked ;-) -- >8 -- From: SZEDER Gábor <szeder.dev@gmail.com> Date: Wed, 3 Apr 2019 02:49:47 +0200 Subject: [PATCH] ci: make Homebrew's operations faster --- ci/install-dependencies.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh index 7f6acdd803..f804b40ddd 100755 --- a/ci/install-dependencies.sh +++ b/ci/install-dependencies.sh @@ -34,7 +34,7 @@ linux-clang|linux-gcc) popd ;; osx-clang|osx-gcc) - brew update >/dev/null + export HOMEBREW_NO_INSTALL_CLEANUP=1 HOMEBREW_NO_AUTO_UPDATE=1 # Uncomment this if you want to run perf tests: # brew install gnu-time test -z "$BREW_INSTALL_PACKAGES" ||
On 14/06/2019 11:00, SZEDER Gábor wrote: > Update 'compat/obstack.{c,h}' from upstream, because they already use > 'size_t' instead of 'long' in places that might eventually end up as > an argument to malloc(), which might solve build errors with GCC 8 on > Windows. > > The first patch just imports from upstream and doesn't modify anything > at all, and, consequently, it can't be compiled because of a screenful > or two of errors. This is bad for future bisects, of course. > > OTOH, adding all the necessary build fixes right away makes review > harder... > > I'm not sure how to deal with this situation, so here is a series with > the fixes in separate patches for review, for now. If there's an > agreement that this is the direction to take, then I'll squash in the > fixes in the first patch and touch up the resulting commit message. > > > Ramsay, could you please run sparse on top of these patch series to > make sure that I caught and converted all "0 instead of NULL" usages > in the last patch? Thanks. I applied your patches to current master (@0aae918dd9) and, since you dropped the final hunk of commit 3254310863 ("obstack.c: Fix some sparse warnings", 2011-09-11), sparse complains, thus: $ diff sp-out sp-out1 27a28,30 > compat/obstack.c:331:5: warning: incorrect type in initializer (different modifiers) > compat/obstack.c:331:5: expected void ( *[addressable] [toplevel] obstack_alloc_failed_handler )( ... ) > compat/obstack.c:331:5: got void ( [noreturn] * )( ... ) $ So, yes you did catch all "using plain integer as NULL pointer" warnings! :-D Thanks. ATB, Ramsay Jones
On 14/06/2019 21:30, Ramsay Jones wrote: > > > On 14/06/2019 11:00, SZEDER Gábor wrote: >> Update 'compat/obstack.{c,h}' from upstream, because they already use >> 'size_t' instead of 'long' in places that might eventually end up as >> an argument to malloc(), which might solve build errors with GCC 8 on >> Windows. >> >> The first patch just imports from upstream and doesn't modify anything >> at all, and, consequently, it can't be compiled because of a screenful >> or two of errors. This is bad for future bisects, of course. >> >> OTOH, adding all the necessary build fixes right away makes review >> harder... >> >> I'm not sure how to deal with this situation, so here is a series with >> the fixes in separate patches for review, for now. If there's an >> agreement that this is the direction to take, then I'll squash in the >> fixes in the first patch and touch up the resulting commit message. >> >> >> Ramsay, could you please run sparse on top of these patch series to >> make sure that I caught and converted all "0 instead of NULL" usages >> in the last patch? Thanks. > > I applied your patches to current master (@0aae918dd9) and, since > you dropped the final hunk of commit 3254310863 ("obstack.c: Fix > some sparse warnings", 2011-09-11), sparse complains, thus: > > $ diff sp-out sp-out1 > 27a28,30 > > compat/obstack.c:331:5: warning: incorrect type in initializer (different modifiers) > > compat/obstack.c:331:5: expected void ( *[addressable] [toplevel] obstack_alloc_failed_handler )( ... ) > > compat/obstack.c:331:5: got void ( [noreturn] * )( ... ) > $ > > So, yes you did catch all "using plain integer as NULL pointer" > warnings! :-D Sorry for being a bit slow here, but I just realized that I should not have seen that on Linux (and should have tested on cygwin), because the obstack code gets elided on Linux ... Oh, wait: $ diff sc sc1 3a4,7 > compat/obstack.o - _obstack_allocated_p > compat/obstack.o - obstack_alloc_failed_handler > compat/obstack.o - _obstack_begin_1 > compat/obstack.o - _obstack_memory_used $ Hmm, so on master, this code is totally elided on Linux, but that is no longer the case with your patches applied. I guess you need to look at the definition of the {_OBSTACK_}ELIDE_CODE preprocessor variable(s). HTH. ATB, Ramsay Jones
On Fri, Jun 14, 2019 at 09:30:20PM +0100, Ramsay Jones wrote: > > > On 14/06/2019 11:00, SZEDER Gábor wrote: > > Update 'compat/obstack.{c,h}' from upstream, because they already use > > 'size_t' instead of 'long' in places that might eventually end up as > > an argument to malloc(), which might solve build errors with GCC 8 on > > Windows. > > > > The first patch just imports from upstream and doesn't modify anything > > at all, and, consequently, it can't be compiled because of a screenful > > or two of errors. This is bad for future bisects, of course. > > > > OTOH, adding all the necessary build fixes right away makes review > > harder... > > > > I'm not sure how to deal with this situation, so here is a series with > > the fixes in separate patches for review, for now. If there's an > > agreement that this is the direction to take, then I'll squash in the > > fixes in the first patch and touch up the resulting commit message. > > > > > > Ramsay, could you please run sparse on top of these patch series to > > make sure that I caught and converted all "0 instead of NULL" usages > > in the last patch? Thanks. > > I applied your patches to current master (@0aae918dd9) and, since > you dropped the final hunk of commit 3254310863 ("obstack.c: Fix > some sparse warnings", 2011-09-11), sparse complains, thus: Oh, indeed. 3254310863 removed that "__attribute__ ((noreturn))" from the function's definition, but nowadays upstream writes that as "static _Noreturn void print_and_abort (void)", and I didn't realize that this _Noreturn is the same thing. > $ diff sp-out sp-out1 > 27a28,30 > > compat/obstack.c:331:5: warning: incorrect type in initializer (different modifiers) > > compat/obstack.c:331:5: expected void ( *[addressable] [toplevel] obstack_alloc_failed_handler )( ... ) > > compat/obstack.c:331:5: got void ( [noreturn] * )( ... ) > $ > > So, yes you did catch all "using plain integer as NULL pointer" > warnings! :-D Heh :) Anyway, I won't do anything for the time being, in the hope that we can get on board with removing kwset/obstack...
Update 'compat/obstack.{c,h}' from upstream, because they already use 'size_t' instead of 'long' in places that might eventually end up as an argument to malloc(), which might solve build errors with GCC 8 on Windows. The first patch just imports from upstream and doesn't modify anything at all, and, consequently, it can't be compiled because of a screenful or two of errors. This is bad for future bisects, of course. OTOH, adding all the necessary build fixes right away makes review harder... I'm not sure how to deal with this situation, so here is a series with the fixes in separate patches for review, for now. If there's an agreement that this is the direction to take, then I'll squash in the fixes in the first patch and touch up the resulting commit message. Ramsay, could you please run sparse on top of these patch series to make sure that I caught and converted all "0 instead of NULL" usages in the last patch? Thanks. And here is an all-green build of these patches on Travis CI: https://travis-ci.org/szeder/git/builds/545645247 (and one bonus patch on top to deal with some Homebrew nonsense) SZEDER Gábor (4): compat/obstack: update from upstream SQUASH??? compat/obstack: fix portability issues SQUASH??? compat/obstack: fix build errors with Clang compat/obstack: fix some sparse warnings compat/obstack.c | 356 ++++++++------------ compat/obstack.h | 832 ++++++++++++++++++++++++----------------------- 2 files changed, 572 insertions(+), 616 deletions(-)