| Message ID | 0dec6e1e-207c-be13-ae95-294d9b1e8831@ramsayjones.plus.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Makefile: fix cygwin build failure | expand |
Hi Ramsay, On Wed, Nov 09, 2022 at 10:46:05PM +0000, Ramsay Jones wrote: > Commit 1c97a5043f (Makefile: define "TEST_{PROGRAM,OBJS}" variables > earlier, 2022-10-31) breaks the cygwin build, like so: It seems reasonable to me, and I'd like to pick it up rather quickly (on top of Ævar's branch), especially if this is going to break things downstream in Git for Windows. Ævar: this sort of change is a little tricky to review without more diff context ;-). Do you have any objections to me slotting this on top of your branch? Thanks, Taylor
On Wed, Nov 09 2022, Taylor Blau wrote: > Hi Ramsay, > > On Wed, Nov 09, 2022 at 10:46:05PM +0000, Ramsay Jones wrote: >> Commit 1c97a5043f (Makefile: define "TEST_{PROGRAM,OBJS}" variables >> earlier, 2022-10-31) breaks the cygwin build, like so: > > It seems reasonable to me, and I'd like to pick it up rather quickly (on > top of Ævar's branch), especially if this is going to break things > downstream in Git for Windows. > > Ævar: this sort of change is a little tricky to review without more diff > context ;-). Do you have any objections to me slotting this on top of > your branch? Yes, I've reviewed this, sorry about missing this edge case. This fix & analysis looks solid to me (it's still just in "seen", right?) FWIW I think a more thorough fix for it would be to future-proof this sort of IMMEDIATE expansion by defining such core variables earlier: -- >8 -- Subject: [PATCH] Makefile & make "uname" and "$X" available earlier In [1] I broke the build on Cygwin, because by moving "all:: $(TEST_PROGRAMS)" earlier in the file (around line 800) it was declared before around line ~1300, where we include "config.mak.uname", and that's where we'll set "X = .exe" on Cygwin and Windows. Moving the "all" line down[2] is a more narrow fix for this, but this attempts to make this sort of thing safer in the future. We'll now load a "config.mak.uname-early" (really within the first 100 lines of code, but there's a giant comment at the top). This ensures that in the future any Makefile rules that have "IMMEDIATE" expansion (e.g. the RHS of a "rule" will work as expected if they use $(X), not just if they use the "DEFERRED" expansion (which e.g. "=" assignment uses). See [3] in the GNU make documentation for details. 1. 1c97a5043f8 (Makefile: define "TEST_{PROGRAM,OBJS}" variables earlier, 2022-10-31) 2. https://lore.kernel.org/git/0dec6e1e-207c-be13-ae95-294d9b1e8831@ramsayjones.plus.com/ 3. https://www.gnu.org/software/make/manual/html_node/Reading-Makefiles.html Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- Makefile | 9 ++++++--- config.mak.uname | 7 ------- config.mak.uname-early | 33 +++++++++++++++++++++++++++++++++ 3 files changed, 39 insertions(+), 10 deletions(-) create mode 100644 config.mak.uname-early diff --git a/Makefile b/Makefile index 4927379184c..e31678e0547 100644 --- a/Makefile +++ b/Makefile @@ -621,6 +621,12 @@ TEST_OBJS = TEST_PROGRAMS_NEED_X = THIRD_PARTY_SOURCES = +# Binary suffix, set to .exe for Windows builds +X = +# Make $(uname_*) variables available, and possibly change $X to +# ".exe" (on Windows) +include config.mak.uname-early + # Having this variable in your environment would break pipelines because # you cause "cd" to echo its destination to stdout. It can also take # scripts to unexpected places. If you like CDPATH, define it for your @@ -714,9 +720,6 @@ PROGRAM_OBJS += shell.o .PHONY: program-objs program-objs: $(PROGRAM_OBJS) -# Binary suffix, set to .exe for Windows builds -X = - PROGRAMS += $(patsubst %.o,git-%$X,$(PROGRAM_OBJS)) TEST_BUILTINS_OBJS += test-advise.o diff --git a/config.mak.uname b/config.mak.uname index d63629fe807..616fa9052e2 100644 --- a/config.mak.uname +++ b/config.mak.uname @@ -16,10 +16,6 @@ ifneq ($(findstring MINGW,$(uname_S)),) endif ifdef MSVC - # avoid the MingW and Cygwin configuration sections - uname_S := Windows - uname_O := Windows - # Generate and include makefile variables that point to the # currently installed set of MSVC command line tools. compat/vcbuild/MSVC-DEFS-GEN: compat/vcbuild/find_vs_env.bat @@ -238,7 +234,6 @@ ifeq ($(uname_O),Cygwin) NEEDS_LIBICONV = YesPlease NO_FAST_WORKING_DIRECTORY = UnfortunatelyYes NO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease - X = .exe UNRELIABLE_FSTAT = UnfortunatelyYes OBJECT_CREATION_USES_RENAMES = UnfortunatelyNeedsTo MMAP_PREVENTS_DELETE = UnfortunatelyYes @@ -523,7 +518,6 @@ ifndef DEBUG else BASIC_CFLAGS += -MDd -DDEBUG -D_DEBUG endif - X = .exe compat/msvc.o: compat/msvc.c compat/mingw.c GIT-CFLAGS endif @@ -676,7 +670,6 @@ ifeq ($(uname_S),MINGW) PTHREAD_LIBS = RC = windres -O coff NATIVE_CRLF = YesPlease - X = .exe ifneq (,$(wildcard ../THIS_IS_MSYSGIT)) htmldir = doc/git/html/ prefix = diff --git a/config.mak.uname-early b/config.mak.uname-early new file mode 100644 index 00000000000..000d490a506 --- /dev/null +++ b/config.mak.uname-early @@ -0,0 +1,33 @@ +# This is mainly used by config.mak.uname-early, but we load it much +# earlier to get access to $(X). + +uname_S := $(shell sh -c 'uname -s 2>/dev/null || echo not') +uname_M := $(shell sh -c 'uname -m 2>/dev/null || echo not') +uname_O := $(shell sh -c 'uname -o 2>/dev/null || echo not') +uname_R := $(shell sh -c 'uname -r 2>/dev/null || echo not') +uname_P := $(shell sh -c 'uname -p 2>/dev/null || echo not') +uname_V := $(shell sh -c 'uname -v 2>/dev/null || echo not') + +ifneq ($(findstring MINGW,$(uname_S)),) + uname_S := MINGW +endif + +ifdef MSVC + # avoid the MingW and Cygwin configuration sections in + # config.mak.uname + uname_S := Windows + uname_O := Windows +endif + + +ifeq ($(uname_S),MINGW) + X = .exe +else +ifeq ($(uname_S),Windows) + X = .exe +else +ifeq ($(uname_O),Cygwin) + X = .exe +endif +endif +endif
On Thu, Nov 10, 2022 at 12:18:01AM +0100, Ævar Arnfjörð Bjarmason wrote: > > On Wed, Nov 09 2022, Taylor Blau wrote: > > > Hi Ramsay, > > > > On Wed, Nov 09, 2022 at 10:46:05PM +0000, Ramsay Jones wrote: > >> Commit 1c97a5043f (Makefile: define "TEST_{PROGRAM,OBJS}" variables > >> earlier, 2022-10-31) breaks the cygwin build, like so: > > > > It seems reasonable to me, and I'd like to pick it up rather quickly (on > > top of Ævar's branch), especially if this is going to break things > > downstream in Git for Windows. > > > > Ævar: this sort of change is a little tricky to review without more diff > > context ;-). Do you have any objections to me slotting this on top of > > your branch? > > Yes, I've reviewed this, sorry about missing this edge case. This fix & > analysis looks solid to me (it's still just in "seen", right?) Yes, 'ab/remove--super-prefix' is only in seen for now. So that we don't break the Cygwin build in the middle of history, could you send a reroll of that topic that incorporates this patch squashed into the right location so that each step builds independently? Thanks, Taylor
On Wed, Nov 09, 2022 at 09:20:28PM -0500, Taylor Blau wrote:
> Yes, 'ab/remove--super-prefix' is only in seen for now.
Oops, I clearly meant 'ab/make-bin-wrappers' here. Sorry about that.
Thanks,
Taylor
On Wed, Nov 09, 2022 at 09:21:23PM -0500, Taylor Blau wrote: > On Wed, Nov 09, 2022 at 09:20:28PM -0500, Taylor Blau wrote: > > Yes, 'ab/remove--super-prefix' is only in seen for now. > > Oops, I clearly meant 'ab/make-bin-wrappers' here. Sorry about that. ...Double oops. Now that I think about it, my notes show that we were planning on dropping this topic per the discussion beginning at [1]. If that's not the case and you were planning on resending this topic, then incorporating these changes in would be appreciated. If not, I think we can drop both of these. Thanks, Taylor [1]: https://lore.kernel.org/git/Y2rPrNz6BD6DlRcO@nand.local/
On Wed, 9 Nov 2022 at 22:46, Ramsay Jones wrote: > <snip> > [1] After a cygwin update, '/usr/bin/msgfmt.exe' refused to run, saying > that it could not locate the 'cygunistring-5.dll' file. Using cygcheck, > I found that this dll is provided by the 'libunistring5 1.1-1' package. > After installing this package, everything works just fine. > > I don't know how package dependencies are specified/updated, but it > would seem the 'gettext-devel' package has a direct or indirect > dependency on the 'libunistring5' package. Looking at my setup.log file > I would guess one-or-more of the following packages needs an update to > note this dependency: 'gettext-devel 0.21.1-1', 'gettext 0.21.1-1', > 'libgettextpo0 0.21.1-1', 'libintl-devel 0.21.1-1', 'libintl8 0.21.1-1', > or 'libasprintf0 0.21.1-1'. > > Unfortunately, I am not subscribed to the cygwin mailinglist :( It looks like this was broken by an update to some of these packages a couple of weeks ago[0]; I've reproduced the problem and reported it to the mailing list[1], so the package maintainer should be able to update the dependency information :) [0]: https://cygwin.com/pipermail/cygwin-announce/2022-October/010764.html [1]: https://cygwin.com/pipermail/cygwin/2022-November/252445.html
On 10/11/2022 02:22, Taylor Blau wrote: > On Wed, Nov 09, 2022 at 09:21:23PM -0500, Taylor Blau wrote: >> On Wed, Nov 09, 2022 at 09:20:28PM -0500, Taylor Blau wrote: >>> Yes, 'ab/remove--super-prefix' is only in seen for now. >> >> Oops, I clearly meant 'ab/make-bin-wrappers' here. Sorry about that. > > ...Double oops. Now that I think about it, my notes show that we were > planning on dropping this topic per the discussion beginning at [1]. > > If that's not the case and you were planning on resending this topic, > then incorporating these changes in would be appreciated. If not, I > think we can drop both of these. > > Thanks, > Taylor > > [1]: https://lore.kernel.org/git/Y2rPrNz6BD6DlRcO@nand.local/ I noticed my origin server: $ git config remote.origin.url git://git.kernel.org/pub/scm/git/git.git $ .. updated tonight, so I fetched, built, and noticed cygwin is still broken! :( If the 'ab/make-bin-wrappers' branch is going to stay, could you please squash my patch[1] into (or put it on top of) this branch. Thanks! ATB, Ramsay Jones [1] https://lore.kernel.org/git/0dec6e1e-207c-be13-ae95-294d9b1e8831@ramsayjones.plus.com/
Taylor Blau <me@ttaylorr.com> writes: > On Wed, Nov 09, 2022 at 09:21:23PM -0500, Taylor Blau wrote: >> On Wed, Nov 09, 2022 at 09:20:28PM -0500, Taylor Blau wrote: >> > Yes, 'ab/remove--super-prefix' is only in seen for now. >> >> Oops, I clearly meant 'ab/make-bin-wrappers' here. Sorry about that. > > ...Double oops. Now that I think about it, my notes show that we were > planning on dropping this topic per the discussion beginning at [1]. OK, let's discard it for now.
diff --git a/Makefile b/Makefile index acb02f3882..03ceaf3e79 100644 --- a/Makefile +++ b/Makefile @@ -869,7 +869,6 @@ TEST_PROGRAMS_NEED_X += test-fake-ssh TEST_PROGRAMS_NEED_X += test-tool TEST_PROGRAMS = $(patsubst %,t/helper/%$X,$(TEST_PROGRAMS_NEED_X)) -all:: $(TEST_PROGRAMS) TEST_PROGRAM_OBJS += $(patsubst %,t/helper/%.o,$(TEST_PROGRAMS_NEED_X)) .PRECIOUS: $(TEST_PROGRAM_OBJS) @@ -3208,7 +3207,7 @@ $(call bin_wrappers_template,TEST_PROGRAMS_NEED_X,'$$(@F)',t/helper/,$$X) endef $(eval $(call bin_wrappers_templates)) -all:: $(BIN_WRAPPERS) +all:: $(TEST_PROGRAMS) $(BIN_WRAPPERS) # GNU make supports exporting all variables by "export" without parameters. # However, the environment gets quite big, and some programs have problems
Commit 1c97a5043f (Makefile: define "TEST_{PROGRAM,OBJS}" variables earlier, 2022-10-31) breaks the cygwin build, like so: $ make GIT_VERSION = 2.38.1.674.gca75de31c9 * new build flags CC oss-fuzz/fuzz-commit-graph.o CC oss-fuzz/fuzz-pack-headers.o CC oss-fuzz/fuzz-pack-idx.o make: *** No rule to make target 't/helper/test-fake-ssh', needed by 'all'. Stop. $ This is caused by moving an 'all::' target higher in the Makefile, before the 'include' of the config.mak.* files. This results in the $X make variable having the default value (empty) rather than a value suitable for cygwin (ie. '.exe'). Although the value of this variable is lazily evaluated, the 'all::' target forces an evaluation prior to it being correctly set. In order to fix the build, move the 'all::' target lower in the Makefile (close to where it was originally placed). Although it could come anywhere after the 'include's, placing it here makes the diff of the build outputs smaller (placing it directly after the 'include's causes a change in the order of build products). Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com> --- Hi Taylor, Ævar, I decided to spend some time tonight cleaning up my cygwin git repo (I have _far_ too many branches scattered across history, some _very_ old, some half done, some with no commit message etc., etc.). So, before looking to rebase/re-work some old commits on the current version of git, I thought it might be an idea to peek at the latest work; I have not seen any updates to my origin repo, for the last couple of weeks, since Junio took a break (as you may recall :) ). So, I set up a 'git' remote: $ git remote get-url git git@github.com:git/git.git $ .. to have a peek at the updates to the 'master', 'next' and 'seen' branches. Also, out of habit, I decided to build all three branches in the usual way (well, apart from being on a detached HEAD, of course). First 'git/master' failed to build! ;) It turned out that I had updated cygwin and installed a bad update to the gettext-devel package, in particular '/usr/bin/msgfmt.exe' was completely broken. :( (Adam, if you haven't already run into this, you may appreciate the solution given at [1] below). Having fixed my cygwin installation, 'git/master' and 'git/next' built just fine, 'git/seen' however failed to build (see commit message above). I'm not sure what the plans are for the 'ab/make-bin-wrappers' branch, but if it is going to be re-rolled, could you please squash this into the patch corresponding to commit 1c97a5043f. (Otherwise, could you maybe add this to the tip of that branch?). Note: this patch was created directly on top of 'git/seen'@ca75de31c9, but I wouldn't anticipate any problem adding it to that branch. I am a little surprised that it has taken this long to spot this build failure, since this should also affect a (vanilla) MSYS2 build, along with a Git-For-Windows Makefile build. Hmm, I have no way of knowing, but this seems to indicate that nobody builds GFW using the Makefile these days! :D If anything in the commit message is unclear, please let me know. Thanks! ATB, Ramsay Jones [1] After a cygwin update, '/usr/bin/msgfmt.exe' refused to run, saying that it could not locate the 'cygunistring-5.dll' file. Using cygcheck, I found that this dll is provided by the 'libunistring5 1.1-1' package. After installing this package, everything works just fine. I don't know how package dependencies are specified/updated, but it would seem the 'gettext-devel' package has a direct or indirect dependency on the 'libunistring5' package. Looking at my setup.log file I would guess one-or-more of the following packages needs an update to note this dependency: 'gettext-devel 0.21.1-1', 'gettext 0.21.1-1', 'libgettextpo0 0.21.1-1', 'libintl-devel 0.21.1-1', 'libintl8 0.21.1-1', or 'libasprintf0 0.21.1-1'. Unfortunately, I am not subscribed to the cygwin mailinglist :( Makefile | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)