-
-
Notifications
You must be signed in to change notification settings - Fork 149
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Statically link our code #99
Conversation
See my comment at the corresponding bug. |
So, building without a shared object was successful but for some reason I had to add |
and now CI build is failing because It doesn't have xcb! if you ask me we always did need it, it's included in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job, builds and runs though the pipeline has to be fixed before a merge.
CMakeLists.txt
Outdated
|
||
|
||
add_executable(antimicrox ${antimicrox_MAIN}) | ||
target_link_libraries (antimicrox antilib Qt5::Widgets Qt5::Core Qt5::Gui Qt5::Network Qt5::Concurrent) | ||
target_link_libraries(antimicrox Qt5::Widgets Qt5::Core Qt5::Gui Qt5::Network Qt5::Concurrent ${LIBS}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where's ${SDL_LIBRARY}
? Maybe that contained xcb
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pkg_check_modules(XXX REQUIRED yyy)
provides XXX_LIBRARIES
which in the case is SDL2_LIBRARIES
this is a oddity in the ways we find libraries.
we could just use find_package(SDL2 REQUIRED)
which provides SDL_LIBRARIES
and SDL_LIBRARY
(they are the same).
So i think in a previous version we used find_package
but later it was changed to pkg_check_modules
but they forgot to remove the variable.(I can't think of any reasons other than older cmake versions behaving differently)
anyways, I could refactor further and bring back find_package
?
ref: https://cmake.org/cmake/help/latest/module/FindSDL.html
ref: https://cmake.org/cmake/help/latest/module/FindPkgConfig.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and before my changes it was empty(not set) so removing it wont change anything.
Sorry, I found SDL at an earlier |
endif() | ||
|
||
add_library( antilib | ||
SHARED |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a suspicion that xcb
linking was not needed because this used to be compiled as a shared library, it was only linked dynamically, and xcb
is a requirement for qt
so it's available at runtime. @pktiuk is this a wild guess?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We directly need xcb because some functions fro xcb/xcb.h where used.
And these are not a part of qt in any ways possible..
It's nice, but we need green ✔️ to merge. So that problem needs to be sorted out first somehow. |
another pull request should add xcb to the build script |
I've tried various settings and configs but I cant get github actions to build on my fork
I suspect that I'm not installing the currect packages. I added two lines to inspect the
but under ubuntu
|
I've verified that |
@pktiuk save us! :D |
He might not be available for the time being, we have to figure this ourselfs. |
@gombosg can you build my PR on your end? |
I won't be able to look at this during this week, but I think this change can (and should) just wait. |
I spent way too much time on this already. I have no clue what's going on. Tried linking against Tried updating to I even checked the pkgconfig files on Ubuntu repos and my machine and they are the same. So I give up. Next step is removing
|
Sorry you had to wait for me, but yesterday I had an exam and today I was preparing for the next tomorrow xD I have dealt with this problem in another PR #107 This one can be closed. |
Closes #98
Might fail for the 'deb' target. I'm not sure...Since this is mostly a search-and-replace job, things should not break, though it might break some install scripts...