Skip to content

Updates to upstream asio 1.28.2#11

Merged
eddelbuettel merged 6 commits intoeddelbuettel:masterfrom
shikokuchuo:asio-1.28
Apr 8, 2025
Merged

Updates to upstream asio 1.28.2#11
eddelbuettel merged 6 commits intoeddelbuettel:masterfrom
shikokuchuo:asio-1.28

Conversation

@shikokuchuo
Copy link
Copy Markdown
Contributor

Closes #10.

This is the PR we've prepared @eddelbuettel.

On top of the update, there is one small patch, which ensures that snprintf is always used rather than sprintf, which R CMD check would also complain about.

Thanks!

Copy link
Copy Markdown
Owner

@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.

I made some comments about things I think we should consider changing. Nothing super-set in stone but established best practice.

Comment thread ChangeLog Outdated
@@ -1,3 +1,10 @@
2025-04-08 Dirk Eddelbuettel <edd@debian.org>
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Well this is wrong because the commit is yours not mine.

Can we simplify and make it more truthful, ie you add a Changelog entry about the changes to inst/include/* and if possibly make no other changes?

A patch for sprintf -> snprint is sensible too, it may make sense to keep it in a local file (say local/change_to_1.28.2.patch or something like that,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've limited this now to the library update and the patch.

For the patch, I think having the mention in the Changelog should be enough. It doesn't look like we update this library very often and down the line it may not even be needed - as long we run revdepchecks, we'd catch any future omission.

I'm not seeing a great benefit to keeping it in another file as we'd still need to patch asio.hpp to include it I suppose.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I still like to see the patch simply as a reminder.

And I don't mean in another file to be included. Simply as a 'note' of changes made due to CRAN. Look at my BH repo, I keep them there too (also to reapply them, of course, but it is more "industrial sized" there).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Right, got it. Done in a22fb6c.

Comment thread inst/COPYRIGHTS Outdated
Comment thread DESCRIPTION Outdated
@eddelbuettel
Copy link
Copy Markdown
Owner

'Files changed: 678' is very scary. Hard to review, but I trust you on the overall change to asio itself. I made some genty nudging proposals for other other minor details, let me know how you feel about those.

@shikokuchuo
Copy link
Copy Markdown
Contributor Author

Thanks @eddelbuettel an impressively fast response! I've applied your suggestions in 061549c.

The large number of files changed are just the library, a straight inclusion from https://sourceforge.net/projects/asio/files/asio/1.28.2%20%28Stable%29/ (for reference).

@eddelbuettel
Copy link
Copy Markdown
Owner

Well the timing and timezones work in our favour. I was actually at the track for not quite an hour earlier, and was just gone for 2+ hours. Back now, all moving in the right direction and I can probably take care of 'shipping this' today.

@eddelbuettel
Copy link
Copy Markdown
Owner

FYI BH has the patches here: https://github.com/eddelbuettel/bh/tree/master/local/patches

@shikokuchuo
Copy link
Copy Markdown
Contributor Author

Still impressive given all the repos you maintain! Thanks again.

Comment thread ChangeLog
2025-04-08 Charlie Gao <charlie.gao@shikokuchuo.net>

* inst/include/asio*: Upgraded to Asio 1.28.2
* inst/include/asio/detail/config.cpp: Ensure ASIO_HAS_SNPRINTF is defined
Copy link
Copy Markdown
Owner

@eddelbuettel eddelbuettel Apr 8, 2025

Choose a reason for hiding this comment

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

Thank you -- we could add the patch file here, but I can also sneak that in when I do my iteration.

Copy link
Copy Markdown
Owner

@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.

This is very clean now. Appreciate the changes.

@eddelbuettel eddelbuettel merged commit 24239a2 into eddelbuettel:master Apr 8, 2025
1 check passed
@shikokuchuo shikokuchuo deleted the asio-1.28 branch April 8, 2025 16:19
@eddelbuettel
Copy link
Copy Markdown
Owner

@shikokuchuo One possible concern: Did you see the changelog / new entry from my last release? Apparently I had to switch a few upstream things to (v)snprintf. But besides the one you fixed the three reverse depends you looked did not toss up any sprintf issues? (I ran a quick reverse depends check too, but that is mostly for R CMD check fail and less about NOTEs.)

eddelbuettel added a commit to RcppCore/rcpp-logs that referenced this pull request Apr 8, 2025
@eddelbuettel
Copy link
Copy Markdown
Owner

Actually the patch file answers that. I suppose upstream did not cast a net wide enough, so you got into trouble on macOS and this addresses this. Did you send that upstream too, just in case?

@shikokuchuo
Copy link
Copy Markdown
Contributor Author

shikokuchuo commented Apr 8, 2025

@shikokuchuo One possible concern: Did you see the changelog / new entry from my last release? Apparently I had to switch a few upstream things to (v)snprintf. But besides the one you fixed the three reverse depends you looked did not toss up any sprintf issues? (I ran a quick reverse depends check too, but that is mostly for R CMD check fail and less about NOTEs.)

This is fine. I just checked through those edits. In 1.28.2, those places all have a ASIO_HAS_SNPRINTF conditional branch now, so we're good with my patch.

@shikokuchuo
Copy link
Copy Markdown
Contributor Author

shikokuchuo commented Apr 8, 2025

Actually the patch file answers that. I suppose upstream did not cast a net wide enough, so you got into trouble on macOS and this addresses this. Did you send that upstream too, just in case?

I think you're referring to 'allocator<void>' is deprecated. That's a separate issue, and in #9 there's a link to a comment from the upstream author saying that's a false positive. We've dealt with it separately in the same way as in #9.

@eddelbuettel
Copy link
Copy Markdown
Owner

eddelbuettel commented Apr 8, 2025

No I was thinking of snprintf but I rushed; looking at your patch again explains why it may not be for upstream. You are essentially ensuring Simon's old macOS-13 setup has it set too. We should be good.

@eddelbuettel
Copy link
Copy Markdown
Owner

Shipped. Bit of a queue there which needs to clear first but might be up in a few hours:

edd@rob:~/git/asioheaders(master)$ ciw.r                        # littler wrapper for ciw::ciw()
     Folder                          Name                Time   Size          Age
     <char>                        <char>              <POSc> <char>   <difftime>
 1: pretest   AsioHeaders_1.28.2-1.tar.gz 2025-04-08 19:09:00   677K   0.07 hours
 2: pretest            tickr_1.0.1.tar.gz 2025-04-08 18:47:00    49K   0.43 hours
 3: pretest             VLF_1.1-03.tar.gz 2025-04-08 18:37:00   496K   0.60 hours
 4: pretest             tcpl_3.2.1.tar.gz 2025-04-08 18:35:00   3.2M   0.63 hours
 5: pretest     PathwaySpace_1.0.1.tar.gz 2025-04-08 18:33:00   4.5M   0.67 hours
 6: pretest             puff_0.1.0.tar.gz 2025-04-08 18:33:00   298K   0.67 hours
 7: pretest    spanishoddata_0.1.1.tar.gz 2025-04-08 18:33:00   1.5M   0.67 hours
 8: pretest             solaR_0.47.tar.gz 2025-04-08 18:20:00   118K   0.88 hours
 9: pretest      rscorecard_0.31.0.tar.gz 2025-04-08 18:19:00   164K   0.90 hours
10: pretest     CloneSeeker_1.0.13.tar.gz 2025-04-08 18:11:00   301K   1.03 hours
11: pretest        featForge_0.1.1.tar.gz 2025-04-08 18:10:00    49K   1.05 hours
12: pretest SPARRAfairness_0.1.0.0.tar.gz 2025-04-08 17:56:00   1.0M   1.28 hours
13: pretest OptHoldoutSize_0.1.0.1.tar.gz 2025-04-08 17:42:00   2.9M   1.52 hours
14: pretest             heck_0.1.4.tar.gz 2025-04-08 17:40:00   567K   1.55 hours
15: pretest       crosswalkr_0.3.0.tar.gz 2025-04-08 17:36:00    72K   1.62 hours
16: pretest       treesitter_0.2.0.tar.gz 2025-04-08 17:30:00   381K   1.72 hours
17: pretest          popdemo_1.3-2.tar.gz 2025-04-08 17:29:00   3.4M   1.73 hours
18: pretest          DeSciDe_1.0.0.tar.gz 2025-04-08 17:28:00   868K   1.75 hours
19: pretest             beam_2.0.4.tar.gz 2025-04-08 17:24:00   260K   1.82 hours
20: pretest           artma_0.1.12.tar.gz 2025-04-08 17:20:00    37K   1.88 hours
21: pretest          FlexReg_1.3.1.tar.gz 2025-04-08 17:19:00   303K   1.90 hours
22: recheck         Rfast2_0.1.5.3.tar.gz 2025-04-08 17:17:00   169K   1.93 hours
23: recheck      formatters_0.5.11.tar.gz 2025-04-08 17:10:00   2.4M   2.05 hours
24: pending    GrowthCurveME_0.1.1.tar.gz 2025-04-08 17:09:00   412K   2.07 hours
25: recheck        rmumps_5.2.1-34.tar.gz 2025-04-08 16:08:00   2.2M   3.08 hours
26: recheck         cmsafops_1.4.1.tar.gz 2025-04-08 14:36:00   296K   4.62 hours
27: recheck          ggplot2_3.5.2.tar.gz 2025-04-08 12:59:00   3.3M   6.23 hours
28: waiting         pbkrtest_0.5.4.tar.gz 2025-04-02 17:30:00    76K 145.72 hours
     Folder                          Name                Time   Size          Age
edd@rob:~/git/asioheaders(master)$ 

@shikokuchuo
Copy link
Copy Markdown
Contributor Author

Amazing! We've enough time on our side 😄

@eddelbuettel
Copy link
Copy Markdown
Owner

All done, not immediately as reckoned, but done:

image

Thanks for the high-quality PR, and I hope it helps the three posit packages dependencing on it.

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.

Consider upsteam update to 1.28.0

2 participants