Skip to content

Conversation

@rcurtin
Copy link
Member

@rcurtin rcurtin commented May 31, 2024

I managed to knock this out during a meeting. The changes are pretty simple:

  • Include stb_image.h and stb_image_write.h as part of the R package distribution. This is important because we are not guaranteed that a user has these present on their system (which I think will cause a package installation failure if they don't).
  • Patch use of sprintf() inside stb_image_write.h to snprintf(). This is actually already done in a slightly different way in an open STB patch, although that still leaves an sprintf() call in, so maybe the patch I did is better, at least for CRAN.

I patched this locally in the version of mlpack I just submitted to CRAN.

This fixes #3724.

@eddelbuettel
Copy link
Contributor

Shouldn't the modified stb_image.h be included in the PR too? Or am I missing something?

@rcurtin
Copy link
Member Author

rcurtin commented May 31, 2024

I just copy it from wherever CMake found it at the top level (e.g. ${STB_IMAGE_INCLUDE_DIR}/stb_image.h). That is guaranteed to exist, otherwise StbImage_FOUND would not be set (and the files wouldn't be included at all).

Copy link
Contributor

@eddelbuettel eddelbuettel left a comment

Choose a reason for hiding this comment

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

Should be good to go provided the file good so 🚢

@eddelbuettel
Copy link
Contributor

Kudos!

image

@rcurtin rcurtin merged commit ef91d8e into mlpack:master Jun 3, 2024
@rcurtin rcurtin deleted the r-cran-stb branch June 3, 2024 13:02
This was referenced Sep 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[R] Switch sprintf to snprintf

3 participants