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

[WIP] Build refactor #129

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

[WIP] Build refactor #129

wants to merge 3 commits into from

Conversation

TFSThiagoBR98
Copy link
Collaborator

@TFSThiagoBR98 TFSThiagoBR98 commented Feb 21, 2022

This is some work for project cleanup

  • Split the Makefile
  • Add mixed build type for both wasm and legacy support;
  • Migrate from autotools to meson, to reduce patches and the build time by ≈25%;
  • Add support for build flags (DEBUG, ENABLE_PTHREADS, ENABLE_LTO);
    • e.g.: make ENABLE_LTO=true DEBUG=true

.gitignore Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@TheOneric
Copy link
Member

I merged most of the small commits unrelated to the build system changes.
As for the git clean change, not removing untracked files from our root repo makes a lot of sense to me, but the benefit isn't as clear to me for the submodules. Do you have an actual use for untracked files in the submodules? Stray files there may taint the licensecheck results and cause issues on commit updates if they collide with newly added files.

I'll write more regarding the build system changes at a later date, but I can already see that this commit does too many unrelated things at once.

@TFSThiagoBR98
Copy link
Collaborator Author

As for the git clean change, not removing untracked files from our root repo makes a lot of sense to me, but the benefit isn't as clear to me for the submodules. Do you have an actual use for untracked files in the submodules? Stray files there may taint the licensecheck results and cause issues on commit updates if they collide with newly added files.

I'll revert it for submodules.

I'll write more regarding the build system changes at a later date, but I can already see that this commit does too many unrelated things at once.

@TheOneric, I'll split this into multiple PR, for better review the changes.

@TFSThiagoBR98 TFSThiagoBR98 changed the title Split Makefile, Add build flags [WIP] Split Makefile, Add build flags Mar 22, 2022
@TheOneric
Copy link
Member

I'll split this into multiple PR, for better review the changes.

Multiple commits in one PR is probably sufficient. If you prefer, feel free to use multiple PRs though.

@TFSThiagoBR98
Copy link
Collaborator Author

TFSThiagoBR98 commented Mar 22, 2022

Multiple commits

Ok, i'll do that

When run make git-update, this will checkout the submodule
for the especified version.
@TFSThiagoBR98 TFSThiagoBR98 changed the title [WIP] Split Makefile, Add build flags [WIP] Build refactor Mar 22, 2022
Copy link
Member

@TheOneric TheOneric left a comment

Choose a reason for hiding this comment

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

First regarding only the current 3 commits:

  • The m4 subfolder is commonly used for M4 files containing macros used in eg autoconf's configure.ac. Putting Makefile fragments there does not seem intuitive to me.
  • I once also considered splitting the Makefile up similar to how you did now, but never got around to taking a shot at it. However looking at the effects now, I'm no longer so sure that this is actually helpful.
    It hides even the targets other fragments depend on and there remains a lot of copy-and-paste which if the instacnes are scattered over multiple files I'm afraid are only even more likely to unintentionally drift apart over time introducing bugs.

SInce we are using GNU Make, an alternative approach could be defining callable macros for all common tasks it makes sense — possibly with the macro definitions moved into a different file. This would keep all the targets in one file, reduce copy-and-paste shortening the Makefile and likely reducing the surface for bugs.

All the git-* rules for example could be shortened into this:
(Btw, we currently do not mark meta-targets as such via .PHONY but should probably do so)

git-checkout:
    git submodule sync --recursive && \
    git submodule update --init --recursive

git-smreset: git-brotli git-expat git-fontconfig git-freetype git-fribidi git-harfbuzz git-libass

.PHONY: git-checkout git-smreset

define TR_GIT_SM_RESET
git-$(1):
    cd lib/$(1) && \
    git reset --hard && \
    git clean -dfx
    git submodule update --force lib/$(1)

.PHONY: git-$(1)
endef

$(foreach subm, brotli expat fontconfig freetype fribidi harfbuzz libass, $(eval $(call TR_GIT_SM_RESET,$(subm))))

Note that since it creates new Make-targets the expansion needs to be processed by Make by calling eval, to make it easier to know the macro (whose definition might be in a different file in a final version)* does, I tenatively used an TR_ prefix for “creates a target”.

For configuration + build + install of projects using autoconf-like configure + make, we could use something like the below, shown only at the example of libass, but the shown macro could be reused for many of our currently built projects:

define FN_COMPILE_GNU
    cd build/lib/$(1) && \
    EM_PKG_CONFIG_PATH=$(DIST_DIR)/lib/pkgconfig \
    emconfigure $(or $(CONFIGURE_PREFIX_$(1)),.)/configure \
        CFLAGS=" \
        -s USE_PTHREADS=0 \
        $(GLOBAL_CFLAGS) \
        -s NO_EXIT_RUNTIME=1 \
        -s MODULARIZE=1 \
        " \
        --prefix="$(DIST_DIR)" \
        --host=x86-none-linux \
        --build=x86_64 \
        --disable-shared \
        --enable-static \
        $(CONFIGURE_FLAGS_$(1)) \
    && \
    emmake make -j8 && \
    emmake make install
endef

CONFIGURE_PREFIX_libass := ../../../lib/libass
CONFIGURE_FLAGS_libass  := --disable-asm --enable-harfbuzz --enable-fontconfig
$(DIST_DIR)/lib/libass.a: $(DIST_DIR)/lib/libfontconfig.a $(DIST_DIR)/lib/libharfbuzz.a \
                  $(DIST_DIR)/lib/libexpat.a $(DIST_DIR)/lib/libfribidi.a $(DIST_DIR)/lib/libfreetype.a \
                  $(DIST_DIR)/lib/libbrotlidec.a build/lib/libass/configured
    $(call FN_COMPILE_GNU,libass)

Note, that unneeded CONFIGURE_… variables do not need to be defined and the exapnded call does not need to be eavled.
Also we could probably use Make’s export directive for EM_PKG_CONFIG_PATH to also not need to put it into all the macros of the different build systems. Possibly we could also export the GLOBAL_CFLAGS as just CFLAGS and expand instead of overwrite the value when we want to do so. For further shortening we could also introduce a common LIBDIR or just LIBD variable for $(DIST_DIR)/lib.
(See TheOneric/JavascriptSubtitlesOctopus@449669a...813e4ec for incomplete commits)

What are your thoughts on this?

Comment on lines 40 to 45
git pull origin master
git fetch origin && \
git checkout $(BROTLI_GIT_VERSION) && \
git submodule sync --recursive && \
git submodule update --init --recursive
Copy link
Member

Choose a reason for hiding this comment

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

I dislike how this requires duplicating the submodule version information which is already stored in our root git itself.
Instead the following can do the same without duplication:

    cd lib/brotli && \
    git reset --hard && \
    git clean -dfx
    git submodule update --force lib/

On another note; those targets seem to have originally been intended to update our submodules all at once as the git-update name of the target invoking all other git-* targets suggests. This commit changes their purpose to resetting/syncing the working state to the HEAD state. Imho this is new purpose is more useful, but perhaps we should then also rename the git-update target to something like git-smreset or so?

Makefile Show resolved Hide resolved
@TheOneric
Copy link
Member

Next, some remarks from my notes on the initial version and not contained in the current commits:

  • Please do not require bash specifically if nothing specific to bash is being used. And iinm we currently do not use any bashisms in our build, so unless there’s a good reason we should probably continue to work with any POSIX shell.

  • The old commit message the primary goal of the switch to Meson was to “reduce the number of patches.”. It doesn't appear those two are correlated at all:

    • The harfbuzz/00002 patch seemed superfluous regardless of which build system is used
    • According to Future fontconfig versions ⪈2.13.94 won't need the fcstat patch #103 the Fontconfig fcstat patch is supposedly unneeded only after we bump our vendored Fontconfig version. If I didn't miss something, you did not bump the version but were removing the patch.
      I did test bumping fontconfig and removing the patch a couple of weeks ago: while it built, at runtime I was getting errors regarding fcstat, but hadn't yet have time to investigate further. Have you checked for runtime errors after you removed this patch?
    • Other than the two patches above it appears that the only patch which can be removed maybe because of meson is freetype/0001-disable-exports. In my testing removing the patch without changing to Meson, did not cause any build problems. What issues did this try to fix?

@TheOneric
Copy link
Member

TheOneric commented Mar 27, 2022

Now regarding the switch the benefits and drawbacks of switching many subprojects to Meson in the context of JSO.

There are some time-savings in Meson using Ninja as a backend and that it compared to autotools from git, only does one configuration step and likely does it quicker. However, I suspect the bulk of the speed improvement you measured comes from the hardcoded eight jobs being replaced by ninja’s default behaviour of using as many jobs as there are logical CPUs available.
Eight jobs are too much for GitHub Action runners, which advertise two logical CPUs, and in local builds it may be either too much or too little. And in some cases, like FriBidi, our build always only uses one(!) thread (probably the -j 8 parameter was forgotten, something which could be avoided by using macros to avoid copy-paste).
This by itself could also be easily achieved with Make, but sure using Meson also gives the other benefits listed before, so why not.

However, regarding speed there’s one thing that would be even better than using a more appropiate job count for each project separately and which is possible with Make but not Ninja (and therefore Meson if it cannot do a “mega-build” in which all projects use Meson): parallelise not only the individual (sub)project builds but also across all targets by using a jobserver. This means if one project no longer needs all the threads, another target can already be executed.
I started an attempt at this a few months ago, but it seemed like (probably) the emmake wrapper interfered with the relevant jobserver arguments. I had the idea of removing all the emmakes in the Makefile and instead calling the root Makefile with emmake, but never actually did because other things came up which took precedence.
If the jobserver can be made to play nicely with emmake the gains from it may possibly outperform the Ninja and configure related gains from Meson. The catch is HarfBuzz having deprecated its autotools build, so once we switch HB to Meson, it either will consume more threads than we’d like or we need to ensure HarfBuzz specifically is not run in parallel with other builds.
If it doesn't, or we decide it isn't worth the effort, switching the projects which advertise Meson as a first-class buildsystem to Meson is probably a tad faster than using their Make-based buildsystem. We should still stop hardcoding -j8.

There also is a patch series (ninja-build/ninja/#1140) to add jobserver support to Ninja since almost 6 years which is still maintained and updated. The last response from a Ninja dev is from 2 years ago though, which doesn't exactly inspire confidence in this getting merged anytime soon.

Since we have multiple CMake-only, at least one autotools-only with prospective two more additional projects, a Meson-and-ninja-only “mega-build” seems unlikely to happen. Using top-level meson with experimental non-Meson subprojects will also suffer from not being able to parallelise across projects (in addition to it being experimental and the warnings from the docs).

@eli-schwartz
Copy link

Which are the cmake-only projects?

freetypeand harfbuzz both support Meson, cmake, and configure+make all at once, very dedicated of them.
fontconfig and fribidi both support meson and configure+make

expat and brotli both support cmake and configure+make

libass is autotools only, but has work on porting to meson.

None of those seem to be cmake-only, and only libass itself is configure+make only.

@TheOneric
Copy link
Member

Btw, since JSO compiles from git not from a tarball, the difference between a proper standard autotools setup (e.g. libass) and custom config+make systems (in FreeType’s case a thin but required wrapper around autools) is relevant as the configuration steps differ.

expat and brotli both support cmake and configure+make

I missed them supporting an autotools build; then it’s 2× CMake-or-autotools not CMake-only. Doesn't make much of a difference though, since both have a Make backend and are not Meson.
WrapDB doesn't seem suitable as explained in another comment.

libass is autotools only, but has work on porting to meson.

That port only aims for Meson as a second-class system missing some features and e.g. X32, possibly enough for JSO though. Also not certain when this comes to a conclusion. But then still, libunibreak and libiconv also use Make-based systems and might be added in the future.

If I understood your explanation correctly, with a Meson-at-toplevel build using the experimental “external project” feature, the individual external projects and those controlled by Meson will use distinct thread pools which together can exceed the user-chosen limit. If so, using it does not seem desirable.
Our options then would be about if we want cross-(sub)project parallelisation. If yes, we should avoid ninja-backed systems when possible (and take a look if HarfBuzz’s CMake system will continued to be supported or is also deprecated). If not, using Meson when provided as a first-class build-system is likely slightly faster than using a Make-based system. In both cases we would stick to GNU Make at top-level.
If at some point in the future Ninja gains jobserver-client support, we can with GNU Make at the toplevel still get both Meson for the subprojects and cross-project parallelisation.

@eli-schwartz
Copy link

and take a look if HarfBuzz’s CMake system will continued to be supported or is also deprecated

Eh... it was removed once already, and then reverted and re-added with the rationale that "there is no need to rush" to remove it, to prevent people being caught off guard. They state in various PRs that they're not keeping it around forever.

If at some point in the future Ninja gains jobserver-client support, we can with GNU Make at the toplevel still get both Meson for the subprojects and cross-project parallelisation.

But you can use Kitware's ninja fork which does have jobserver-client support, but not jobserver support -- this can generally be worked around even for a ninja-first build with a simple Makefile:

all:
	@+ninja

It is true that at the rate things are going, the ninja devs will likely never merge it -- but, Kitware regularly rebases that patchset for new releases and this is e.g. where pip install ninja is sourced from. There are people very invested in that feature. Just don't bother with the one from Debian's repository in that case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants