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

README.md: update dependency packages list #626

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

r00t-
Copy link
Contributor

@r00t- r00t- commented Feb 11, 2024

inspired by @MKesenheimer's #625

@r00t-
Copy link
Contributor Author

r00t- commented Feb 11, 2024

this is a bit like #624 , #621 , #561
we have some documentation/utilities in various places that is/are poorly maintained and not tested by ci.
maybe we should just remove those and replace them with a reference to something that is tested by ci.
(i.e. https://github.com/volkszaehler/vzlogger/blob/master/.github/workflows/build%2Btest.yml#L8C1-L9C1 )

libgnutls-dev libsasl2-dev uuid-dev uuid-runtime libtool dh-autoreconf libunistring-dev
sudo apt-get install build-essential git-core cmake pkg-config subversion libcurl4-dev \
gnutls-dev libsasl2-dev uuid-dev uuid-runtime libtool dh-autoreconf libunistring-dev \
libjson-c-dev
Copy link

@sicheste sicheste Feb 19, 2024

Choose a reason for hiding this comment

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

Suggested change
libjson-c-dev
libjson-c-dev libcurl4-gnutls-dev libssl-dev libunistring-dev

libcurl4-gnutls-dev and libssl-dev

~/vzlogger $ sudo bash ./install.sh vzlogger libjson libsml mqtt
[...]
-- Looking for json in /usr/local
Json-c search: '/usr/local/include;/usr/local/include;/usr/local/include;/usr/include'
Json-c found: '/usr/local/include'
-- Could NOT find CURL (missing: CURL_LIBRARY CURL_INCLUDE_DIR)
[...]
CMake Error: The following variables are used in this project, but they are set to NOTFOUND.
Please set them or make sure they are set and tested correctly in the CMake files:
CURL_LIBRARY
    linked by target "vzlogger" in directory /home/pi/vzlogger/vzlogger/src
OPENSSL_CRYPTO_LIBRARY (ADVANCED)
    linked by target "vzlogger" in directory /home/pi/vzlogger/vzlogger/src
OPENSSL_SSL_LIBRARY (ADVANCED)
    linked by target "vzlogger" in directory /home/pi/vzlogger/vzlogger/src

-- Configuring incomplete, errors occurred!
See also "/home/pi/vzlogger/vzlogger/build/CMakeFiles/CMakeOutput.log".
See also "/home/pi/vzlogger/vzlogger/build/CMakeFiles/CMakeError.log".

Note

Why are GnuTLS and OpenSSL used? One of them should be enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we have some code that uses openssl directly, you can see this here for example: https://github.com/volkszaehler/vzlogger/pull/530/files

and then there's the ssl library used by curl.
we might benefit from using an openssl-backed libcurl to reduce the memory footprint.

i wonder why you are seeing NOTFOUND there, because there's libcurl4-dev on the list, which should pull in SOME version of libcurl, (and when adding libcurl-*ssl it becomes redundant.)
i was hoping to avoid demanding a specific ssl library for curl here.
(was also wondering how users would deal with maybe ending up with a not-ssl-enabled version of libcurl - if any distro ships such a thing. i think most usage is via http to localhost anyway.)

(i guess you're not seriously proposing to add a second copy of libunistring-dev, and that's just a copy/paste error.)

Copy link
Contributor Author

@r00t- r00t- Mar 7, 2024

Choose a reason for hiding this comment

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

the dockerfile only installs curl-dev even...

curl-dev \

the ci pipeline installs libbcurl4-openssl-dev:

run: sudo apt-get update && sudo apt-get install libjson-c-dev libcurl4-openssl-dev libmicrohttpd-dev libgcrypt20-dev libsasl2-dev libunistring-dev libmosquitto-dev libgmock-dev libgtest-dev

we should really replace this with a reference to either of those, as those can be automatically tested and will not become stale.

Choose a reason for hiding this comment

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

To recap:
I have been compiling vzlogger by having a look on the instructions on the master branch. And during that I installed missing things and noted down what I installed to get things running. Then I recognized that there is already a PR considering updating the dependencies. I tried to merge my changes into the list. Could happen that I duplicated / missed something by manually merging my findings into it. So does not have to be perfect what I noted as a comment.

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