Skip to content

Conversation

@eddelbuettel
Copy link
Member

This PR adds a little bit of polish and simplifies the code in configure and src/Makevars.in.

We also turn on continuous integration for macos-13 which is a x86_64 platform and hence without (current) blp support providing a test of the fallback build.

@eddelbuettel eddelbuettel requested a review from johnlaing March 13, 2025 14:39
@eddelbuettel eddelbuettel force-pushed the feature/config_polish branch from d95b113 to 3b860b8 Compare March 14, 2025 20:23
@eddelbuettel
Copy link
Member Author

Ok -- I made one more pass over it and redefined things in terms of an 'affirming' compile-time variable -DHaveBlp set via src/Makevars when we have Blp available. Also set for the Windows build.

With that I rewrote the #if / #else logic in terms of #if defined(HaveBlp) and also brought the #if into each of the exported functions. That means we again have exactly one function interface either way. I find this a little cleaner.

@johnlaing Would greatly appreciate another review. It's not super urgent but I would love for this to get to CRAN (along with other PR #404 which I know you looked at but for which I do not think I have from you per se).

@eddelbuettel eddelbuettel force-pushed the feature/config_polish branch from 3b860b8 to 74e3400 Compare March 16, 2025 22:16
@eddelbuettel
Copy link
Member Author

Rebased on top of merged #404 (and thanks for looking that over) and should be clean and ready.

@eddelbuettel
Copy link
Member Author

@johnlaing The PR should be ready to merge. There is no code change, but a more or less cosmetic change of making the decision to build with Blp support an opt-in (an actual #define rather than absence of a not-defined) and brings the condition test inside each exported function. That is cleaner, but possibly a little more unwieldy style-wise. On balance it is better.

This will achieve the stated goal of no longer failing to compile where there is no Blp support (as e.g on ancient macos as at CRAN) and I would like to get it there. If you are to tied up to review let me know, I am confident that we can proceed here but I would appreciated the check from an experienced second set of eyes. Plus, of course, if you have time, a local build and run.

@johnlaing
Copy link
Contributor

I prioritized the other PR which added a new feature and have not had time to review this one. I would like to look and potentially will be able to do so this weekend.

@eddelbuettel
Copy link
Member Author

Given that the commits were made ten days ago, I will likely take absense of a new issue as implicit consent and move along merging and releasing.

@johnlaing
Copy link
Contributor

It's not implicit consent, it's I haven't had time to review. If you want to go ahead and merge anyway that's up to you.

@eddelbuettel
Copy link
Member Author

Rank ordering of preferences. If you could not make time during ten days, I cannot have reasonable expectations that will change so I may decide to move ahead.

I did the work to get a fix to CRAN, and that is still the goal.

@eddelbuettel
Copy link
Member Author

I have a few more other packages in my queue that I am taking care of so I can wait until Monday or Tuesday. Should you find time to test this before then it would be greatly appreciated. If you cannot, then I will most likely proceed without it.

@johnlaing
Copy link
Contributor

All tests pass with a live Bloomberg, so that's great. I think this approach to the preprocessor is much more clear as well.

Not directly a consequence of this PR, but we've been sliding in this direction: configure could probably use some refactoring. The logic seems redundant and it appears that the cpu variable is no longer set in every path. Perhaps a future project.

@eddelbuettel
Copy link
Member Author

Thank you for checking! Really really appreciate it.

Yes. Configure could change, but it is a bit of chore because the blp repo holding the binary blobs also has to change. When we last refactored I (mistakenly!) thought we could preserve two platforms for macOS (given that we once had working object code from them for x86_64 aka amd64 -- that never worked). So in favour of revisiting but let's ship what we have (cleaned up now) in code to CRAN.

Speaking of cleanups, there is also some polish we could apply to things below src/. Some of the (excellent but early) code by Whit could get updated to newer paradigms. Then again ... that's only for style points.

I'll ship this to CRAN 'soon' and then we get rid of the three stoooopid ERRORs on macOS because the maintainer for macOS would never listen to our requests to not force-build. Shoud have done this sooner. Oh well -- hindsight bias.

@eddelbuettel
Copy link
Member Author

I will merge now, that give R-universe a moment to build binaries. Just in case, the previous iterations all look good already.

@eddelbuettel eddelbuettel merged commit cd63a88 into master Mar 29, 2025
6 checks passed
@eddelbuettel eddelbuettel deleted the feature/config_polish branch March 29, 2025 20:26
@eddelbuettel
Copy link
Member Author

Also see here: cpu is used in some code paths. I have not touched the code in two weeks, but I believe when I had the three day code sprint I initialized had cpu commented out and reverted.

Rblpapi/configure

Lines 110 to 112 in 93661dd

if [ ! -f ${blpLibrary} ]; then
download https://github.com/Rblp/blp/raw/${blpBranch}/${platform}${flavour}${cpu}/blpLibrary.tar.gz
else

@eddelbuettel
Copy link
Member Author

Now at CRAN. Thanks for your repeated checks, and the cleanup PR!

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.

3 participants