Skip to content

winbuild: reduce command-line length by dropping whitespace#16528

Closed
vszakats wants to merge 7 commits into
curl:masterfrom
vszakats:winbuild-funtime
Closed

winbuild: reduce command-line length by dropping whitespace#16528
vszakats wants to merge 7 commits into
curl:masterfrom
vszakats:winbuild-funtime

Conversation

@vszakats
Copy link
Copy Markdown
Member

@vszakats vszakats commented Feb 28, 2025

Keep the @for %%i in [...] lines within limits by stripping whitespace
from the input .c source lists read from Makefile.inc. To avoid this
error after adding a new .c source:

configuration name: libcurl-vc14-x64-release-dll-ssl-dll-ipv6-sspi
NMAKE : fatal error U1095: expanded command line 'for %i in (altsvc.obj            amigaos.obj
           asyn-ares.obj         asyn-thread.obj       base64.obj            bufq.obj
              bufref.obj            cf-h1-proxy.obj       cf-h2-proxy.obj       cf-haproxy.obj [...]
  vssh/wolfssh.obj) do @echo ..\builds\libcurl-vc14-x64-release-dll-ssl-dll-ipv6-sspi-obj-lib/%i \
                   ' too long
Stop.
Command exited with code 2

Ref: https://ci.appveyor.com/project/curlorg/curl/builds/51605338/job/dqg6qtebtscb279g#L44

Reported-by: Stefan Eissing
Bug: #16508 (comment)
Fixes #16521


If that's not enough for the next 6 months, we can:

  • strip the line-ending space from the interim .inc files.
  • shorten the -obj-lib, -obj-curl suffixes for the object directories
    to reduce the command-line length further.
  • introduce linker response files.

@vszakats vszakats added build Windows Windows-specific labels Feb 28, 2025
@bagder
Copy link
Copy Markdown
Member

bagder commented Feb 28, 2025

That looked pretty good!

@vszakats vszakats changed the title winbuild: try compacting spaces 1 winbuild: reduce command-line length by dropping some whitespace Feb 28, 2025
@vszakats vszakats changed the title winbuild: reduce command-line length by dropping some whitespace winbuild: reduce command-line length by dropping whitespace Feb 28, 2025
@vszakats
Copy link
Copy Markdown
Member Author

One new input file increases the linker command-line by 80 bytes,
so the first iteration (saving 1 byte per input) allows just 2 new inputs.
It also depends on the build configuration string, which is present in
every filename.

I'm trying another iteration with response files. If that fails, shortening
the suffix probably gives enough room (~12 new files max) for the time
remaining.

@jay
Copy link
Copy Markdown
Member

jay commented Mar 1, 2025

Did your CSOURCES trick not work because I think it should work all by itself

edit: example:

diff --git a/winbuild/Makefile.vc b/winbuild/Makefile.vc
index d404279..eccf684 100644
--- a/winbuild/Makefile.vc
+++ b/winbuild/Makefile.vc
@@ -294,6 +294,8 @@ LIBCURL_DIROBJ = ..\builds\$(CONFIG_NAME_LIB)-obj-lib
 CURL_DIROBJ = ..\builds\$(CONFIG_NAME_LIB)-obj-curl
 DIRDIST = ..\builds\$(CONFIG_NAME_LIB)\
 
+LIBCURL_OBJS=$(LIBCURL_OBJS:  = )
+
 $(MODE):
 	@echo LIBCURL_OBJS = \> LIBCURL_OBJS.inc
 	@for %%i in ($(LIBCURL_OBJS)) do @echo $(LIBCURL_DIROBJ)/%%i \>> LIBCURL_OBJS.inc

@vszakats
Copy link
Copy Markdown
Member Author

vszakats commented Mar 1, 2025

Did your CSOURCES trick not work because I think it should work all by itself

It doesn't work because that list then gets converted to two interim .inc files,
and that's used by MakefileBuild.vc.
The first iteration here stripped the space between the filename and the line-ending
\ in the interim .inc files.

@jay
Copy link
Copy Markdown
Member

jay commented Mar 1, 2025

It doesn't work because that list then gets converted to two interim .inc files,
and that's used by MakefileBuild.vc.

The one line change I posted in the previous message works for me to build Stefan's PR

@vszakats
Copy link
Copy Markdown
Member Author

vszakats commented Mar 1, 2025

It doesn't work because that list then gets converted to two interim .inc files,
and that's used by MakefileBuild.vc.

The one line change I posted in the previous message works for me to build Stefan's PR

You're right, that's also passed to MakefileBuild.vc, though I tried that first in c3f6751 and couldn't see a change in the logs.

@vszakats
Copy link
Copy Markdown
Member Author

vszakats commented Mar 1, 2025

Correction: No, that variable isn't passed down the MakefileBuild.vc. It just uses the same variable name, but the latter file picks it up from the interim .inc files. Man, it's confusing.

I'm not sure why it fixed it for you. Can it be different (shorter) config string than the CI uses?

@jay
Copy link
Copy Markdown
Member

jay commented Mar 1, 2025

It's libcurl-vc10-x86-debug-dll-ssl-dll-ipv6-sspi-schannel-obj-lib so I think it's longer (multissl build). the inc output files are attached. i can take a closer look later but i'd try running that single line in the ci just to see what happens

inc_output_files.zip

@vszakats
Copy link
Copy Markdown
Member Author

vszakats commented Mar 1, 2025

Aha, there are many command-lines that can overflow! I assumed it's
the compiler/linker ones, but in @icing's PR the for loop generating the
interim .inc is the one! That's why the initial space stripping works and it
means that the 1st iteration here was enough to fix it.

If the compiler/linker lines bump into this, we can move on the other
options.

Here's a green run with Stefan's patch applied:
https://ci.appveyor.com/project/curlorg/curl/builds/51610055

vszakats and others added 7 commits March 1, 2025 02:03
Further testing with timeouts in event based processing revealed that our current shutdown handling in the connection pool was not clear enough. Graceful shutdowns can only happen inside a multi handle and it was confusing to track in the code which situation actually applies. It seems better to split the shutdown handling off and have that code always be part of a multi handle.

Add `cshutdn.[ch]` with its own struct to maintain connections being shut down. A `cshutdn` always belongs to a multi handle and uses that for socket/timeout monitoring.

The `cpool`, which can be part of a multi or share, either passes connections to a `cshutdn` or terminates them with a one-time, best effort.

Add an `admin` easy handle to each multi and share. This is used to perform all maintenance operations where no "real" easy handle is available. `cpool` and `cshutdn` do no longer maintain their own internal handle. This solves the problem that the multi admin handle requires some additional initialisation (e.g. timeout list).

The share needs its admin handle as it is often cleaned up when no other transfer or multi handle exists any more. But we need a `data` in almost every call.

Changes in `curl` itself:
- for parallel transfers, do not set a connection pool in the share, rely on the multi's connection pool instead. While not a requirement for the new `cshutdn` to work, this is a) helpful in testing to trigger graceful shutdowns b) a broader code coverage of libcurl via the curl tool
- on test_event with uv, cleanup the multi handle before returning from parallel_event(). The uv struct is on the stack, cleanup of the multi later will crash when it tries to register sockets. This is a "eat your own dogfood" related fix.
@vszakats vszakats closed this in 5693342 Mar 1, 2025
@vszakats vszakats deleted the winbuild-funtime branch March 1, 2025 01:05
pps83 pushed a commit to pps83/curl that referenced this pull request Apr 26, 2025
Keep the `@for %%i in [...]` lines within limits by stripping whitespace
from the input `.c` source lists read from `Makefile.inc`. To avoid this
error after adding a new `.c` source:
```
configuration name: libcurl-vc14-x64-release-dll-ssl-dll-ipv6-sspi
NMAKE : fatal error U1095: expanded command line 'for %i in (altsvc.obj            amigaos.obj
           asyn-ares.obj         asyn-thread.obj       base64.obj            bufq.obj
              bufref.obj            cf-h1-proxy.obj       cf-h2-proxy.obj       cf-haproxy.obj [...]
  vssh/wolfssh.obj) do @echo ..\builds\libcurl-vc14-x64-release-dll-ssl-dll-ipv6-sspi-obj-lib/%i \
                   ' too long
Stop.
Command exited with code 2
```
Ref: https://ci.appveyor.com/project/curlorg/curl/builds/51605338/job/dqg6qtebtscb279g#L44

Reported-by: Stefan Eissing
Bug: curl#16508 (comment)
Fixes curl#16521
Closes curl#16528
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.

4 participants