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

[vcpkg|scripts] add configure cache to vcpkg_configure_make #22473

Draft
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

Neumann-A
Copy link
Contributor

@Neumann-A Neumann-A commented Jan 11, 2022

Adds the possibility to add a cache file for make to avoid running configure checks. (Needs additional ABI hashing for the cache files.)

depends on a tool change to include some files in the binary hash

@dg0yt
Copy link
Contributor

dg0yt commented Jan 11, 2022

How to mix with a per-port cache file?
Test port: gettext.

@Neumann-A
Copy link
Contributor Author

@dg0yt simply overwrite the value or try to merge the two files in the portfile.

@Neumann-A
Copy link
Contributor Author

@dg0yt: Also is gettext maybe not using AC_CONFIG_SUBDIRS and running configure manually in the subdirs?

@dg0yt
Copy link
Contributor

dg0yt commented Jan 11, 2022

I explicitly added a shared cache to port gettext, to speed up the build time (which is mostly configuration tests).

@Neumann-A
Copy link
Contributor Author

@dg0yt In the gettext case I would probably add NO_CONFIGURE_CACHE and deal with the merging of options within the portfile. (Simply moving the VCPKG_MAKE_CONFIGURE_CACHE to the file location gettext internally uses)

@JackBoosY JackBoosY added the category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly label Jan 13, 2022
@JackBoosY
Copy link
Contributor

Any progress?

# Conflicts:
#	scripts/cmake/vcpkg_configure_make.cmake
@dg0yt
Copy link
Contributor

dg0yt commented Apr 20, 2022

Maybe it should be skipped for most triplets until mature enough? Too much world rebuild congestion ATM.
(And too much feature development in old maintainer scripts.)

@JackBoosY
Copy link
Contributor

Maybe it should be skipped for most triplets until major enough? Too much world rebuild congestion ATM. (And too much feature development in old maintainer scripts.)

I think it can be done if we can explicitly modify the affected parts and detect them.

@Neumann-A
Copy link
Contributor Author

He is just saying I should skip merging this PR with master until all the other WR PRs have been solved.

@dg0yt
Copy link
Contributor

dg0yt commented Apr 20, 2022

With the scripts, skipping some triplets is difficult indeed, unless hacking it into some other script which is not hashed.
The other PRs touch script ports, so they could just use temporary ci.baseline entry.

@dg0yt
Copy link
Contributor

dg0yt commented May 20, 2022

There is #24823 now. Do you want to update and finish this PR for rolling up the changes to the same script?

@Neumann-A
Copy link
Contributor Author

@dg0yt I think the MS team hasn't decided to merge this approach yet? So it probably doesn't make sense to block the bugfix on this PR.

@JackBoosY
Copy link
Contributor

I think we should go through the configuration process completely every time instead of using a cache that might contain problems?
When --editable is used, the cache is undoubtedly activated.

@Neumann-A
Copy link
Contributor Author

I think we should go through the configuration process completely every time instead of using a cache that might contain problems?

a) it doesn't make sense to always run all of the configure test. Especially if the result is know (e.g. checks for if the compiler is GNU)
b) /Oi makes problems for configure giving the wrong configure result.
c) in cross builds some configure results need to be feed explicitly

When --editable is used, the cache is undoubtedly activated.

since when? The build folder with the configure results gets deleted.....

@dg0yt
Copy link
Contributor

dg0yt commented May 23, 2022

I think we should go through the configuration process completely every time instead of using a cache that might contain problems?

a) it doesn't make sense to always run all of the configure test. Especially if the result is know (e.g. checks for if the compiler is GNU)

Reminding the reviewers that this configuration process is one of the slow parts of autotools builds on windows, because a) it relies on running many programs, and b) running programs is particular expensive on Windows due to passing the border betweens Windows world and mingw/msys2 subsystem world.

So it least fairly immutable properties of Windows triplets should be cached/prefefined globally.

@JackBoosY
Copy link
Contributor

cc @ras0219-msft @BillyONeal

github-actions[bot]
github-actions bot previously approved these changes Jun 7, 2022
# Conflicts:
#	docs/maintainers/vcpkg_configure_make.md
#	scripts/cmake/vcpkg_configure_make.cmake
and update release only triplet
@Neumann-A
Copy link
Contributor Author

My PC:
Before:

Elapsed time to handle libiconv:x64-windows-release: 1 min
Elapsed time to handle gettext-libintl:x64-windows-release: 57 s
Elapsed time to handle gettext:x64-windows-release: 4.8 min

After:

Elapsed time to handle libiconv:x64-windows-release: 53 s
Elapsed time to handle gettext-libintl:x64-windows-release: 55 s
Elapsed time to handle gettext:x64-windows-release: 4.6 min

so seems to shave a few seconds of. Probably needs more values to be added to the cache.

@Neumann-A
Copy link
Contributor Author

@dg0yt: any ideas what the expensive configure checks are? At least the one I used here are basically so fast that they doesn't seem to matter.

@dg0yt
Copy link
Contributor

dg0yt commented Apr 30, 2024

@dg0yt: any ideas what the expensive configure checks are? At least the one I used here are basically so fast that they doesn't seem to matter.

I'm not sure if we are looking at the same thing. For gettext, it is like this:

  • Tests are run sequentially.
  • The package runs sub-configures from a top-level configure.
  • The sub-configures have the same tests.

So the expensive thing is to do the same test several times. Passing a cache file into the top-level configure makes the later tests re-use previous results.

And this could be said about all packages that use gnulib: They do similar tests although the result can be expected to reproduce the same result for a given triplet, unless gaps are filled by installing ports. That's why capturing and caching gl_cv_... values could help to save some more time.

@Neumann-A
Copy link
Contributor Author

Neumann-A commented May 1, 2024

So the expensive thing is to do the same test several times. Passing a cache file into the top-level configure makes the later tests re-use previous results.

For me it seems like some tests are rerun regardless. The configure script does not have a way to skip some tests if the variables are already defined. I mean it still saves a few seconds but I was honestly expecting a lot more considering how long configure sometimes takes.

libiconv:x64-windows: 1.7 min
gettext-libintl:x64-windows: 1.8 min
gettext:x64-windows: 4.5 min
Total install time: 8.1 min
<more stuff added to chache>
Elapsed time to handle libiconv:x64-windows: 1.1 min
Elapsed time to handle gettext-libintl:x64-windows: 1.8 min
Elapsed time to handle gettext:x64-windows: 4.4 min
Total install time: 7.3 min

@dg0yt
Copy link
Contributor

dg0yt commented May 1, 2024

Also "debug" and "release" do the same tests. But in gettext, "debug" should be nearly nothing.

@dg0yt
Copy link
Contributor

dg0yt commented May 1, 2024

And after configure, make should really be able to build "debug" and "release" in parallel, adding some concurrency to otherwise sequential steps.
(Some packages build everything twice to determine symbols for DLL export, and they determine DLL export in sequential processing. libunistring has a patch to parallelize the worst bottleneck.)

@Neumann-A
Copy link
Contributor Author

make should really be able to build "debug" and "release" in parallel

If the sources are always copied before configure I'll agree with that.

Also "debug" and "release" do the same tests. But in gettext, "debug" should be nearly nothing.

Yeah it is but I expected configure in general to be faster.

@dg0yt
Copy link
Contributor

dg0yt commented May 2, 2024

Apart from being sequential, configure permanently crosses the border between the MSYS environmemt (bash) and Windows (cl), each time paying the price of process environment conversion.

@BillyONeal BillyONeal added category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed and removed category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly labels Jun 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants