fmang / opustags

Ogg Opus tags editor
BSD 3-Clause "New" or "Revised" License
75 stars 10 forks source link

macOS compatibility (sort of) #24

Closed akx closed 5 years ago

akx commented 5 years ago

As discussed over email, here's a PR that makes the current opustags build on macOS Mojave. However, make check does not finish successfully – more about that in a second.

To ensure I had a sane baseline to go on, I added a little Ubuntu 18.04 Dockerfile that makes it easier to build the project in a contained (pun intended) Linux environment. However the cmake there (3.10.2) complained about the attempt to link things to the OBJECT library libopustags, so I changed that to STATIC instead. (I've never really touched cmake, so I can only hope that's somewhere near correct – at the very least, the output filename is a little silly: Linking CXX static library liblibopustags.a...)
However make check doesn't run successfully within the container; see https://gist.github.com/akx/987b719d0329da9a3140f90c649fa1f2 for the output.

On macOS, one needs a couple additional headers to have errno and mkstemps available; those were a breeze to add. Additionally, since libogg isn't a system library on macOS and people tend to install it from Homebrew, the library directory needs to be added to the link path; also, iconv is not available in the default linkage, so it needs to be added too.

Again, unfortunately, make check doesn't run successfully: https://gist.github.com/akx/ce7c0763a6c489fb7749039b9a18909a

fmang commented 5 years ago

I'd rather avoid making libopustags a STATIC library for now, OBJECT is more appropriate. Try this instead:

- target_link_libraries(libopustags PUBLIC ${OGG_LIBRARIES})
+ link_libraries(${OGG_LIBRARIES})

I don't think a Dockerfile is appropriate here. The more environments opustags is tested on, the better. If there's a killer argument for it, it should go in a separate pull request anyway.

I'm not that surprised the test suite fails for Unicode. I'm surprised system.t doesn't pass though. I'll make a few pull request and ping you to check, if that's okay with you.

fmang commented 5 years ago

I've updated system.t for better diagnostics. You can run it individually with:

cd build/t
make system.t
prove ./system.t

You can post the results here or create issues for better tracking.

akx commented 5 years ago

I'd rather avoid making libopustags a STATIC library for now, OBJECT is more appropriate. Try this instead: [...]

With this change, I still get

CMake Error at CMakeLists.txt:30 (target_link_libraries):
  Target "libopustags" of type OBJECT_LIBRARY may not be linked into another
  target.  One may link only to STATIC or SHARED libraries, or to executables
  with the ENABLE_EXPORTS property set.

The more environments opustags is tested on, the better.

I totally agree – Docker was just the easiest way to get a Linuxish baseline environment to test on, since I'm on a Mac at work and on Windows at home.

akx commented 5 years ago

By the way, though, here's the output of prove ./system.t on the host Mac:

$ prove ./system.t
./system.t .. 1/2 # unexpected error: Could not convert string 'cat 猫 chat': Illegal byte sequence
# fail: lossy conversion from UTF-8 is detected
./system.t .. Failed 1/2 subtests

Test Summary Report
-------------------
./system.t (Wstat: 0 Tests: 2 Failed: 1)
  Failed test:  2
Files=1, Tests=2,  0 wallclock secs ( 0.03 usr +  0.01 sys =  0.04 CPU)
Result: FAIL

For what it's worth, iconv --version (using the system iconv) says "iconv (GNU libiconv 1.11)".

fmang commented 5 years ago

Most of the tests fail because of encoding issues. I've added a few checks in opustags.t so that it detects what coding is available and skip the tests it cannot run.

For your specific issue with system.t, try changing t/system.cc:

- rc = from_utf8("\xFF\xFF", out);
- is(rc, ot::st::badly_encoded, "conversion from bad UTF-8 fails");

If it doesn't work, try this:

- rc = from_utf8("cat 猫 chat", out);
+ rc = from_utf8("cat \u732B chat", out);
fmang commented 5 years ago

Well, the test suite should work better now. I'll merge this and release the new version not too late. If there's anything that requires fixing let me know.

Thanks for your help!