Skip to content

cmake: make test-ci target skip building dependencies#15001

Closed
vszakats wants to merge 8 commits intocurl:masterfrom
vszakats:cm-test-ci-opt
Closed

cmake: make test-ci target skip building dependencies#15001
vszakats wants to merge 8 commits intocurl:masterfrom
vszakats:cm-test-ci-opt

Conversation

@vszakats
Copy link
Copy Markdown
Member

@vszakats vszakats commented Sep 22, 2024

Make test-ci not depend on the testdeps target.

test-ci is designed to run curl tests in CI. In CI we build all
necessary dependencies explicitly beforehand, and they are always ready
when calling the test-ci step. Thus, it isn't necessary to enforce
them via a dependency rule. Dropping it saves redundant work and delay
in CI jobs.

The testdeps dependency should not normally be a problem. It's
supposed to be a no-op if those targets are already built. In practice
however, it causes a delay and/or an actual rebuild of those
dependencies depending on generator (= build tool) used and other
factors.

As observed in the GHA/windows workflow, the testdeps dependency:

Notice that test-* targets depending on testdeps is NOT sufficient
to build everything to run tests, e.g. it misses to build the curl tool,
which is essential. This is also true for autotools' test-ci target
which misses to build libcurl and the curl tool.

Perhaps it'd be best to drop testdeps as a dependency for all
test-* targets and make it official to require building dependencies
manually. Alternatively these targets could be fixed to rebuild
everything necessary to run tests.

@vszakats vszakats added the cmake label Sep 22, 2024
@vszakats vszakats marked this pull request as draft September 22, 2024 10:02
Make `test-ci` not depend on the `testdeps` target.

`test-ci` is designed to run curl tests in CI. In CI we build all
necessary dependencies explicitly beforehand, and they are always ready
when calling the `test-ci` step. Thus, it isn't necessary to enforce
them via an dependency rule.

The `testdeps` target should not normally be a problem. It's supposed
to be a no-op if those targets are already built. In practice however,
it causes a delay and/or an actual rebuild of those dependencies
depending on generator (= build tool) used and other factors.

As observed in the GHA/windows workflow, the `testdeps` dependency:

- with Ninja, did not cause any delay, and no extra work was done:
  https://github.com/curl/curl/actions/runs/10980099984/job/30485440389#step:25:18

- with GNU Make, caused a re-evaluation of all `testdeps` targets,
  but did not actually rebuild them. This re-evaluation took a
  noticeable time (esp. with non-bundled tests):
  https://github.com/curl/curl/actions/runs/10980099984/job/30485440155#step:14:11 (with bundles)
  https://github.com/curl/curl/actions/runs/10973851013/job/30471690331#step:14:11 (w/o bundles)

- With MSBuild, caused a re-evaluation of all `testdeps` targets,
  and a _rebuild_:
  https://github.com/curl/curl/actions/runs/10980099984/job/30485435968#step:14:19 (with bundles)
  https://github.com/curl/curl/actions/runs/10973851013/job/30471689714#step:14:19 (w/o bundles)
  It's suspected that our use of
  `-DCMAKE_VS_GLOBALS=TrackFileAccess=false` in CI is contributing to
  this. This option is supposed to affect incremental builds only. For
  some reason it affects CI builds too, even though they are not
  incremental, and no sources are changed between the build steps.

Notice that `test-*` targets depending on `testdeps` is NOT sufficient
to build everything to run tests, e.g. it misses to build the curl tool,
which is essential. This is also true for autotools' `test-ci` target
which misses to build libcurl and the curl tool.

Perhaps it'd be best to drop `testdeps` as a dependency for _all_
`test-*` targets and make it official to require building dependencies
manually. Alternatively these targets could be fixed to rebuild
everything necessary to run tests.
@github-actions github-actions bot added Windows Windows-specific CI Continuous Integration labels Sep 22, 2024
@vszakats vszakats marked this pull request as ready for review September 22, 2024 11:42
@vszakats vszakats closed this in 7b88bb4 Sep 23, 2024
@vszakats vszakats deleted the cm-test-ci-opt branch September 23, 2024 09:54
pps83 pushed a commit to pps83/curl that referenced this pull request Apr 26, 2025
Make `test-ci` not depend on the `testdeps` target.

`test-ci` is designed to run curl tests in CI. In CI we build all
necessary dependencies explicitly beforehand, and they are always ready
when calling the `test-ci` step. Thus, it isn't necessary to enforce
them via a dependency rule. Dropping it saves redundant work and delay
in CI jobs.

The `testdeps` dependency should not normally be a problem. It's
supposed to be a no-op if those targets are already built. In practice
however, it causes a delay and/or an actual rebuild of those
dependencies depending on generator (= build tool) used and other
factors.

As observed in the GHA/windows workflow, the `testdeps` dependency:

- with Ninja, causes no delay, and no extra work:
  https://github.com/curl/curl/actions/runs/10980099984/job/30485440389#step:25:18

- with GNU Make, caused a re-evaluation of `testdeps` targets,
  but did not actually rebuild them. This re-evaluation took a
  noticeable time (esp. with non-bundled tests):
  https://github.com/curl/curl/actions/runs/10980099984/job/30485440155#step:14:11 (with bundles)
  https://github.com/curl/curl/actions/runs/10973851013/job/30471690331#step:14:11 (w/o bundles)
  verbose: https://github.com/curl/curl/actions/runs/10980506956/job/30486434629#step:14:13

- with MSBuild, caused a re-evaluation of `testdeps` targets, and
  triggered a _rebuild_:
  https://github.com/curl/curl/actions/runs/10980099984/job/30485435968#step:14:19 (with bundles)
  https://github.com/curl/curl/actions/runs/10973851013/job/30471689714#step:14:19 (w/o bundles)
  verbose: https://github.com/curl/curl/actions/runs/10980506956/job/30486436368#step:14:48
  It's suspected that our use of
  `-DCMAKE_VS_GLOBALS=TrackFileAccess=false` in CI is contributing to
  this. This option is supposed to affect incremental builds only. For
  some reason it affects CI builds too, even though they are not
  incremental, and no sources are changed between build steps.
  Reported-by: Aki Sakurai
  Ref: curl#14999

Notice that `test-*` targets depending on `testdeps` is NOT sufficient
to build everything to run tests, e.g. it misses to build the curl tool,
which is essential. This is also true for autotools' `test-ci` target
which misses to build libcurl and the curl tool.

Perhaps it'd be best to drop `testdeps` as a dependency for _all_
`test-*` targets and make it official to require building dependencies
manually. Alternatively these targets could be fixed to rebuild
everything necessary to run tests.

Closes curl#15001
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

build CI Continuous Integration cmake tests Windows Windows-specific

Development

Successfully merging this pull request may close these issues.

1 participant