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

CRAN check encounters compilation warnings (r-release-macos) #9

Closed
davidchall opened this issue Nov 28, 2023 · 20 comments
Closed

CRAN check encounters compilation warnings (r-release-macos) #9

davidchall opened this issue Nov 28, 2023 · 20 comments

Comments

@davidchall
Copy link

The ipaddress package encounters CRAN check warnings on r-release-macos, caused by compilation warnings from AsioHeaders:

Found the following significant warnings:
    /Volumes/Builds/packages/big-sur-arm64/Rlib/4.3/AsioHeaders/include/asio/associated_allocator.hpp:100:49: warning: 'allocator<void>' is deprecated [-Wdeprecated-declarations]
    /Volumes/Builds/packages/big-sur-arm64/Rlib/4.3/AsioHeaders/include/asio/execution/any_executor.hpp:611:47: warning: 'allocator<void>' is deprecated [-Wdeprecated-declarations]
    /Volumes/Builds/packages/big-sur-arm64/Rlib/4.3/AsioHeaders/include/asio/io_context.hpp:231:36: warning: 'allocator<void>' is deprecated [-Wdeprecated-declarations]
    /Volumes/Builds/packages/big-sur-arm64/Rlib/4.3/AsioHeaders/include/asio/io_context.hpp:856:43: warning: 'allocator<void>' is deprecated [-Wdeprecated-declarations]
    /Volumes/Builds/packages/big-sur-arm64/Rlib/4.3/AsioHeaders/include/asio/io_context.hpp:859:37: warning: 'allocator<void>' is deprecated [-Wdeprecated-declarations]
    /Volumes/Builds/packages/big-sur-arm64/Rlib/4.3/AsioHeaders/include/asio/io_context.hpp:860:29: warning: 'allocator<void>' is deprecated [-Wdeprecated-declarations]
    /Volumes/Builds/packages/big-sur-arm64/Rlib/4.3/AsioHeaders/include/asio/system_executor.hpp:171:54: warning: 'allocator<void>' is deprecated [-Wdeprecated-declarations]
    /Volumes/Builds/packages/big-sur-arm64/Rlib/4.3/AsioHeaders/include/asio/system_executor.hpp:175:28: warning: 'allocator<void>' is deprecated [-Wdeprecated-declarations]
    /Volumes/Builds/packages/big-sur-arm64/Rlib/4.3/AsioHeaders/include/asio/system_executor.hpp:444:45: warning: 'allocator<void>' is deprecated [-Wdeprecated-declarations]
    /Volumes/Builds/packages/big-sur-arm64/Rlib/4.3/AsioHeaders/include/asio/system_context.hpp:39:12: warning: 'allocator<void>' is deprecated [-Wdeprecated-declarations]
    /Volumes/Builds/packages/big-sur-arm64/Rlib/4.3/AsioHeaders/include/asio/detail/recycling_allocator.hpp:97:30: warning: 'allocator<void>' is deprecated [-Wdeprecated-declarations]

I suspect these warnings started appearing because CRAN have updated their compiler toolchain on r-release-macos, but I don't have further details. Do you have advice on how to resolve these, or will this require a new release of AsioHeaders?

@eddelbuettel
Copy link
Owner

Hi @davidchall thanks for posting this. Now I don't quite recall what I saw but I think i saw some package somewhere having an issue. Maybe it was yours (scratches chin...).

The best best is probably to look upstream if there is a fix 'for free' in a new release. I apparently last upgrade in June of 2022 to 1.22.1 (and I recall upstream was somewhat quiet leading up to it). But I now see 1.28.0 at https://think-async.com/Asio/ so that will likely fix it. I will try to address this 'pronto' but I have a few other changes on my plate due to the format string checks added by r-devel. If you happen to have spare cycles help with a PR would be great. As I recall it is mostly a matter of copying / rsyncing the files in and then running a rev.dep check.

@davidchall
Copy link
Author

davidchall commented Nov 28, 2023

Hi @eddelbuettel - Unfortunately, I still see allocator<void> in the master branch:
https://github.com/chriskohlhoff/asio/blob/fa27820c05afd740fa2adc1ecfb9da5afe026443/asio/include/asio/associated_allocator.hpp#L113

It appears some users have encountered this compiler warning since Jan 2021: chriskohlhoff/asio#785

Update: Asio author says the compilation warning is wrong: chriskohlhoff/asio#290 (comment)

@eddelbuettel
Copy link
Owner

Ouch. Now we need a little bit of creative thinking ....

@davidchall
Copy link
Author

davidchall commented Nov 29, 2023

Hi @eddelbuettel - I think I have a better understanding of what's gone wrong now.

Here are the latest recommendations:

In the meantime, please suppress deprecation warnings in your project settings, i.e. define _LIBCPP_DISABLE_DEPRECATION_WARNINGS. Or alternatively, upgrade to latest Xcode and compile with -std=c++20.

But I'm still not sure what's the best approach for CRAN. My understanding is that they typically don't want us to suppress compiler warnings.

@eddelbuettel
Copy link
Owner

eddelbuettel commented Nov 29, 2023

They don't but they only control for what they can control for -- usually they check (via grep ie regexp from R) for the e.g.

 # pragma GCC diagnostic ignored "-Wnon-virtual-dtor"

I need to comment out for BH. On the other hand I still have global suppressor of nags for Armadilo on in RcppArmadillo "but they do not know" as you'd need semantic analysis per LSP or clangd or whatever.

So I think we're good. Can you try the #define locally?

@davidchall
Copy link
Author

Hi @eddelbuettel - In davidchall/ipaddress#95, I've successfully reproduced the macOS compiler warning (build). Then in the following commit I've silenced the warning by adding the macro to the Makevars file (build).

I think this is what you asked me to confirm. Are the next steps for you to incorporate this macro into AsioHeaders?

@eddelbuettel
Copy link
Owner

Spot on. I suspect we can do that centrally via a PR (but then need to remember to patch each time going forward). Or is recommending to add to Makevars "good enough" and it remains in client packages? Just thinking out aloud.

Thoughts?

@davidchall
Copy link
Author

My preference is to add this to AsioHeaders for a couple of reasons:

  1. Keeping the fix located with the source allows downstream package developers to use AsioHeaders transparently.
  2. Enabling the macro in a header file provides an opportunity to add conditional logic (e.g., operating system, compiler), so it's only enabled where necessary.

Regarding repeated patches, I suspect (perhaps naively) this will be a one-time patch, because:

  1. Historically, AsioHeaders doesn't update the underlying Asio frequently (previous update was July 2020),
  2. When CRAN next updates its macOS toolchain, this issue will likely resolve.

@eddelbuettel
Copy link
Owner

Are we talking about the same thing? inst/include/ only contains upstream files. Hence the need to patch.

Or am I missing something? Apologies if I do as I have my head in multiple concurrent conversations ...

@eddelbuettel
Copy link
Owner

So one way around it (via the beloved Fundamental Theorem of Software Engineering) would be to add a new header which includes the #define before including the default asio header(s).

@davidchall
Copy link
Author

Are we talking about the same thing? inst/include/ only contains upstream files. Hence the need to patch.

Or am I missing something? Apologies if I do as I have my head in multiple concurrent conversations ...

Sorry if I made this confusing. Here's my expectation of how this could work:

  1. [Optional] AsioHeaders updates bundled Asio to 1.28.0
  2. AsioHeaders patches to conditionally define macro (perhaps at the top of each Asio header file?)
  3. Publish new version of AsioHeaders
  4. CRAN check warning is resolved for downstream packages (e.g., ipaddress)
  5. [Optional] If AsioHeaders updates bundled Asio, will need to repeat patch
  6. CRAN updates macOS toolchain, which resolves compiler warning
  7. If AsioHeaders updates bundled Asio, patch no longer required

So one way around it (via the beloved Fundamental Theorem of Software Engineering) would be to add a new header which includes the #define before including the default asio header(s).

This wouldn't work for ipaddress because it includes specific headers (not a default header). See code search results.

@eddelbuettel
Copy link
Owner

Can you not separate out the #define you need and do it your end?

I would love to be responsible for fewer recurring steps if I can. Maybe I do codesearch later to see how other users of the package pull it in.

@eddelbuettel
Copy link
Owner

There are two reverse dependencies (both LinkingTo) for AsioHeaders: your ipaddress, and Winston's websocket.

I think adding a simple -D..... to their respective Makevars is a pretty attractive option that closes this issue.

@davidchall
Copy link
Author

Yeah, I initially thought that was like using a sledgehammer to hit a nail, but it's much simpler than the other options. I'll go with that.

@eddelbuettel
Copy link
Owner

It's also more robust. You won't have to depend on me not forgetting to patch and all that.

And remind me again: we only need this on macOS until the compiler updates?

@davidchall
Copy link
Author

And remind me again: we only need this on macOS until the compiler updates?

I believe so. I've not seen the warning from the default macOS image on GitHub Actions or my local machine, and these both use more recent toolchains. I had to manually override the macOS SDK path to force this warning to appear.

@eddelbuettel
Copy link
Owner

Can you think of a clever conditional way to inject the #define into src/Makevars only when needed? For example I decided to play games here where we pick C++14 only when needed (on R older than 4.2.0 which made it a defaul). (That was mostly an exercise. Leaving it empty and letting people on older systems fight for themselves seems good enough for CRAN even if not technically good enough for all of us...)

@davidchall
Copy link
Author

Unfortunately, I couldn't find a way to do it. So I plan to use the sledgehammer and keep an eye on when CRAN updates above "MacOSX11.3.sdk".

@eddelbuettel
Copy link
Owner

Yeppers so I think we are circling around 'unconditionally add the #define' as it helps where needed, does not harm otherwise, and is easy to administrate.

@eddelbuettel
Copy link
Owner

Would appear you have a new version at CRAN so closing this.

@eddelbuettel eddelbuettel closed this as not planned Won't fix, can't repro, duplicate, stale Dec 2, 2023
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

No branches or pull requests

2 participants