[flac-dev] emflac

[Martin von Gagern]

Hi!

Thanks for maintaining FLAC! I recently started a project to compile
parts of the FLAC reference distribution to JavaScript using emscripten.

  https://github.com/gagern/emflac#readme

I'm using the 1.3.1 release and the Makefile.lite build process.
I'm including the FLAC reference implementation as a submodule, and so
far I managed to compile everything without modifying its sources.


In the process I noticed some general issues. I'll discuss them here,
but if you want me to, I can easily turn them into pull requests on
GitHub. I'd rather avoid creating an account anywhere else if possible.


1. Version checks in macros.h

There is something strange about the libFLAC macros.h.
It checks for GNUC 4.3+, but emcc is apparently based on a clang 3.9.0
which identifies itself as GNUC 4.2.1. It does support the statement
expression used for flac_max. So perhaps you could add a different
condition; something like

  (…) || (defined(__clang__) && (__clang_major__ > 3 ||      \
          (__clang_major__ == 3 && __clang_minor__ >= 9)))


2. Min and max macros

I'm also surprised by the degree of variation here. flac_max uses a
statement expression. flac_min relies on some preprocessor magic and the
__COUNTER__ macro instead. For non-gcc-4.3+ it falls back MIN and MAX
from sys/param.h which probably satisfies this dependency for most users
who use configure, but which does not cache its arguments but evaluate
them twice, at least on my system. But for non-Windows with no
sys/param.h, it defines MIN and MAX but does not define flac_min and
flac_max. So in this case you actually get a compile time error.

I would suggest some more consistent coding here. Always try statement
expressions for supporting versions of gcc and clang. Drop the
__COUNTER__ magic for flac_min unless there is a good reason to keep it.
Rename the final fallback macros to flac_max and flac_min.

In emflac I ended up overriding this macros.h by injecting my own
include directory early in the command line. There I use the statement
expression form exclusively, since I control the build environment.


3. Exe suffix

I'm surprised the Makefile.lite build process has variables for library
names, but none for library suffixes (.a, .so, .dll, .dylib, …) but none
for executable suffixes (.exe on Windows in particular). I guess
compilers on Windows will simply append the .exe to the file name, but
that means the Makefile rule will compare the wrong target and will
needlessly re-link executables on every run, since it checks whether a
file called flac exists, when only flac.exe gets created. I don't have a
Windows setup to test this right now. Emscripten needs .js as a suffix
for executables, but I hacked this into PROGRAM_NAME.


4. Makefile.lite configurability

There are some more things I'd like to be able to configure about
Makefile.lite. I can override pretty much every tool using a variable
assignment on the make command line. One notable exception is ranlib.
Having that configurable would be consistent, in my opinion.

The variable LINK is used differently for libraries and binaries: it's
“ar cru” for the former but “$(CC) $(LINKAGE)” for the latter. Using two
different variable names here would allow overriding these without
caring about whether a given subdirectory is producing a binary or a
library.

I'm currently building “shared libraries” (i.e. combined LLVM bytecode
object files) even though all I need is the static libraries. It would
be nice to have a target which allows me to specify that I want to build
the static release version of the libraries only. One approach would be
overriding RELEASE_DYNAMIC_LIB to be the empty string, and depending all
Makefile rules which have this as their target since an empty target
seems to be invalid syntax. This could be particularly important for
platforms which don't support both static and shared libraries, although
I don't know of such a platform at the moment.


5. Next release

The 1.3.1 tag is 22 months old, according to the git repo. According to
http://mid.mail-archive.com/20160108215608.52c6b6b0f83ba0078134e06f@mega-nerd.com
there was some discussion about a 1.3.2 release in January. But
apparently nothing ever came of it. I haven't read all of the thread yet
(and I miss Gmane!)


6. Licensing

If you want, please look through how I handled the different Licenses in
the emflac source repo, readme document and binary distribution. I hope
you agree with my approach (i.e. my sources are MIT, the resulting
binaries of the flac tool are GPL, and a newly written light decoder is
MIT & Xiph(BSD)). Traditionally non-GPL commands were free to call GPL
tools if they invoked them as separate processes using their command
line interface. I wonder how this translates into situations where the
“command-line tool” is in fact a separate module running on the same
interpreter. If anyone has knowledge or strong feelings here, let me know.


Greetings,
 Martin von Gagern