Skip to content

Update location of patched file for CRAN #3940

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.

Sign up for GitHub

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

Jump to bottom
Merged
rcurtin merged 3 commits into from
May 20, 2025
Merged

Update location of patched file for CRAN #3940

rcurtin merged 3 commits into from
May 20, 2025

Conversation

@rcurtin
Copy link
Member

@rcurtin rcurtin commented May 15, 2025

We have a manual patch for CRAN in stb_image_write.h, because CRAN complains about the use of sprintf. The patch is simple---just modify and use snprintf instead.

However, I noticed that this patch is not being applied to the R sources anymore; this is because we are now bundling STB with mlpack instead of depending on it from external sources.

Therefore, now, we need to directly patch the bundled file (and we need to do it always).

There's no need for a new release specifically for CRAN after this PR---I just made the change manually to the 4.6.1 tarball.

@eddelbuettel
Copy link
Contributor

eddelbuettel commented May 15, 2025

Why not update the now-vendored STB and record the diff? Doing it on each compilation seems ... repetitive? But if you very strongly prefer immutable upstream sources it is one way to go. Entirely your call.

@rcurtin
Copy link
Member Author

rcurtin commented May 15, 2025

I'd prefer to keep it immutable because when we update our vendored STB files, it would be really easy to overwrite the changes that we have intentionally added (e.g. sprintf -> snprintf), and if we updated STB the diff might be large---so even if we included comments like // NOTE: mlpack-specific change, it would be easy to miss that during the review. I'm not sure if I'm being overly paranoid or what. Maybe in the future we can revisit this; I totally get where you're coming from. Open to other ideas if we can ensure that this particular change won't get lost.

github-actions[bot]
github-actions bot approved these changes May 17, 2025
View reviewed changes
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Second approval provided automatically after 24 hours. 👍

@rcurtin rcurtin merged commit ecee144 into mlpack:master May 20, 2025
14 of 15 checks passed
@rcurtin rcurtin deleted the cran-stb-image-location branch May 20, 2025 19:38
@rcurtin rcurtin mentioned this pull request May 22, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@shrit shrit shrit approved these changes

@github-actions github-actions[bot] github-actions[bot] approved these changes

Assignees

No one assigned

Labels

c: automatic bindings c: build system t: bugfix

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@rcurtin @eddelbuettel @shrit