adobe-type-tools / afdko

Adobe Font Development Kit for OpenType
https://adobe-type-tools.github.io/afdko/
Other
1.03k stars 167 forks source link

Please allow use of system antlr4 to compile #1407

Open alerque opened 2 years ago

alerque commented 2 years ago

This stunt has made this seriously difficult to package for distros:

https://github.com/adobe-type-tools/afdko/blob/01a35dacc9e8d1735b7f752f3232d38c34e6f843/CMakeLists.txt#L41-L45

Downloading an unreleased version of something via Git in the middle of a build is a pretty big no-no. This should be something more generic like find_package(antlr4-generator REQUIRED). It looks like the whole hack is to get something in the test suite to match expectations. There is almost certainly another way to accomplish this, and if not putting the entire hack behind a build time flag of some kind so it could either be build normally or with the unreleased vendored dependency download for full test support would at least be appreciated.

iterumllc commented 2 years ago

There are a couple of concerns being expressed here.

First, in relation to the general framework, this is the means of integration that the Antlr 4 project encourages in its documentation. That may or may not have to do with the tight coupling of the parser-generator and the runtime, where one is unlikely to work with another if there is even a small version difference . Therefore simply specifying "antlr4" wouldn't be sufficient and specifying a particular version may place too high a burden on maintainers.

In any case we would be receptive to adapting this part of the build to suit other needs as long as they're consistent with Antlr 4 compatibility.

Second, the git tag being used in place of tags/4.9.2 is temporary and is working around the fact that the test suite for the version of utf8cpp pulled in by 4.9.2 doesn't compile with gcc 10+. The utf8cpp group noticed this and updated, and the Antlr4 group has also implemented a different work-around, but the latter has not yet been officially released. Given that the compilation and testing of utf8cpp is internal to the Antlr4 build I'm not sure what the better workaround would be (assuming that reconstructing a fair amount of the Antlr 4 build infrastructure doesn't count as 'better').

There will presumably be an official release of Antlr 4 fairly soon (there's been recent activity in the issue queue) which will resolve the git tag concern. As for the general means of integrating with Antlr, that will likely remain as long as it is recommended by the Antlr project.

iterumllc commented 2 years ago

Just to clarify: Because the runtime is tightly coupled with the parser generator the makeotfexe build links the former statically. So however all of this activity takes place the result is a binary with a bunch of stuff in it, not a set of dynamically linked objects. This means that whether 4.9.2, some particular git tag, or something else is being downloaded should only be relevant to AFDKO. (Dynamic linking is possible but would create headaches when one package needs Antlr 4.9.1, another 4.9.3, and so on.)

josh-hadley commented 2 years ago

@alerque would you be interested in working on an update along the lines of what @iterumllc suggests?

we would be receptive to adapting this part of the build to suit other needs as long as they're consistent with Antlr 4 compatibility

I can leave this issue open for tracking that work if so.

sternenseemann commented 2 years ago

I would like to bump this and second having at least a flag to allow for building against the system antlr4 runtime (dynamically). For packaging afdko in nixpkgs, I have resorted to patching the ExternalProject out for now.

Therefore simply specifying "antlr4" wouldn't be sufficient and specifying a particular version may place too high a burden on maintainers.

Arguably quite the opposite is true. Maintainers already have to deal with the strict version requirements on antlr and by necessity developed approaches for dealing with it.

Besides that dynamic linking is generally a requirement since it allows a library to be shared between multiple packages (and saving space compared to static linking). Also it allows for both packages to be built separately which has multiple advantages: lower build times, simpler build which is easier to fix/reason about etc.

Given that the compilation and testing of utf8cpp is internal to the Antlr4 build I'm not sure what the better workaround would be (assuming that reconstructing a fair amount of the Antlr 4 build infrastructure doesn't count as 'better').

Since 4.9.3, it antlr4 will prefer the system utfcpp if available, so this is no longer an issues: https://github.com/antlr/antlr4/commit/5a808b470e1314b63b0a921178040ccabb357945


Also would it be possible to make the dependency on the cmake, ninja etc. python packages optional? It is basically always preferrable to get these tools via the distribution's package manager instead of downloading binaries in wheels.

iterumllc commented 2 years ago

The current construction of the afdko packaging and its relation to Antlr, CMake, etc. mostly trace back to the fack that it is primarily and foremost a Python package with C code -- something that needs to be built, under normal circumstances, with pip install afdko and similar commands. There is no viable system that we've found for expressing dependencies on non-python packages within that system.

All that said, this is an open source package and we welcome PRs to make it more amenable to packaging for Linux distributions as long as the changes do not compromise building the code under other circumstances. Given that the ExternalAntlr4Cpp.cmake file is an almost unmodified copy of the file provided by the Antlr 4 group, though, it may be more fruitful to work with them on improvements. That file is at https://github.com/antlr/antlr4/blame/master/runtime/Cpp/cmake/ExternalAntlr4Cpp.cmake

iterumllc commented 2 years ago

Since 4.9.3, it antlr4 will prefer the system utfcpp if available,

Unfortunately, as far as we can tell this is only true in a limited sense of "prefer". See https://github.com/antlr/antlr4/pull/3354

duncan-roe commented 2 years ago

antlr4 has now moved on to 4.10.1. I had arranged that afdko.SlackBuild (for SBo)would modify the afdko build process to use the system antlr4 (patch attached). This broke when SBo upgraded antlr4 from 3.9.3. The new antlr4 generated a lot of errors but these all went away when I changed set(CMAKE_CXX_STANDARD 11) to set(CMAKE_CXX_STANDARD 17) in CMakeLists.txt. However there were now some errors from afdko source which were beyond my limited grasp of C++. patch.txt

iterumllc commented 2 years ago

As i noted above, the runtime is tightly coupled to the files generated by the Antlr parser-generator. There is no stable API between the generated files and the runtime -- the Antlr folks change things around at the level of micro-releases.

Any distros that want to chase the very latest Antlr 4 (there will be other distros that only support much older versions of Antlr at the package level) will have to regenerate the files in c/makeotf/lib/hotconv using the python script and the Antlr Java application. We don't recommend this. You will, in effect, be going off on your own source code journey away from our repository. Antlr changes will often need changes to our own, non-generated files.

If you want to avoid these problems you might consider downloading and linking against the matching runtime, as our provided build files do.

sternenseemann commented 2 years ago

I am fine with providing a specific version of the runtime, but I want it coming from the package manager or, rather, it has to: During builds in our packages, network access is disallowed, so we need to patch the ExternalProject code out.

It'd be great if you could at least just add a flag that switches between antlr from the system and antlr from ExternalProject, you can give it a scary name if you want to.

duncan-roe commented 2 years ago

My bad it wasn't clear in my last post: I went back to letting your provided build download antlr4 3.9.3.