steve-the-bayesian / BOOM

A C++ library for Bayesian modeling, mainly through Markov chain Monte Carlo, but with a few other methods supported. BOOM = "Bayesian Object Oriented Modeling". It is also the sound your computer makes when it crashes.
GNU Lesser General Public License v2.1
36 stars 15 forks source link

Makefile: fix ar flags #79

Closed barracuda156 closed 8 months ago

barracuda156 commented 11 months ago

On macOS the current code may produce a broken static library due to missing table of contents – because ranlib is not being run. As a substitute for that, s can be added to ar flags.

barracuda156 commented 11 months ago

Otherwise I am getting this when installing BoomSpikeSlab:

/opt/local/bin/g++-mp-13 -std=gnu++17 -dynamiclib -Wl,-headerpad_max_install_names -undefined dynamic_lookup -single_module -multiply_defined suppress -L/opt/local/Library/Frameworks/R.framework/Resources/lib -Wl,-headerpad_max_install_names -L/opt/local/lib -lMacportsLegacySupport -Wl,-rpath,/opt/local/lib/libgcc -arch ppc -o BoomSpikeSlab.so boom_spike_slab_init.o logit_spike_slab_wrapper.o mlm_spike_slab_wrapper.o nested_regression_wrapper.o nnet_wrapper.o poisson_spike_slab_wrapper.o probit_spike_slab_wrapper.o quantile_spike_wrapper.o shrinkage_regression_wrapper.o spike_slab_wrapper.o splines.o /opt/local/Library/Frameworks/R.framework/Versions/4.3/Resources/library/Boom/lib/libboom.a -F/opt/local/Library/Frameworks/R.framework/.. -framework R -Wl,-framework -Wl,CoreFoundation
ld: in /opt/local/Library/Frameworks/R.framework/Versions/4.3/Resources/library/Boom/lib/libboom.a, archive has no table of contents
collect2: error: ld returned 1 exit status
make: *** [BoomSpikeSlab.so] Error 1
ERROR: compilation failed for package ‘BoomSpikeSlab’

After running ranlib on libboom.a, everything works fine, and tests for BoomSpikeSlab pass. I am not sure why Boom installation succeeds despite a broken lib.

steve-the-bayesian commented 11 months ago

This was caught by CRAN for Boom_0.9.13, and was fixed in Boom_0.9.14 (the next day). If you're still having this problem with 0.9.14 please let me know.

On Mon, Dec 18, 2023 at 1:01 PM Sergey Fedorov @.***> wrote:

On macOS the current code may produce a broken static library due to missing table of contents – because ranlib is not being run. As a substitute for that, s can be added to ar flags.

You can view, comment on, or merge this pull request online at:

https://github.com/steve-the-bayesian/BOOM/pull/79 Commit Summary

File Changes

(1 file https://github.com/steve-the-bayesian/BOOM/pull/79/files)

Patch Links:

— Reply to this email directly, view it on GitHub https://github.com/steve-the-bayesian/BOOM/pull/79, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABMVDVMN6TQVJDMXLZO3CA3YKCVJJAVCNFSM6AAAAABA2EB6OGVHI2DSMVQWIX3LMV43ASLTON2WKOZSGA2DONBUG4ZTCMQ . You are receiving this because you are subscribed to this thread.Message ID: @.***>

barracuda156 commented 11 months ago

@steve-the-bayesian Thank you for responding. I actually got the error with 0.9.14. CRAN tarball for it still seems to have rc without s and without ranlib.

steve-the-bayesian commented 11 months ago

Here is the AR line from Boom 0.9.10

libboom.a: ${OBJECTS}

       ${AR} rc $@ $^

strip $@

This matches the line in 0.9.14 and is the method required by CRAN. It builds and links on both my mac and linux machines. I don't think the 's' option or ranlib are needed.

On Mon, Dec 18, 2023 at 1:34 PM Sergey Fedorov @.***> wrote:

@steve-the-bayesian https://github.com/steve-the-bayesian Thank you for responding. I actually got the error with 0.9.14. CRAN tarball for it still seems to have rc without s and without ranlib.

— Reply to this email directly, view it on GitHub https://github.com/steve-the-bayesian/BOOM/pull/79#issuecomment-1861708122, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABMVDVKCQEU56BDOFITFXUDYKCZETAVCNFSM6AAAAABA2EB6OGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNRRG4YDQMJSGI . You are receiving this because you were mentioned.Message ID: @.***>

barracuda156 commented 11 months ago

@steve-the-bayesian It is not needed on every platform, but it is needed on some.

See, for example, this issue: https://github.com/PolMine/RcppCWB/pull/80#issuecomment-1594583581 And commit that fixed it: https://github.com/PolMine/RcppCWB/commit/c6df1cc42f545732404de4740d0e0bbef1c41cb5

barracuda156 commented 11 months ago

Builds fine with updated ar flags on every macOS: https://ports.macports.org/port/R-Boom/details

steve-the-bayesian commented 11 months ago

Sergey, does this need to be the CRAN version? I build R packages for Boom and related packages (including bsts) locally using scripts located in the BOOM/install directory. I could add a 'macports' flag that would instruct the script to build the library however you like, and then we wouldn't have CRAN's heavy portability rules as a constraint.

Have a look at .../install/create_boom_rpackage and let me know if that approach fits your needs.

On Tue, Dec 19, 2023 at 10:48 PM Sergey Fedorov @.***> wrote:

Builds fine with updated ar flags on every macOS: https://ports.macports.org/port/R-Boom/details

— Reply to this email directly, view it on GitHub https://github.com/steve-the-bayesian/BOOM/pull/79#issuecomment-1863939455, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABMVDVM7ZBLRRH2SCRVVEWTYKKC4FAVCNFSM6AAAAABA2EB6OGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNRTHEZTSNBVGU . You are receiving this because you were mentioned.Message ID: @.***>

barracuda156 commented 11 months ago

@steve-the-bayesian Well, if the point is just fixing it for Macports, that has been done already locally. However, that leaves the build broken for anyone who tries to build it on affected platforms without Macports. Unfortunately, it does not even fail as such, but leaves the library unusable, which can be frustrating for someone who is not familiar with the issue.

Of course, if there is a known case where adding s breaks the library for some other platform, it should be avoided in a general case. On macOS it is fine across the board, this is pretty sure. I cannot guarantee the same for every possible Unix-like OS, but it seems it is universally supported.

CRAN's heavy portability rules as a constraint

While I am aware that CRAN normally does not bother about anything besides a few bleeding edge versions, I do not think they have a policy of breaking something out of principle :) Having said that, this is of course up to you to decide whether better portability matters or not. IMO, the fix is trivial and does not introduce any complications in terms of future efforts to support it.

steve-the-bayesian commented 11 months ago

Gotcha. I'll run it by CRAN. If they don't object I'll keep the change. If they do (and they're full of surprises) then I'm open to other solutions.

On Wed, Dec 20, 2023 at 11:00 AM Sergey Fedorov @.***> wrote:

@steve-the-bayesian https://github.com/steve-the-bayesian Well, if the point is just fixing it for Macports, that has been done already locally. However, that leaves the build broken for anyone who tries to build it on affected platforms without Macports. Unfortunately, it does not even fail as such, but leaves the library unusable, which can be frustrating for someone who is not familiar with the issue.

Of course, if there is a known case where adding s breaks the library for some other platform, it should be avoided in a general case. On macOS it is fine across the board, this is pretty sure. I cannot guarantee the same for every possible Unix-like OS, but it seems it is universally supported.

CRAN's heavy portability rules as a constraint

While I am aware that CRAN normally does not bother about anything besides a few bleeding edge versions, I do not think they have a policy of breaking something out of principle :) Having said that, this is of course up to you to decide whether better portability matters or not. IMO, the fix is trivial and does not introduce any complications in terms of future efforts to support it.

— Reply to this email directly, view it on GitHub https://github.com/steve-the-bayesian/BOOM/pull/79#issuecomment-1864988327, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABMVDVKOLM5OJ73DQLX3EQLYKMYUTAVCNFSM6AAAAABA2EB6OGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNRUHE4DQMZSG4 . You are receiving this because you were mentioned.Message ID: @.***>

barracuda156 commented 11 months ago

Thank you!