Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Makefile.mk: portable Makefile.m32 #9764

Closed
wants to merge 58 commits into from
Closed

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Oct 18, 2022

Update Makefile.m32 to:

  • move objects into a subdirectory.
  • add support for MS-DOS. Tested with DJGPP.
  • add support for Watt-32 (on MS-DOS).
  • add support for AmigaOS.
  • replace ARCH with TRIPLET.
  • build tool_hugehelp.c proper (when tools are available).
  • drop MS-DOS compatibility macro USE_ZLIB (replaced by HAVE_LIBZ)
  • add support for ZLIB_LIBS to override -lz.
  • omit object files when building examples.
  • default CC to gcc once again, for convenience. (Caveat: compiler name cc cannot be set now.)
  • set -DCURL_NO_OLDIES for examples, like autotools does.
  • delete makefile.dj files. Notice the configuration details and defaults are not retained with the new method.
  • delete makefile.amiga files. A successful build needs a few custom options. We're also not retaining all build details from the existing Amiga make files.
  • rename Makefile.m32 to Makefile.mk to reflect that they are not Windows/MinGW32-specific anymore.
  • add support for new CFG options: -map, -debug, -trackmem
  • set -DNDEBUG by default.
  • allow using -DOS=... in all lib/config-*.h headers, syncing this with config-win32.h.
  • look for zlib parts in $ZLIB_PATH/include and $ZLIB_PATH/lib instead of bare $ZLIB_PATH.
  • set -DOS= with the manually set or auto-detected TRIPLET value.

Merged separately:

Closes #xxxx

More readable diff without whitespaces: https://github.com/curl/curl/pull/9764/files?w=1

[ci skip]

@vszakats vszakats added build feature-window A merge of this requires an open feature window tidy-up labels Oct 18, 2022
@vszakats
Copy link
Member Author

vszakats commented Oct 18, 2022

Details to produce an Amiga OS build without compiler and linker errors:

export CROSSPREFIX=/opt/appveyor/build-agent/opt/amiga/bin/m68k-amigaos-
export CC=gcc
export CPPFLAGS='-DHAVE_PROTO_BSDSOCKET_H'
export CFLAGS='-mcrt=clib2'
export LDFLAGS="${CFLAGS}"
export LIBS='-lnet -lm'
make -C lib -f Makefile.mk
make -C src -f Makefile.mk

Warnings remaining:

multi.c: In function 'curl_multi_wakeup':
multi.c:1534:10: warning: initialization discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
       if(wakeup_write(multi->wakeup_pair[1], buf, sizeof(buf)) < 0) {
          ^~~~~~~~~~~~
sendf.c: In function 'Curl_send_plain':
sendf.c:371:21: warning: initialization discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
     bytes_written = swrite(sockfd, mem, len);
                     ^~~~~~
telnet.c: In function 'send_negotiation':
telnet.c:313:19: warning: initialization discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
   bytes_written = swrite(conn->sock[FIRSTSOCKET], buf, 3);
                   ^~~~~~
telnet.c: In function 'suboption':
telnet.c:896:23: warning: initialization discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
       bytes_written = swrite(conn->sock[FIRSTSOCKET], temp, len);
                       ^~~~~~
telnet.c:908:23: warning: initialization discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
       bytes_written = swrite(conn->sock[FIRSTSOCKET], temp, len);
                       ^~~~~~
telnet.c:941:23: warning: initialization discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
       bytes_written = swrite(conn->sock[FIRSTSOCKET], temp, len);
                       ^~~~~~
telnet.c: In function 'sendsuboption':
telnet.c:995:21: warning: initialization discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
     bytes_written = swrite(conn->sock[FIRSTSOCKET], tn->subbuffer, 3);
                     ^~~~~~
telnet.c:1004:21: warning: initialization discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
     bytes_written = swrite(conn->sock[FIRSTSOCKET], tn->subbuffer + 7, 2);
                     ^~~~~~

curl linked successfully, it also launched without errors in an Amiga emulator, produced the first line of -V output, then crashed (seems to be a stack overflow after any single, successful printf() call.)

Screen Shot

-W -Wall warnings:

amigaos.c: In function 'Curl_amiga_init':
amigaos.c:217:18: warning: pointer targets in initialization differ in signedness [-Wpointer-sign]
     SocketBase = OpenLibrary("bsdsocket.library", 4);
                  ^~~~~~~~~~~
amigaos.c:220:5: warning: pointer targets in initialization differ in signedness [-Wpointer-sign]
     __request("No TCP/IP Stack running!");
     ^~~~~~~~~
amigaos.c:227:5: warning: pointer targets in initialization differ in signedness [-Wpointer-sign]
     __request("SocketBaseTags ERROR");
     ^~~~~~~~~
ftp.c: In function 'ftp_state_mdtm_resp':
ftp.c:2100:31: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]
          (data->info.filetime >= 0) ) {
                               ^~
getinfo.c: In function 'getinfo_long':
getinfo.c:238:33: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
     else if(data->info.filetime < LONG_MIN)
                                 ^
hostip.c: In function 'hostcache_timestamp_remove':
hostip.c:208:34: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
     && (data->now - c->timestamp >= data->cache_timeout);
                                  ^~
http.c: In function 'Curl_http_header':
http.c:3591:13: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
       if(-1 != date)
             ^~
parsedate.c: In function 'parsedate':
parsedate.c:520:24: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
   if((tzoff > 0) && (t > TIME_T_MAX - tzoff)) {
                        ^
parsedate.c: In function 'curl_getdate':
parsedate.c:548:15: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
     if(parsed == -1)
               ^~
parsedate.c: In function 'Curl_getdate_capped':
parsedate.c:568:15: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
     if(parsed == -1)
               ^~
tool_operate.c: In function 'AmigaSetComment':
tool_operate.c:343:5: warning: pointer targets in initialization differ in signedness [-Wpointer-sign]
     SetComment(outs->filename, per->this_url);
     ^~~~~~~~~~
tool_operate.c:343:5: warning: pointer targets in initialization differ in signedness [-Wpointer-sign]

@vszakats vszakats force-pushed the m32port2 branch 4 times, most recently from 801f29a to 0c7c999 Compare October 22, 2022 23:48
@vszakats vszakats changed the title Makefile.m32: extend to other platforms [ci skip] Makefile.gcc: extend Makefile.m32 to other platforms [ci skip] Oct 23, 2022
@vszakats vszakats changed the title Makefile.gcc: extend Makefile.m32 to other platforms [ci skip] Makefile.gcc: make Makefile.m32 portable [ci skip] Oct 23, 2022
@vszakats vszakats changed the title Makefile.gcc: make Makefile.m32 portable [ci skip] Makefile.gcc: make Makefile.m32 portable Oct 23, 2022
@vszakats vszakats changed the title Makefile.gcc: make Makefile.m32 portable Makefile.mk: make Makefile.m32 portable Oct 24, 2022
@vszakats vszakats changed the title Makefile.mk: make Makefile.m32 portable Makefile.mk: portable Makefile.m32 Oct 25, 2022
@vszakats vszakats force-pushed the m32port2 branch 2 times, most recently from 3e438c3 to 7e1dd3b Compare October 26, 2022 09:19
@vszakats vszakats removed the feature-window A merge of this requires an open feature window label Nov 1, 2022
@vszakats
Copy link
Member Author

vszakats commented Nov 8, 2022

This PR is ready at this point. Comments are welcome!

My main question is if we're fundamentally okay with merging MS-DOS, Amiga and MinGW standalone Makefiles into one. IMO this makes things cleaner and simpler to maintain, but it will unavoidably affect DJGPP and Amiga users who used the standalone Makefiles. In those cases the configuration will need to be changed. This isn't terribily complicated and there should be no loss in functionality, but it needs to be done nevertheless, and there is a non-zero chance for fallouts.

On the other hand it reduces the number of distinct build methods. It also makes testing MS-DOS and Amiga builds simpler, as they now align perfectly with the MinGW (standalone = m32) ones.

@jay
Copy link
Member

jay commented Nov 11, 2022

I'm fine with this, though I see hardly any documentation changes. Shouldn't there be a notice or a section for Amiga or MS-DOS users that build this way, to let them know of the new makefile?

@vszakats
Copy link
Member Author

@jay: Yes, that would be a nice touch. Though Amiga did not have an entry in INSTALL.md before, nor did MS-DOS, but the relevant parts of packages/DOS/README would be best copied in there. I've made a commit, let's see how it improves things.

[ Also fixed for msdos and amiga so that CFG values propagate just like with mingw32. ]

@jay
Copy link
Member

jay commented Nov 11, 2022

lgtm. @gvanem what do you think?

@vszakats
Copy link
Member Author

vszakats commented Nov 12, 2022

Details to produce MS-DOS binaries (from a *NIX env):

Get dev env:

cd /opt
# For other platforms, see: https://github.com/andrewwutw/build-djgpp
curl -L https://github.com/andrewwutw/build-djgpp/releases/download/v3.3/djgpp-osx-gcc1210.tar.bz2 | tar -x
(
  cd djgpp
  curl -LO https://mirrorservice.org/sites/ftp.delorie.com/pub/djgpp/current/v2tk/wat3211b.zip
  curl -LO https://mirrorservice.org/sites/ftp.delorie.com/pub/djgpp/current/v2tk/zlb1212b.zip
  curl -LO https://mirrorservice.org/sites/ftp.delorie.com/pub/djgpp/current/v2tk/ssl102ub.zip
  unzip wat3211b.zip
  unzip zlb1212b.zip
  unzip ssl102ub.zip
)

Build:

export WATT_PATH=/opt/djgpp/net/watt
export ZLIB_PATH=/opt/djgpp
export OPENSSL_PATH=/opt/djgpp
export PATH="${PATH}:/opt/djgpp/bin/"
make -f Makefile.dist djgpp-zlib-ssl

or

export WATT_PATH=/opt/djgpp/net/watt
export ZLIB_PATH=/opt/djgpp
export OPENSSL_PATH=/opt/djgpp
export CROSSPREFIX=/opt/djgpp/bin/i586-pc-msdosdjgpp-
export CFG=-zlib-ssl
make -C lib -f Makefile.mk
make -C src -f Makefile.mk

-W -Wall warnings::

ftp.c: In function 'ftp_state_mdtm_resp':
ftp.c:2099:31: warning: comparison of unsigned expression in '>= 0' is always true [-Wtype-limits]
 2099 |          (data->info.filetime >= 0) ) {
      |                               ^~
getinfo.c: In function 'getinfo_long':
getinfo.c:238:33: warning: comparison of integer expressions of different signedness: 'time_t' {aka 'unsigned int'} and 'long int' [-Wsign-compare]
  238 |     else if(data->info.filetime < LONG_MIN)
      |                                 ^
hostip.c: In function 'hostcache_timestamp_remove':
hostip.c:204:34: warning: comparison of integer expressions of different signedness: 'time_t' {aka 'unsigned int'} and 'long int' [-Wsign-compare]
  204 |     && (data->now - c->timestamp >= data->cache_timeout);
      |                                  ^~
http.c: In function 'Curl_http_header':
http.c:3591:13: warning: comparison of integer expressions of different signedness: 'int' and 'time_t' {aka 'unsigned int'} [-Wsign-compare]
 3591 |       if(-1 != date)
      |             ^~
parsedate.c: In function 'parsedate':
parsedate.c:520:24: warning: comparison of integer expressions of different signedness: 'time_t' {aka 'unsigned int'} and 'int' [-Wsign-compare]
  520 |   if((tzoff > 0) && (t > TIME_T_MAX - tzoff)) {
      |                        ^
parsedate.c: In function 'curl_getdate':
parsedate.c:548:15: warning: comparison of integer expressions of different signedness: 'time_t' {aka 'unsigned int'} and 'int' [-Wsign-compare]
  548 |     if(parsed == -1)
      |               ^~
parsedate.c: In function 'Curl_getdate_capped':
parsedate.c:568:15: warning: comparison of integer expressions of different signedness: 'time_t' {aka 'unsigned int'} and 'int' [-Wsign-compare]
  568 |     if(parsed == -1)
      |               ^~

@vszakats
Copy link
Member Author

If there is any objection to this, I can update the PR to retain existing MS-DOS/Amiga build files and just keep most updates to Makefile.m32, including renaming them to .mk (perhaps without the MS-DOS / Amiga / less-used bits to keep it simple). If there is none, I plan to merge it in the next few days.

@MarcelRaad
Copy link
Member

I haven't looked at this in detail, but I do like reducing the number of different build methods 👍

vszakats added a commit to curl/curl-for-win that referenced this pull request Nov 21, 2022
Of course both autotools and cmake uses (or can use) GNU Make. Within
curl-for-win it means using raw/pure GNU Make without an extra generator
layer like cmake/autotools (or something proprietary).

This makes the term project agnostic. (E.g. curl uses `Makefile.m32`,
awaiting [1] a rename to `Makefile.mk`, libssh2 uses `GNUMakefile`,
etc.)

It's also more greppable and less ambiguous than some alternatives
('make', 'gmake', 'mk').

[1] curl/curl#9764
vszakats added a commit to curl/curl-for-win that referenced this pull request Nov 22, 2022
With the upstream PR merge, adapt our script for the changes.
Meaning simpler local configuration and using `Makefile.mk`
instead of `Makefile.m32`.

Ref: curl/curl#9764
@gvanem
Copy link
Contributor

gvanem commented Nov 27, 2022

gcc/ld does not like this:

  CPPFLAGS += -I"$(WATT_PATH)/inc"
  _LDFLAGS += -L"$(WATT_PATH)/lib"

But this works:

  CPPFLAGS += -I$(WATT_PATH)/inc
  _LDFLAGS += -L$(WATT_PATH)/lib

@vszakats
Copy link
Member Author

vszakats commented Nov 27, 2022

In that case I'd prefer to revert and reapply without the MS-DOS (and AmigaOS) parts. Or just cherry-pick a few commits. Reason being: double quotes are everywhere for other dependencies, so fixing watt is not enough. Making double quotes a variable would make everything look unreadable.

Before I do so: What environment / gcc/ld version is where double-quotes are not recognized?

(In my tests, double-quotes worked without issues, though I only tested cross-builds, not actually running the build on MS-DOS.)

Also, would this work better?:

  CPPFLAGS += "-I$(WATT_PATH)/inc"
  _LDFLAGS += "-L$(WATT_PATH)/lib"

@gvanem
Copy link
Contributor

gvanem commented Nov 27, 2022

What environment / gcc/ld version is where double-quotes are not recognized?

I'm on Win-10 using djgpp's gcc ver. 12.

CPPFLAGS += "-I$(WATT_PATH)/inc"

That doesn't work either:

f:/gv/djgpp/windows/bin/i586-pc-msdosdjgpp-gcc -W -Wall  -I. -I../include -DNDEBUG "-If:/gv/net/watt/inc" -DBUILDING_LIBCURL -c altsvc.c -o
i586-pc-msdosdjgpp/altsvc.o
In file included from altsvc.c:28:
curl_setup.h:519:14: fatal error: tcp.h: No such file or directory
  519 | #    include <tcp.h>
      |              ^~~~~~~
compilation terminated.

@vszakats
Copy link
Member Author

Odd. Reverting.

@vszakats
Copy link
Member Author

If someone is in the know of an easy fix for these issues, it'd be a more desirable solution of course.

@vszakats
Copy link
Member Author

Another option is to give up supporting dependency paths with spaces. I don't like it, but don't mind either. Also, spaces weren't actively tested. This would undo this commit that added support back in 2011: a6c168b

@gvanem
Copy link
Contributor

gvanem commented Nov 27, 2022

Another option is to give up supporting dependency paths with spaces.

Agree. Having paths with spaces in SW-development is madness.

@jay
Copy link
Member

jay commented Nov 27, 2022

Sorry but I disagree. Quoting should work at least when the whole argument is quoted "-Ifoo". If it doesn't then that's a failure of the build system and not curl.

@vszakats
Copy link
Member Author

It's puzzling that a recent DJGPP, built for Windows, on a recent Windows cannot handle double-quoted parameters. I'm not in the position to make tests, but I'm curious how/why this can happen. Double-quoted parameters are supposed to work fine on Windows, last time I checked.

@gvanem
Copy link
Contributor

gvanem commented Nov 28, 2022

But single quotes works:

  CPPFLAGS += -I'$(WATT_PATH)/inc'
  _LDFLAGS += -L'$(WATT_PATH)/lib'

So perhaps some shell/make issue?

@vszakats
Copy link
Member Author

Interesting. In the classic Windows shell (pre-Win10 at least), single quotes do not work, while double-quotes do. In unixy shells, both should work. Did this change with newer Windows shell(s)? Also, things may be mixed up when using a GNU Make .exe built for a different environment (say Cygwin). Back then I had the best results with mingw32-make.exe.

I guess it'd be useful to know more about your particular env. GNU Make version and build, Windows version, shell, DJGPP version?

What's also strange, that these double-quotes were there for a decade+ and nobody reported and issue with it on Windows. This assumes that everyone run it under MSYS[2], or did not run it at all, both unlikely, or something else at play.

@gvanem
Copy link
Contributor

gvanem commented Nov 28, 2022

Add a --debug-all (to latest gnumake or mingw32-make), I see this passed to sh.exe:

CreateProcess(f:\CygWin64\bin\sh.exe,f:/CygWin64/bin/sh.exe 
-c "f:/gv/djgpp/windows/bin/i586-pc-msdosdjgpp-gcc -W -Wall -Wno-type-limits -Wno-sign-compare -I. -I../include 
-DNDEBUG -I\"f:/gv/net/watt/inc\" -DBUILDING_LIBCURL -c cfilters.c -o msdos/cfilters.o",...)
...

And issue with those escaped quotes (\")?

@vszakats
Copy link
Member Author

@gvanem Is it so that you're using a Cygwin shell with a non-Cygwin mingw32-make?

It'd be worth a try without using anything from Cygwin. Cygwin is a rat's nest of issues, especially when mixed in with non-Cygwin tools.

The MSYS2 shell would be a good match for mingw32-make.exe, or the plain Windows shell should work just as well I think.

@jay
Copy link
Member

jay commented Nov 28, 2022

MSYS2 shells are some variation of a cygwin shell because it's a fork of cygwin. mingw32-make should probably be run from the command prompt.

@vszakats
Copy link
Member Author

@jay: MSYS2 somewhat confusingly offers two GNU Make builds: make.exe [dl] and mingw32-make.exe [dl]. Equivalent builds to the latter are shipping with non-MSYS2 toolchain packages, too.

I've verified my notes and mingw32-make.exe should work fine in or outside MSYS2. In fact all MSYS2's MINGW-packages are native Windows ones, not depending on MSYS2, and thus working fine standalone.

Having said that, I agree fully that the best is to use it with the native Windows shell. Also the easiest.

[ Another source of confusion is that the DJGPP Windows cross-toolchain doesn't come with a GNU Make, and Windows doesn't have one by default either, so one needs to figure out that mingw32-make.exe is necessary, where to get that and how to run it. ]

@gvanem
Copy link
Contributor

gvanem commented Nov 29, 2022

Is it so that you're using a Cygwin shell with a non-Cygwin mingw32-make?

That's correct. Have worked fine for year.

Edit: I spawn Cygwin's sh.exe from my 4NT shell. Otherwise I stay long away from any bash related.

@vszakats
Copy link
Member Author

Based on what we know so far I don't consider this a Makefile.mk issue, but a local environment one.

That said, it would be nice to see if it works in a non-Cygwin shell, preferably with mingw32-make and the default OS shell. Or an MSYS2 shell.

I'm also okay to remove myself from the MS-DOS-business and go along with the revert.

@vszakats
Copy link
Member Author

vszakats commented Nov 29, 2022

I went along and tested this, and I cannot replicate it. The linked, standalone DJGPP cross-toolchain for Windows, MSYS2's mingw32-make.exe run via the default Windows shell worked just fine. Produced a curl.exe. One key thing was to use forward slashes in the settings, otherwise all sorts of issues ensue (this is considered "normal" in these scenarios). Also, any native (non-Cygwin) mingw32-make.exe build would probably work just as good. UPDATE: I also run the same batch from an MSYS2 shell: worked just as fine (except groff auto-detection, with a fix coming up).

One other thing I found is that the OPENSSL_LIBS setting must be deleted for cross-builds, as the actual library name is libcrypto.a (the default in Makefile.mk), not libcrypt.a. I must have renamed it locally while testing the old build system, which used the 8.3 name. I've updated this thread, but too late to change it in the commit message.

Here's my batch file:

set CROSSPREFIX=C:/djgpp/bin/i586-pc-msdosdjgpp-
set WATT_PATH=C:/djgpp/net/watt
set ZLIB_PATH=C:/djgpp
set OPENSSL_PATH=C:/djgpp
set CFG=-zlib-ssl
C:\msys64\mingw64\bin\mingw32-make -C lib -f Makefile.mk
C:\msys64\mingw64\bin\mingw32-make -C src -f Makefile.mk

With this, I'm "closing" this issue and the PR without merging. If someone wants to merge anyway, or apply any other fix or documentation update, feel free of course.

@gvanem
Copy link
Contributor

gvanem commented Nov 29, 2022

C:\msys64\mingw64\bin\mingw32-make -C src -f Makefile.mk

Try with make --warn-undefined-variables -C src -f Makefile.mk. There's a lot!

@vszakats
Copy link
Member Author

Is there any one of those that create a build breakage? Otherwise assuming an empty value for them is fine. Some of those are undefined on purpose, so that the caller can pass values (e.g. ARCH).

@gvanem
Copy link
Contributor

gvanem commented Nov 29, 2022

Then it should read ARCH ?= etc. IMHO.

@vszakats
Copy link
Member Author

vszakats commented Nov 29, 2022

I'm going to add VAR ?= for the customization variables. This gives an easy way to lookup all of them by grepping for ?=, while also addressing the "a lot" part.

In general I don't see the value in silencing these debug warnings. Setting everything to empty by default and in else branches might be useful to avoid local variables spilling into the scripts, but the audience is so small and those variables are so few that I don't think this is something we need to focus on. If such issue is reported, we can fix it then.

vszakats added a commit that referenced this pull request Dec 2, 2022
- Fix `NROFF` auto-detection with certain shell/make-build combinations:

  When a non-MSYS2 GNU Make runs inside an MSYS2 shell, Make executes
  the detection command as-is via `CreateProcess()`. It fails because
  `command` is an `sh` built-in. Ensure to explicitly invoke the shell.

- Initialize user-customizable variables:

  Silences a list of warnings when running GNU Make with the option
  `--warn-undefined-variables`. Another benefit is that it's now easy
  to look up all user-customizable `Makefile.mk` variables by grepping
  for ` ?=` in the curl source tree.

  Suggested-by: Gisle Vanem
  Ref: #9764 (comment)

- Fix `MKDIR` invocation:

  Avoid a warning and potential issue in envs without forward-slash
  support.

Closes #10000
@vszakats vszakats deleted the m32port2 branch February 2, 2023 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

4 participants