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

[gettext] Split, fixes, faster build, checked-in config cache #30429

Merged
merged 18 commits into from
May 22, 2023

Conversation

dg0yt
Copy link
Contributor

@dg0yt dg0yt commented Mar 25, 2023

Major contributions:

  • Splits gettext-libintl out of gettext.
    • Allows to simplify the portfiles (at the price of some redundancy e.g. in patches).
    • Allows to clearly mark gettext-libintl as being under LGPL, not GPL.
    • Allows to install gettext[tools] without reinstalling ports which only need gettext-libintl.
      To fully leverage the effect, downstream ports still need to switch dependencies.
  • Reverts use of the original makefiles, but adds new modifications to improve port build time.
    In particlar, uses bash built-in evaluations instead of sub-shells, skips some subdirs, and enables concurrency in one large subdir.
    (I won't try to upstream this. Upstream targets a larger set of platforms and tools and will refuse these changes. We have seen this for libunistring.)
  • Adds a script port which helps to maintain autoconf configuration cache files. This greatly reduces the time which is spent in the configure step on Windows.
    Removed from PR
  • Adds a few more bits to what is quickly installed by gettext[core], removing some extra duties from port mchehab-zbar.
  • Adds more known/required configuration results, for speed and correctness.
    Fixes [gettext] Build error (conflict with libxml2) #30579, libxml2 installation order problem.

.

  • Changes comply with the maintainer guide
  • SHA512s are updated for each updated download
  • The "supports" clause reflects platforms that may be fixed by this new version
  • Any fixed CI baseline entries are removed from that file.
  • Any patches that are no longer applied are deleted from the port's directory.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is added to each modified port's versions file.

@Thomas1664
Copy link
Contributor

Maybe I should really start with the vcpkg-make port.

+1; I asked myself multiple times why cmake has its own port but make doesn't.

Coincidentally, I worked a few weeks ago on optimizing make builds as well. I thought that using native binaries instead of msys2 could speed up configure/ build but I was only able to compile make for Windows. Unfortunately, I was unable to get successful builds with it. Next I tried bash which also didn't work because it is unable to find getopt.h although I declared a dependency on getopt-win32 but I think this won't be the only missing header... I also thought about caching the results of configure but I didn't know how to implement this.

That being said, I highly appreciate your work.

@dg0yt
Copy link
Contributor Author

dg0yt commented Mar 26, 2023

MSYS2 exists because no better sufficiently complete environment exists for doing autotools builds on Windows. Probably it is reasonable to assume that they do a fairly good job with regard to the trade-off between compatibility and performance.
However, the autobuild buildsystems focus on portability at the price of performance. This implies using the smallest common denominator with regard to sh and make features, testing for toolchain properties, and applying gnulib portability polyfill. In addition, sometimes steps don't make good use of concurrency (cf. my libunistring patch).

For gettext[core]:x64-mingw-static, most native "build" time is actually spent in the configure step. Figures from my windows test machine, per build type (debug or release):

  • The build step takes app. 40 s.
  • The configure step takes more than 200 s.
    • Reusing the full config cache reduces the this to app. 120 s.
  • "Bashifying" the frequent "x='`echo ... | sed ...`'" patterns (from AC_SUBST) saves another 15 s.
  • The ultimate improvement would be parallel configure. (And it shouldn't be too difficult to implement.)

(For the tracing of configure, I piped the output into ts from moreutils.)

At the moment I'm reviewing the differences of the windows (MSVC) config caches, as available from the CI failure logs. Some findings are surprising (at least for me):

  • Some values are "guessed" for non-native builds. It should be save to pre-seed this also for native builds.
    e.g. -x64 (native) vs. x86 (cross):
    --- collected/rel/config.cache-x64-windows-rel.log      2023-03-26 18:03:35.281517294 +0200
    +++ collected/rel/config.cache-x86-windows-rel.log      2023-03-26 18:03:35.285517351 +0200
    @@ -97 +97 @@
    -ac_cv_func_malloc_0_nonnull=${ac_cv_func_malloc_0_nonnull=yes}
    +ac_cv_func_malloc_0_nonnull=${ac_cv_func_malloc_0_nonnull='guessing yes'}
  • Some guessed values differ from detected values.
    e.g. -x64 (native) vs. x86 (cross):
    --- collected/rel/config.cache-x64-windows-rel.log      2023-03-26 18:03:35.281517294 +0200
    +++ collected/rel/config.cache-x86-windows-rel.log      2023-03-26 18:03:35.285517351 +0200
    @@ -469 +469 @@
    -gl_cv_func_getcwd_null=${gl_cv_func_getcwd_null=yes}
    +gl_cv_func_getcwd_null=${gl_cv_func_getcwd_null='guessing no'}
  • Even some detected values differ unexpectedly.
    e.g. x64-windows (native) vs. x64-windows-static (cross):
    --- collected/rel/config.cache-x64-windows-rel.log      2023-03-26 18:10:57.587822733 +0200
    +++ collected/rel/config.cache-x64-windows-static-rel.log       2023-03-26 18:10:57.587822733 +0200
    @@ -153 +153 @@
    -ac_cv_func_snprintf=${ac_cv_func_snprintf=no}
    +ac_cv_func_snprintf=${ac_cv_func_snprintf=yes}

@MonicaLiu0311 MonicaLiu0311 self-assigned this Mar 27, 2023
@@ -27,6 +27,27 @@ vcpkg_find_acquire_program(BISON)
get_filename_component(BISON_PATH "${BISON}" DIRECTORY)
vcpkg_add_to_path("${BISON_PATH}")

if(NOT DEFINED VCPKG_AUTOTOOLS_CONFIG_CACHE)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Defining VCPKG_AUTOTOOLS_CONFIG_CACHE to a non-existing file (e.g. NONE) may be used to disable an existing cache in custom triplet files.

ports/gettext/portfile.cmake Outdated Show resolved Hide resolved
file(READ "${CURRENT_BUILDTREES_DIR}/config.cache-${TARGET_TRIPLET}" config_cache)
string(REGEX REPLACE "\nac_cv_env[^\n]*" "" config_cache "${config_cache}") # Eliminate build type flags
file(WRITE "${CURRENT_BUILDTREES_DIR}/config.cache-${TARGET_TRIPLET}" "${config_cache}")
vcpkg_config_cache_reuse()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even in absence of an initial (checked-in) cache, this could generally speed up configuration for the second build type.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No longer used in these ports.

@dg0yt
Copy link
Contributor Author

dg0yt commented Mar 27, 2023

gettext[core,tools]:x64-windows: Down from 10.8 to 8.0 min.
gettext[core]:x64-uwp: Down from 4.4 min to 2.1 min.

@MonicaLiu0311 MonicaLiu0311 added the category:port-bug The issue is with a library, which is something the port should already support label Mar 27, 2023
@dg0yt
Copy link
Contributor Author

dg0yt commented Apr 10, 2023

With configuration caching, further tuning, libintl moved to a separate port and gettext-runtime tools moved to another feature:
gettext-libintl:x64-windows: Down to 1.7 min.
gettext[tools]:x64-windows: Down to 4.3 min.

In total, build time is down from to 10.7 min to 6 min. But there are other benefits:

  • Many ports only need to depend on gettext-libintl (at least if not using native language support). Installing tools later doesn't need to trigger a rebuild.
  • The custom Makefile is gone. Subdirs are built in the same order as intented upstream. (But still some subdirs are omitted.)
  • Splitting libintl out of gettext allows simpler portfiles, and a clear indication of license for each of the ports.

gettext[core] should now have everthing needed before running autoreconf. A particular use case: port mchehab-zbar.

Switching downstream ports from gettext to libintl should be done separately. There are two many of them. Merge conflicts and regression would block a merge for too long.

@dg0yt
Copy link
Contributor Author

dg0yt commented Apr 10, 2023

FTR the checked-in config.cache files were reviewed manually:

  • windows: Unified across CI configurations.
  • mingw: Unified x64 across MSYS2 and Ubuntu 18.04 host environments.

@dg0yt dg0yt marked this pull request as ready for review April 12, 2023 03:35
@dg0yt dg0yt marked this pull request as draft April 12, 2023 03:38
@dg0yt dg0yt force-pushed the gettext branch 2 times, most recently from 4cb4046 to 6b74ee8 Compare May 1, 2023 07:55
@dg0yt dg0yt changed the title [gettext] Fixes, faster build [gettext] Split, fixes, faster build, checked-in config cache May 1, 2023
@dg0yt dg0yt marked this pull request as ready for review May 1, 2023 15:15
@MonicaLiu0311
Copy link
Contributor

gettext-libintl

The usage test passed (header files found):

gettext-libintl is compatible with built-in CMake targets:

    find_package(Intl REQUIRED)
    target_link_libraries(main PRIVATE Intl::Intl) # since CMake 3.20

gettext

All features are tested successfully in the following triplet:

x86-windows
x64-windows
x64-windows-static

mchehab-zbar

All features are tested successfully in the following triplet:

x86-windows
x64-windows
x64-windows-static
arm64-windows

MonicaLiu0311
MonicaLiu0311 previously approved these changes May 5, 2023
@MonicaLiu0311 MonicaLiu0311 added category:new-port The issue is requesting a new library to be added; consider making a PR! info:reviewed Pull Request changes follow basic guidelines labels May 5, 2023
Copy link
Contributor

@JavierMatosD JavierMatosD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, I like the changes in this PR. That said, there are some things I would like to discuss with the team before going forward. Namely, the maintaining of autoconf configuration cache files and the "bashifying" configure.

@JavierMatosD JavierMatosD added the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label May 16, 2023
@JavierMatosD
Copy link
Contributor

Hi @dg0yt, I'd appreciate your opinion on the matter. Do you think it's necessary to include the vcpkg-autoconf-cache port for the splitting out of gettext-libintl? If not, I suggest creating a separate pull request specifically for the cache configuration management. Although the vcpkg-autoconf-cache port seems promising, I believe it should be thoroughly spec'd out before merging.

Regarding the "bashifying" of the configure script, I think it's acceptable.

So, unless you strongly believe that both the splitting out of gettext-libintl and vcpkg-autoconf-cache should be merged together, I kindly request you to separate the pull request. :)

Looking forward to your feedback.

@JavierMatosD JavierMatosD removed the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label May 18, 2023
@JavierMatosD JavierMatosD marked this pull request as draft May 18, 2023 21:18
@dg0yt
Copy link
Contributor Author

dg0yt commented May 19, 2023

Do you think it's necessary to include the vcpkg-autoconf-cache port for the splitting out of gettext-libintl? If not, I suggest creating a separate pull request specifically for the cache configuration management. Although the vcpkg-autoconf-cache port seems promising, I believe it should be thoroughly spec'd out before merging.

There is no problem with that. It was an important of forming this PR, but in the end it the PR is designed to allow make such cuts feasible.

But there can be some some extra total build time in doing less config caching than before.

@dg0yt
Copy link
Contributor Author

dg0yt commented May 20, 2023

With configuration caching, further tuning, libintl moved to a separate port and gettext-runtime tools moved to another feature: gettext-libintl:x64-windows: Down to 1.7 min. gettext[tools]:x64-windows: Down to 4.3 min.

In total, build time is down from to 10.7 min to 6 min. But there are other benefits:

Back to key benefits but without checked-in config cache.

  • gettext-libintl:x64-windows: 3.5 min.
  • gettext[tools]:x64-windows: 6.5 min.
  • Total: 10 min, slightly less than current master gettext[tools]:x64-windows (11 min), despite one additional pass of configuration.

@dg0yt dg0yt marked this pull request as ready for review May 20, 2023 16:00
@JavierMatosD JavierMatosD merged commit 5b744ed into microsoft:master May 22, 2023
@dg0yt dg0yt deleted the gettext branch May 22, 2023 03:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:new-port The issue is requesting a new library to be added; consider making a PR! category:port-bug The issue is with a library, which is something the port should already support info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[gettext] Build error (conflict with libxml2)
4 participants