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.

Sign up for GitHub

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

Jump to bottom

autotools: add support for 'unity' builds, enable in CI #14815

Closed
wants to merge 8 commits into from
Closed

autotools: add support for 'unity' builds, enable in CI #14815

wants to merge 8 commits into from

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Sep 6, 2024 •
edited
Loading

Implement the "unity" builds as known from CMake, but for autotools.
It's limited to lib and src (CMake also supports it in tests).

Enable with: --enable-unity (disabled by default)

Unity builds speed up builds significantly. Cygwin and Windows builds in
particular, but the effect is noticeable on most systems. It also allows
discovering unity issues with autotools, benefitting also CMake when
building the same combination. In CI it makes turnaround times quicker.

This closes build performance with CMake. autotools still lags behind
because it builds shared and static libcurl in two, separate passes.
CMake does it in one. Manpage compilation isn't batched, it is in CMake.
After unity and test bundle support the slowest parts of the build are
the configuration phase (which is effectively a tedious, non-parallel,
compilation and/or linking of 300+ tiny programs. The next bottleneck
is compiling individual examples and finally test servers (only slow
with autotools).

The autotools implementation is slightly less efficient than CMake,
because 3 sources are permanently excluded while in CMake this isn't
necessary and solved more efficiently while building libtests. There is
also no 'unity' support for tests, making them a less efficient also.

Enable it in CI for most configure jobs. Except in GHA/dist (though
it works fine there too), to use the default config there. Also skip for
the Linux AWC-LC job where it made builds time a few seconds longer
(reason undiscovered.)

Autotools test suite builds compared between master → --enable-unity:

@vszakats vszakats changed the title autotools: add support for "unity" builds autotools: add support for 'unity' builds Sep 6, 2024
This was referenced Sep 15, 2024
Closed
Closed
Closed
vszakats added a commit that referenced this pull request Sep 19, 2024
@vszakats
autotools: tidy-ups in src/Makefile.inc
fdb8b40 
- move `EXTRA_DIST` to the top of file.
- move `checksrc` init next to use.
- use variable `HUGE` instead of repeating a literal.

Cherry-picked from #14815
Closes #14933
vszakats added a commit that referenced this pull request Sep 19, 2024
@vszakats
lib: fix unity builds with BearSSL, MSH3, Quiche, OmniOS
aa1a153 
- fix MSH3 static symbol clash.
- fix Quiche static symbol clash.
- fix local macro clash with BearSSL header.
- fix local macro clash with OmniOS system header.
  ```
  In file included from ../../lib/urldata.h:197,
                     from ../../lib/altsvc.c:32,
                     from libcurlall.c:2:
    ../../lib/cf-socket.h:55:25: error: expected ':', ',', ';', '}' or '__attribute__' before '.' token
       55 | #define sa_addr _sa_ex_u.addr
          |                         ^
    In file included from ../../lib/urldata.h:197,
                     from ../../lib/altsvc.c:32,
                     from libcurlall.c:2:
    ../../lib/cf-socket.h:55:25: error: expected ':', ',', ';', '}' or '__attribute__' before '.' token
       55 | #define sa_addr _sa_ex_u.addr
          |                         ^
  ```
  Ref: https://github.com/curl/curl/actions/runs/10738314933/job/29781644299?pr=14772#step:3:6115

Discovered while adding support for "unity" builds for autotools.

Required-by: #14922
Cherry-picked from #14815
Closes #14932
@vszakats vszakats mentioned this pull request Sep 19, 2024
Closed
19 tasks
@github-actions github-actions bot added the CI Continuous Integration label Sep 20, 2024
@vszakats vszakats force-pushed the am-unity branch 2 times, most recently from db5f36f to 1fe2b9d Compare September 20, 2024 03:09
@vszakats
Copy link
Member Author

vszakats commented Sep 20, 2024 •
edited
Loading

This new "unity" option for autotools is disabled by default, except if the env CURL_CI is
set, which enables it by default. This env already had some use in GHA, this PR sets it for
most workflows globally. In a few workflows it's enabled with --enable-unity and in two
workflows: distro and AWC-LC it's intentionally not enabled.

It does work fine in distro, but it's probably best to use the default there.

AWC-LC is a strange case where the build is already faster than elsewhere for some reason,
and unity makes the build a few seconds slower. (Perhaps something obvious, but I don't
know why this is, and I could not see similar results in other jobs.)

This PR is ready for merge.

moritzbuhl pushed a commit to moritzbuhl/curl that referenced this pull request Sep 20, 2024
@vszakats @moritzbuhl
autotools: tidy-ups in src/Makefile.inc
1e6f43d 
- move `EXTRA_DIST` to the top of file.
- move `checksrc` init next to use.
- use variable `HUGE` instead of repeating a literal.

Cherry-picked from curl#14815
Closes curl#14933
moritzbuhl pushed a commit to moritzbuhl/curl that referenced this pull request Sep 20, 2024
@vszakats @moritzbuhl
lib: fix unity builds with BearSSL, MSH3, Quiche, OmniOS
0220f87 
- fix MSH3 static symbol clash.
- fix Quiche static symbol clash.
- fix local macro clash with BearSSL header.
- fix local macro clash with OmniOS system header.
  ```
  In file included from ../../lib/urldata.h:197,
                     from ../../lib/altsvc.c:32,
                     from libcurlall.c:2:
    ../../lib/cf-socket.h:55:25: error: expected ':', ',', ';', '}' or '__attribute__' before '.' token
       55 | #define sa_addr _sa_ex_u.addr
          |                         ^
    In file included from ../../lib/urldata.h:197,
                     from ../../lib/altsvc.c:32,
                     from libcurlall.c:2:
    ../../lib/cf-socket.h:55:25: error: expected ':', ',', ';', '}' or '__attribute__' before '.' token
       55 | #define sa_addr _sa_ex_u.addr
          |                         ^
  ```
  Ref: https://github.com/curl/curl/actions/runs/10738314933/job/29781644299?pr=14772#step:3:6115

Discovered while adding support for "unity" builds for autotools.

Required-by: curl#14922
Cherry-picked from curl#14815
Closes curl#14932
@vszakats
go traditional and pass --enable-unity directly
c43e9d4 
This avoids surprises when a CI job does't replicate locally with the
same options. `CURL_CI` also didn't save much lines as it turns out.
@vszakats
Copy link
Member Author

vszakats commented Sep 20, 2024 •
edited
Loading

Well, the CURL_CI trick didn't save much editing, while it reduced
transparency and local reproducibility, so went back to just enabling
--enable-unity manually where needed. There is no new "special
case" now, --enable-unity is disabled by default, always.

@vszakats vszakats changed the title autotools: add support for 'unity' builds autotools: add support for 'unity' builds, enable in CI Sep 20, 2024
@vszakats vszakats closed this in 60c3d04 Sep 20, 2024
@vszakats vszakats deleted the am-unity branch September 20, 2024 21:55
@vszakats vszakats mentioned this pull request Sep 20, 2024
Closed
vszakats added a commit that referenced this pull request Sep 20, 2024
@vszakats
GHA/linux: disable unity build for fix scanbuild job
c34aaca 
Unity mode seems to defeat the scanner and miss issues.

before, miss: https://github.com/curl/curl/actions/runs/10967056702/job/30456136390
after, OK: https://github.com/curl/curl/actions/runs/10967128744/job/30456330732#step:35:1232

Tested with PR commit:
32854bb

Follow-up to 60c3d04 #14815
Closes #14987
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
build CI Continuous Integration cmdline tool performance
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@vszakats