libjxl / libjxl

JPEG XL image format reference implementation
BSD 3-Clause "New" or "Revised" License
2.64k stars 250 forks source link

Some include files are missing from the release #3273

Open kuro68k opened 7 months ago

kuro68k commented 7 months ago

Describe the bug Some include files are missing from the release source.

jxl/jxl_export.h (referenced in jxl\decode.h and jxl\types.h) jxl/version.h (referenced in jxl\decode.h) jxl/jxl_threads_export.h (referenced in jxl\resizable_parallel_runner.h)

To Reproduce Download libjxl-0.9.2.zip or libjxl-0.9.2.tar.gz and try to build a project with it.

Expected behavior Missing files included in release archives.

Environment N/A

Additional context As a test I tried building these files under Ubuntu and then using them in my project, and my project still wouldn't build because JXL_EXPORT was not defined. One problem at a time though.

luzpaz commented 7 months ago

Note: 0.9.2 is also not mentioned in https://github.com/libjxl/libjxl/blob/main/CHANGELOG.md

mo271 commented 7 months ago

3282 addresses the missing minor versions in the changelog

mo271 commented 7 months ago

Those files are generated when building with cmake. Perhaps you could give more details on how you tried building?

kuro68k commented 7 months ago

Just from the point of view of someone who wants to make software that uses libjxl, it's not great that you have to build libjxl just to get the needed header files. If you do a binary release of the .lib for Windows, it's probably a good idea to also supply all needed header files.

Anyway, I built libjxl under Ubuntu 23, but as noted the generated header files don't actually build because they lack some needed definitions. I followed the instructions here: https://github.com/libjxl/libjxl/blob/main/BUILDING.md

Generated files attached. But the real issue here is that they are not included in any of the released archives, and it is not easy to build software using libjxl.

jxl.zip

kmilos commented 7 months ago

But the real issue here is that they are not included in any of the released archives

They are in packages in most distributions, e.g.:

https://archlinux.org/packages/extra/x86_64/libjxl/files/ https://packages.debian.org/experimental/amd64/libjxl-dev/filelist https://packages.msys2.org/package/mingw-w64-ucrt-x86_64-libjxl?repo=ucrt64 https://ports.macports.org/port/libjxl/details/

But indeed not shipped in artifacts here, e.g.

https://github.com/libjxl/libjxl/releases/download/v0.9.2/jxl-x64-windows.zip https://github.com/libjxl/libjxl/releases/download/v0.9.2/jxl-x64-windows-static.zip

kuro68k commented 7 months ago

Indeed. Backstory is that I have created a plugin for Directory Opus to support JXL, but it's been on hold for over a year. I am waiting for JXL to mature to the point where I don't have to build it myself, I can just grab the latest static library and integrate it into my project.

I think more applications would support JXL if it was easier to integrate.

kmilos commented 7 months ago

I can just grab the latest static library

Have you tried the x64-windows-static package from vcpkg? Currently not the latest, but should be updated soon enough...

kuro68k commented 7 months ago

I have not. I might look at switching over if you think it will help. As well as waiting for JXL to mature on the development side, I found that performance was poor so was hoping to try the latest version.

eustas commented 7 months ago

_export.h depends on system, that is why I propose not to include it into sources archive. version.h is system-independent; it provides only 3 integers, so I believe we can include it into sources archive (and add check it always the same as generated.

eustas commented 7 months ago

Idea: pull version info into a separate properties file to make build systems equal and depend on the same source. See e.g.: https://github.com/knik0/faad2/blob/master/properties.json

kuro68k commented 7 months ago

_export.h depends on system, that is why I propose not to include it into sources archive.

Does it depend on the OS or the compiler? Either way, it should be possible to create a generic one with preprocessor directives. Or at least one that covers common systems, and use a #error directive for unsupported ones. Having an error message in there with a hint about how to fix it would really help.

eustas commented 7 months ago

It depends on compiler: https://github.com/Kitware/CMake/blob/master/Modules/GenerateExportHeader.cmake Not saying it is impossible, but making it right would be difficult.

kuro68k commented 7 months ago

I'll take your word for it. Looking at that file and the generated headers, it seems like the whole thing is rather complex. Maybe it could be simplified. Or maybe the build system could at least produce headers for gcc and clang. I build in Visual Studio for Windows x64 using clang.