GLVis / glvis

Lightweight OpenGL tool for accurate and flexible finite element visualization
http://glvis.org
BSD 3-Clause "New" or "Revised" License
251 stars 51 forks source link

Optional build dependencies incomplete #220

Closed termi-official closed 2 years ago

termi-official commented 2 years ago

Turns out the build scripts make use if xxd if we want to have the logo in the built program. See https://github.com/GLVis/glvis/blob/master/makefile#L278-L280 . Technically xxd is a tool from vim, which just happens to be shipped with Fedora and Ubuntu by default. Not sure if this belongs here or on the web repo. Might be an issue for novice users, as this can be easily be missed, yielding a linker error:

mpicxx -Xclang -load -Xclang LLVMPolly.so -mllvm -polly -I../mfem -I../mfem/../hypre/src/hypre/include -I../mfem/../metis-5.0/include -I../mfem/../gslib/build/include -I../mfem/../caliper/include -g -fopenmp -DGLVIS_MULTISAMPLE=4 -DGLVIS_MS_LINEWIDTH=1.4 -DGLVIS_OGL3 -DGLVIS_USE_LOGO -I/usr/include/freetype2 -I/usr/include -I/usr/include -I/usr/include -I/usr/include -DGLVIS_USE_LIBPNG -o glvis glvis.cpp -Llib -lglvis -L../mfem -lmfem -L../mfem/../hypre/src/hypre/lib -lHYPRE -L../mfem/../metis-5.0/lib -lmetis -L../mfem/../gslib/build/lib -lgs -Wl,-rpath,../mfem/../caliper/lib64 -L../mfem/../caliper/lib64 -lcaliper -lunwind -ldl -lrt -lz -L/usr/lib -L/usr/lib -Wl,-rpath,/usr/lib -L/usr/lib -Wl,-rpath,/usr/lib -lfreetype -lfontconfig -lSDL2 -lGLEW -lGL -lpng -lpthread
/usr/bin/ld: lib/libglvis.a(sdl_main.o): in function `SdlMainThread::setWindowIcon(SDL_Window*)':
/home/dogiermann/Repos/glvis/lib/sdl_main.cpp:445: undefined reference to `logo_rgba_len'
/usr/bin/ld: /home/dogiermann/Repos/glvis/lib/sdl_main.cpp:446: undefined reference to `logo_rgba_len'
/usr/bin/ld: /home/dogiermann/Repos/glvis/lib/sdl_main.cpp:453: undefined reference to `logo_rgba'
clang-13: error: linker command failed with exit code 1 (use -v to see invocation)
make: *** [makefile:276: glvis] Error 1

Maybe we should just check if the tool is there in the make and cmake files - however I am not sure which command line tools are allowed, so I opened an issue instead of a PR.

v-dobrev commented 2 years ago

I think I've seen this issue before: https://github.com/GLVis/glvis/issues/210.

We can add a check in the makefile with something like this:

XXD_BIN = $(or $(shell command -v xxd),$(error Required tool not found: xxd. Stop.))

I'm not sure what happens with CMake -- maybe it stops with an error if does not find xxd?

termi-official commented 2 years ago

Indeed, this is the same issue.

I would also have opted for checking if xxd is installed by using command, but with ?= instead of direct assignment.

"Fixing" the cmake build should be as easy as

find_program(XXD_BIN xxd)
if(NOT XXD_BIN)
    error(FATAL_ERROR "xxd not found!")
endif()
termi-official commented 2 years ago

Should I make a PR?

v-dobrev commented 2 years ago

Should I make a PR?

A PR will be welcome! Thanks!

tzanio commented 2 years ago

Discussion continued in @222