Skip to content
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

Fix libappimage_static #99

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

azubieta
Copy link
Contributor

@azubieta azubieta commented Apr 2, 2019

Install and export the libappimage_static target.

Move the appimage_registered_desktop_file_path function into libappimmage.cpp and use C++ on it.

@TheAssassin
Copy link
Member

If you still want to merge this, you need to fix the PR.

@azubieta azubieta force-pushed the fix_libappimage_static_target branch from fd5a485 to 156816a Compare April 5, 2019 18:39
@azubieta
Copy link
Contributor Author

azubieta commented Apr 5, 2019

Reference #76

Ideally, a binary with the lowest amount of dependencies must be provided the libappimage user. But, due to the large range of possible configurations keeping the track and bundling (in some cases it would be a bad idea do bundle other static libraries) of each dependency turns into a complex task.

The current proposal left the dependencies management to the libappimage user so they will be responsible for resolving all of them (including libsquashfuse).

@azubieta azubieta force-pushed the fix_libappimage_static_target branch from 156816a to a520623 Compare April 5, 2019 18:47
@azubieta
Copy link
Contributor Author

why this was closed?

@azubieta azubieta reopened this Apr 12, 2019
@TheAssassin
Copy link
Member

You wrote

fix #99

in the PR...

@azubieta
Copy link
Contributor Author

azubieta commented Apr 12, 2019

My bad, that pr had nothing to do with this. Reference updated

src/CMakeLists.txt Show resolved Hide resolved
set_property(TARGET libappimage PROPERTY VERSION ${libappimage_VERSION})
set_property(TARGET libappimage PROPERTY SOVERSION ${libappimage_SOVERSION})
# install libappimage
install(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I somewhere read that using PUBLIC_HEADER like this is actually wrong. Is there any reason you use that property? Are there alternative ways? (IIRC, you introduced that years ago, and I was always curious.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, using PUBLIC_HEADER in Unix system is wrong, that property only has a real effect on MacOS.

add_executable(client_app main.c)
target_link_libraries(client_app libappimage)

add_executable(client_app_static_linked main.c)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "test" here is to check if linking works? You should document that somewhere, e.g., above this line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants