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

Update package structure for compatibility with future rstan #43

Closed
wants to merge 3 commits into from
Closed

Update package structure for compatibility with future rstan #43

wants to merge 3 commits into from

Conversation

andrjohns
Copy link

This PR updates your package's structure to fully delegate to rstantools for installation, to help ensure compatibility with future versions of rstan.

I've included a modification to the configure.win script to skip the model compilation on 32-bit windows, and also made a minor update to the cov.stan model to workaround a bug in Stan 2.31

Let me know if you have any questions/clarifications. Thanks!

@cdriveraus
Copy link
Owner

Thanks, but still tries to compile on 32 bit:
https://win-builder.r-project.org/lam00pUh4l9n/00install.out

@andrjohns
Copy link
Author

Ah figured it out. It worked locally because R would only configure and build for the current architecture. When submitting to winbuilder, it looks like it configures for 64bit and then builds for both (equivalent to adding the Biarch: true flag to the DESCRIPTION. Rather than figuring out some makevars/configure trickery to ignore the sources on 32-bit, I've opted to instead add a compiler flag to reduce memory usage during compilation (at the cost of increased compile times) which allows the package to build under 32-bit

@cdriveraus
Copy link
Owner

ok interesting. Usually CRAN seems to hate playing with compiler flags but I think you have more familiarity in this regard so I'll try and see. ctsem already seems pretty borderline on their timeouts, but maybe if it's just to get by on the oldrelease it will be fine.

@cdriveraus
Copy link
Owner

@andrjohns
Copy link
Author

That log doesn't have the new flag (--param ggc-min-expand=10), and I've confirmed that it builds fully under 32-bit locally. Any chance that an old version was uploaded or similar?

@cdriveraus
Copy link
Owner

just double checked my upload, it has the updated configure.win. I've occasionally had weird problems with cached / old packaged uploads on winbuilder though, will try again with an updated version...

@cdriveraus
Copy link
Owner

@cdriveraus
Copy link
Owner

ctsem_3.7.5.tar.gz
Here is the uploaded package

@andrjohns
Copy link
Author

The configure.win script in that package has a typo: >> src/Makevars.wi (.wi instead of .win)

@cdriveraus
Copy link
Owner

Right that was silly. Works good now, thanks. Any idea how other packages using rstantools avoid the cran check note: LinkingTo' field is unused: package has no 'src' directory
If I include an empty source directory they like that even less

@andrjohns
Copy link
Author

For that you could either have a message to CRAN that the sources are generated on install, or you could try having a src folder with just the rstantools Makevars files which should get automatically replaced on installation

@cdriveraus
Copy link
Owner

Needed a real source file in the src folder, so left RcppExports.cpp, now it works. Thanks for your help on this.

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.

2 participants