Fixes

[ellert]

Similar to eddelbuettel/rinside#21

[eddelbuettel]

Can you detail why this is needed / what systems would need it or benefit from.

And let me now ask you a second time today to file issue tickets BEFORE throwing a PR over the fence. I have no motivation to apply this when I do not know what (perceived, real, at current for me irreproducible as not existing) problem this is meant to address. Sorry to be harsh -- I really appreciate your enthusiasm but you are not making this as easy as you might.

[ellert]

I really thought that the reference to the RInside issue in the original comment would be sufficient, since the issues are exactly the same. But I can repeat the rationale here.

When creating an RPM for R-littler, the RPM creation fails because the buildroot path was detected to be present in some of the files being packaged:

+ /usr/lib/rpm/check-buildroot
Binary file /home/ellert/rpmbuild/BUILDROOT/R-littler-0.3.0-1.fc23.x86_64/usr/bin/r matches
/home/ellert/rpmbuild/BUILDROOT/R-littler-0.3.0-1.fc23.x86_64/usr/src/debug/R-littler-0.3.0/littler/src/littler.h:"R_LIBRARY_DIR","/home/ellert/rpmbuild/BUILDROOT/R-littler-0.3.0-1.fc23.x86_64/usr/lib64/R/library",
/home/ellert/rpmbuild/BUILDROOT/R-littler-0.3.0-1.fc23.x86_64/usr/src/debug/R-littler-0.3.0/littler/src/littler.h:"R_LIBS","/home/ellert/rpmbuild/BUILDROOT/R-littler-0.3.0-1.fc23.x86_64/usr/lib64/R/library",
/home/ellert/rpmbuild/BUILDROOT/R-littler-0.3.0-1.fc23.x86_64/usr/src/debug/R-littler-0.3.0/littler/src/littler.h:"R_PACKAGE_DIR","/home/ellert/rpmbuild/BUILDROOT/R-littler-0.3.0-1.fc23.x86_64/usr/lib64/R/library/littler",
Found '/home/ellert/rpmbuild/BUILDROOT/R-littler-0.3.0-1.fc23.x86_64' in installed files; aborting

By filtering out the variables containing the buildroot path the RPM build succeeds.

The linking should inherit all the compiler flags from the build of R itself. Looking at the build log it can be seen that some of the linker flags are missing. This pull request addresses this issue by adding the missing LDFLAGS.

I haven't come across anyone making such a huge distinction between an "issue" and a "pull request" before. For me, an "issue" is a bug report without a proposed patch, and a "pull request" is a bug report with a proposed patch. So which one you submit is a question about whether you want to attach a patch or not. I consider it to be a good idea to attach a proposed patch to a bug report if you can. It it one of the better ways to explain just what the bug you are reporting really is about. As upstream you are of course fully free to reject any proposed patch as you see fit. Either if you consider to reported bug to not be a bug at all, or if you think there is a better way to address the issue.

[eddelbuettel]

When creating an RPM for R-littler, the RPM creation fails

Thanks, I see now that you were kind enough to actually document an issue.

I haven't come across anyone making such a huge distinction between an "issue" and a "pull request"

We care about code quality and documented behaviour and will not randomly apply patches. If you see something wrong, file an issue, we discuss a course of action and someone (maybe you) proposes a PR. See Contributing.md over at Rcpp, the same spirit applies here.

[eddelbuettel]

My exclude these ? I don't even have them:

edd@max:~/git/littler/src(master)$ Rscript scripts/littler.R 
const char *R_VARS[] = {
"R_ARCH","",
"R_BROWSER","xdg-open",
"R_BZIPCMD","/bin/bzip2",
"R_DEFAULT_PACKAGES","datasets,utils,grDevices,graphics,stats",
"R_DOC_DIR","/usr/share/R/doc",
"R_GZIPCMD","/bin/gzip -n",
"R_HOME","/usr/lib/R",
"R_INCLUDE_DIR","/usr/share/R/include",
"R_LIBS_SITE","/usr/local/lib/R/site-library:/usr/lib/R/site-library:/usr/lib/R/library",
"R_PAPERSIZE","letter",
"R_PAPERSIZE_USER","letter",
"R_PDFVIEWER","/usr/bin/xdg-open",
"R_PLATFORM","x86_64-pc-linux-gnu",
"R_PRINTCMD","/usr/bin/lpr",
"R_RD4PDF","times,inconsolata,hyper",
"R_SHARE_DIR","/usr/share/R/share",
"R_SYSTEM_ABI","linux,gcc,gxx,gfortran,?",
"R_TEXI2DVICMD","/usr/bin/texi2dvi",
"R_UNZIPCMD","/usr/bin/unzip",
"R_ZIPCMD","/usr/bin/zip",
NULL };
edd@max:~/git/littler/src(master)$ 

[ellert]

When run from inside a package install there are more variables present then when run from the command line. Try this:

$ tar -z -x -f littler_0.3.0.tar.gz 
$ mkdir /tmp/littler-test-install
$ R CMD INSTALL -l /tmp/littler-test-install littler
* installing *source* package ‘littler’ ...
[ ... skipping some lines of the output ... ]
* DONE (littler)
$ grep littler-test-install littler/src/littler.h
"R_LIBRARY_DIR","/tmp/littler-test-install",
"R_LIBS","/tmp/littler-test-install",
"R_PACKAGE_DIR","/tmp/littler-test-install/littler",

[eddelbuettel]

Ok, I can confirm that the -l DIR switch sets these.

But I am afraid that if you excluded those directories, then littler may not find your libraries meaning library() or require() may fail to install. Which would be pretty bad.

I'd would really like to exclude this part. Adding AM_LDFLAGS is harmless, hurts nobody and may help you. Doing this, I fear, may cripple littler. In ten years of littler nobody has documented an error. Do I read you right that they just annoy you? That is no reason to risk altering behaviour.

Can you revert this second commit?

[ellert]

It is not annoyance. Without filtering those out rpmbuild refuses to create the RPM. It detects the presence of the build directory in the files being packaged as a packaging error and refuses to create the rpms.

[eddelbuettel]

Ok. What remains them is to demonstrate that removing these does not impair functionality.

[eddelbuettel]

So I mod'ed by local /usr/bin/r which is tickled by a couple of cronjobs and other recurrent runs. If it behaves I'll apply this.

Please try to motivate your changes better, and please do file issue tickets before PRs. It makes reasoning about PRs a lot easier.

[eddelbuettel]

Ok, it behaved. I will fold this in now.