Patch cleanup #135
Patch cleanup #135
Conversation
|
Regarding the There are no emscripten-specific warnings anymore now. |
|
Rebased and made a small change regarding the If you plan to review this and need more time, say so soon and this can wait however long required. Otherwise I'll assume there are no objections and will merge this soon. The now removed RFC messages in the commits were:
|
We shouldn't need any patches for libass anymore.
It does nothing since we only build static libs.
Wen HarfBuzz-enabled FreeType is built, HarfBuzz gets located via its pc-file which references FreeType’s pc-file. If there’s no such pc-file in the search path it errors out. Add the previously built HarfBuzz-less FreeType’s pc-file to the search path to avoid this. It has no ill effect on the static lib we build.
We knowingly expose deprecated API.
Future emscripten versions warn if link-only options are set during compilation.
All builds but brotli already set NO_EXIT_RUNTIME, and in brotli's case it was probably an oversight for the flag to be missing. All dependencies, but not the final jso linkage also set MODULARIZE, but not -s 'EXPORT_NAME="someting"' as emscripten docs suggest should be done. But since we only use static libs, the final ones are also actually the only linking occuring, so just remove the flag.
It's a link-only setting and since we only build static libs only relevant in the final linking step.
This is no longer implied in future emscripten versions.
Freetype upstream made changes to remove the need for our fcstat patch,
but we actually also need to update emscripten as linking to fstatfs is
bugged in our currently used version and will lead to runtime errors.
- Since emscripten 2.0.26 unresolved symbols cause errors at link time
instead of only at runtime.
- The following commit contained in 2.0.27 fixed fstatfs linking:
emscripten-core/emscripten@2126d3c
Together with the prior emscripten bump, this removes the need for the fcstat patch. Closes: libass#103
We are bound to one toolchain and most results of the config-time checks were ignored anyway. This is more concise and doesn't require a configuration step.
The build currently warns about building an executable with an object-file extension and recommends to use -c, -shared or -r eventhough we already use -r. I'm not sure what -r is supposed to do, but -c only compiles and not links the input which should be exactly what we want here and using -c gets rid of the warning without any apparent negative effects.
This reduces our library patches to only the pthread-disabling ones.
For the fcstat fontconfig patch to be dropped updating emscripten was also necessary due to a bug in the currently used version. Hoping to avoid emscripten-related changes taking over, I stuck to the last 2.x release instead of updating to 3.x, but even so some cleanup was required.
There are some points I’d like to hear an opinion on before proceeding:
Furthermore, the update introduced one more warning, but I wasn’t sure what to do about it — though everything still seems to work fine.
However, as one can see
-ralready is used and was even added explicitly?Regarding the HarfBuzz "dependency" patch: in the first version of #129 the patch seems to have become unnecessary because both the bootstrap and final FreeType libs were installed into the same DESTDIR, so the old pcfile became automatically available. However, this also means the library files already existed albeit without HarfBuzz support. While this might have worked in a sequential build, this seems prone to race-conditions on full-parallel builds so I'm not in favour of adopting this approach.
As for why just dropping the patch without any changes intially seemed to work for me: I didn't
cleanbefore the test so the old harfbuzz-supporting FreeType pc-file was still present in the path. It cannot be dropped without an replacement in a clean build.