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 installation paths. #32

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

Conversation

jgoppert
Copy link

GNUInstallDirs was not defined when target_include_directories was set. This caused it to be blank and the cmake target import failed. User projects importing the cmake target should use include qualisys_cpp_sdk/RTProtocol.h etc.

GNUInstallDirs was not defined when target_include_directories
was set. This caused it to be blank and the cmake target import
failed. User projects importing the cmake target should use
include qualisys_cpp_sdk/RTProtocol.h etc.

Signed-off-by: James Goppert <[email protected]>
@gorghino
Copy link

I confirm this is required on your Nvidia jetson platforms.
@Capelliexp

@MaximilienNaveau
Copy link

I did this PR before I saw your @jgoppert .
I believe this simple change fix your issue as well:
#45

@@ -648,7 +648,7 @@ bool CRTProtocol::GetCapture(const char* pFileName, bool bC3D)
}
else
{
sprintf(maErrorStr, "No packet received. %s.", maErrorStr);
snprintf(maErrorStr, 1060, "No packet received. %s.", maErrorStr);

Choose a reason for hiding this comment

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

should this be part of the PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Using snprintf(maErrorStr, 1060, "No packet received. %s.", maErrorStr); to avoid the compiler warning and potentially preventing buffer overflows seems reasonable. 👍 Although I suppose it could be a separate PR.

Maybe it's obvious, but where did a size of 1060 come from?

Choose a reason for hiding this comment

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

I agree this should be s separate PR. Also curious where 1060 comes from, as maErrorStr is 1024 characters.

@OliverGlandberger
Copy link
Contributor

OliverGlandberger commented Nov 7, 2024

Like @MaximilienNaveau pointed out, his approach in #45 also addresses this issue by hardcoding include as the install path. However, I think using include(GNUInstallDirs) to set CMAKE_INSTALL_INCLUDEDIR provides more flexibility by allowing the path to adapt to system-specific conventions.

If there are any additional insights I should be aware of, please let me know. (:

@MaximilienNaveau
Copy link

Like @MaximilienNaveau pointed out, his approach in #45 also addresses this issue by hardcoding include as the install path. However, I think using include(GNUInstallDirs) to set CMAKE_INSTALL_INCLUDEDIR provides more flexibility by allowing the path to adapt to system-specific conventions.

If there are any additional insights I should be aware of, please let me know. (:

I believe this PR is better suited to be merge than mine I closed mine in response

Copy link
Contributor

@nim65s nim65s left a comment

Choose a reason for hiding this comment

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

I did the same in nim65s@c39f643, but forgot to open a PR, so thanks for this !

@nim65s
Copy link
Contributor

nim65s commented Nov 7, 2024

@jgoppert : could you consider adding nim65s@1ba8cdb to this PR ?
Otherwise I could make another one, but it is built on yours, so I guess it would be cleaner.

Copy link

@bahlborn bahlborn left a comment

Choose a reason for hiding this comment

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

Move sprintf->snprintf change to another PR.
Verify use of 1060 as buffer size.

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.

6 participants