-
Notifications
You must be signed in to change notification settings - Fork 993
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 oldrel-macos install fails #4640
Comments
Although, if it couldn't find zlib, I'd expect some errors from the linking step, and those aren't there. |
PR #4442 is related. It isn't merged yet. If this osx install problem is related to the .so name, maybe that PR will help. |
Correspondence with Simon: Hi Simon, data.table 1.13.0 was just published on CRAN.
What should I do from here? Or maybe our Best, Matt Matt, first, you cannot assume pkg-config - why would anyone use pkg-config to check for -lz is entirely beyond me - you don't seem to rely on glib. On macOS libz is part of the system so there is no need for pkg-config, it's one-liner in autoconf to check for libz. Also please note that blowing away user-supplied PKG_* flags is a really bad idea. You should honour either PKG_* flags or at least the standard CPPFLAGS/CFLAGS/LIBS so the user can fix any issues you have in your detection (e.g. the user user should be able to supply their own omp flags, but can't in your package - see the omp macOS link we discussed). The recommended practice is to set flags to include both, then perform tests and then substitute the result to make sure you have a consistent setup. Finally, the reason it fails is because your script doesn't generate Makevars (due to the libz issue) so it is the standard installation that occurs. It may be helpful using standard tools like autoconf which make it much easier to log issues and make sure you don't have bugs in your configuration generation. Out of curiosity, why do you need to re-name the dylib? Cheers, |
I suggest I write down my thoughts on this here so that others who know configuration and macos better than me can give their input before we get back to Simon...
I guess I am pretty stupid. I did it because adding Autoconf to data.table apparently involves including the whole Autoconf program (which is a script) to the package tar.gz. If I remember correctly, and my stupidity is not failing me, the size of Autoconf approached 1MB and tripped the CRAN 5MB size limits. It seems odd to me that every package should include a copy of the whole Autoconf program, so why not use
I didn't know this was even an idea we had. I don't know which flags we can and can't blow away. It is beyond me. I looked again at PR #4374 which was included in this release but I don't think anything there was related to this new issue (it doesn't touch flags like this as far as a quick glance goes).
The link Simon is referring to I guess is https://mac.r-project.org/openmp/. Search that for "Unfortuantely, some packages don't". I did see that before but I didn't know if Simon was referring to us or not. Seems like he is then.
This seems to be the key then. The script is supposed to make an attempt anyway if libz fails, and it appears to be doing so, so I don't know why Makevars might not be being generated. Note that macos builds worked before on CRAN and we have been using pkg-config for several releases now. The macos toolchain used by CRAN has changed at roughly the same time as data.table 1.13.0 being released.
If that really is the best solution then I suppose we can try autoconf again. Seems a shame to lose the simpler and much lighter pkg-config approach, though. We could live with CRANs status being NOTE due to the size I guess.
I seem to remember it's because macos doesn't accept .so with a dot in the name: data.table.so. Or maybe it was Windows didn't like data.table.dll. And because one OS had an issue with dot, we have to rename everywhere. Or something like that. Or is it even R that doesn't like . in the name of the so/dll. It goes way back, maybe over 10 years. Pending PR #4442 changes the rename from . to _ but I don't remember the root cause of why we can't leave the .so named data.table.so. |
Adding 1mb to PKG sources doesn't sound like a good idea. |
Simon, Matt, |
I had asked Simon why r-release-macos was missing but he didn't answer on that. |
Simon, |
From Professor Ripley: Please do comply with the CRAN policy and not send HTML as you are asked. I do not know why this 'worked' for Simon: it fails for me with both echo "*** Compilation will now be attempted ..." which exits without creating src/Makevars . On my macOS boxes, Telling people to use HomeBrew (which is not normally installed and Also, the Solaris error is one reported and corrected before, and BDR |
Reply from Matt:
I wasn't even aware I send in HTML. I use gmail in the browser and on It would be great if you could raise issues and correspond on GitHub That said, we will continue to strive for the ideal state of no
Thank you for elaborating, that is very helpful.
I think it's better to phrase suggestions in the positive than the
We're aware, thanks. Tracking here: #4638 Best, Matt NB: the R-ext documentation referred to is footnote 36 in section 1.2.1.1. |
Follow up email to Simon cc BDR: Simon, I've tried but I don't follow.
Could you explain what you mean by "blowing away"? Section 1.2.1.1 of R-exts contains:
What do you mean by "honour" -- how do I "honour"?
What I see in your link is: "
Unfortuantely, some packages don't Aside: you have a typo in "Unfortuantely" in that doc. What R rules are you referring to? I have looked at R-exts and I
both what? What 2 things do you mean?
tests of what, how?
huh? Again, copied this to #4640. I've closed that with PR #4662 for now, but it seems data.table is still "blowing away" PKG_. I'd like to fix that if you can assist as above. Currently, I'm lost. Matt |
It's odd that release-macos doesn't appear in the matrix.
oldrel-macos appears to be failing, on first glace, due to not linking to zlib? Our output is pretty good so I'm pleased about that at least.
I've asked Simon Urbanek on email about both.
The text was updated successfully, but these errors were encountered: