#18691 closed defect (fixed)
Trac #17572 breaks the installation of several R packages.
Reported by: | charpent | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-6.8 |
Component: | packages: standard | Keywords: | r-project |
Cc: | hsnyder, fbissey, jdemeyer, vbraun, jpswanson, kcrisman | Merged in: | |
Authors: | Emmanuel Charpentier | Reviewers: | François Bissey, Jeroen Demeyer, Volker Braun |
Report Upstream: | N/A | Work issues: | |
Branch: | 5bc911a (Commits) | Commit: | |
Dependencies: | Stopgaps: |
Description
Since 6.8.beta1, some packages (among them "git2r" and, therefore, "devtools" that depends on it, no longer install.
Thses package seem to depend on Makevars ; "git2r" definitely depends on Makevars.
git bisect points to commit bb5c9f3a2d9a6edcffe787716faa91fa5fd4aed6, which is in #17572, as the source of the problem. Indeed : bb5c9f3a... is entitled "Do not let R use global or user-local Makevars".
I am afraid that this solution cannot be accepted : it breaks existing R packages, finctional in all "normal" R installations. Furthermore, it breaks a mechanism documented upstream as legitimate for future packages.
Change History (42)
comment:1 Changed 6 years ago by
comment:2 Changed 6 years ago by
- Branch set to u/charpent/trac__17572_breaks_the_installation_of_several_r_packages_
comment:3 Changed 6 years ago by
- Commit set to 7288cdd4e8d2a04f4163dfac1249898d01e50492
- Status changed from new to needs_review
The proposed patch is a "damage control" patch limiting the effect of #17572 to Darwin OS. Edit : the resultant Sage passes ptestlong
with "All tests passed !"
.
Rationale : the problem (mis-)fixed by #17572 seems to be limited to machines running clang. It seems, from reading pages pointed to in the discussion of this patch, that this happens on Mac OS X due to slightly suboptimal apple development software.
As already seen, this "solution" breaks the installation of several R packages, which explicitly check for systemwide Makevars
or user-specific Makevars.user
set by way of R_HOME and R_PROFILE environment variables.
Our current solution is to unset these variables and export the variables R_MAKEVARS_SITE
and R_MAKEVARS_SITE
, pointing to convenient Sage-specific versions of similarly named files. As far as I can tell, we do not have such files, which breaks the build of some R packages.
I do not have a Mac, and I do not use clang. Therefore, I'm ill-qualified to propose a solution. A stopgap could be to create such empty files at R installation, and keep the patch as it curently is.
However, I do not understand why clang breaks when the relevant variables are not defined, and why gcc breaks when they are defined as pointing to nonexistent files.
So we have a few courses of action :
- keep #17572 as it is now, and add the creation of empty Makevars.site and Makevars.user somewhere at the end of the R installation script. Not that easy,..
- Limit #17572 to Darwin machines (= current damage control patch) and find some apple-specific way to fix the problem of non-installing R packages.
- Revert #17572 entierely and find some other way to induce clang that it can in fact run without these environment variables.
In any case, fixing this one needs the expertise of someone having access to a clang-plagued Mac OS X machine. I can't test a solution of this combination.
I put this "damage control" patch as needs_review
. Feel free to pull it back to needs_work
if you have some proposal to implement solutions 1 or 3. Or something else entirely...
New commits:
7288cdd | Damage control : limit #17572 to Darwin OS.
|
comment:4 Changed 6 years ago by
If the packages trip over missing files then we can just create them at the end of the R spkg-install
comment:5 Changed 6 years ago by
I think we need to be more careful there. #17572 is caused by interference with another R
install on the system. Whether there is breakage or not that is a problem. You don't usually see it on non-apple because your system variable and sage variables are likely to be similar. But it also means that you will share the installed package between your R
install outside of sage and the one installed in sage.
That is not sane.
I think we would be better served by defining R_HOME
in the sage environment rather than bluntly unseting it. I unfortunately do not keep crap in /usr/local
on my apple machine (I know better, and so should a lot of people) so meaningful testing is hard for me.
comment:6 Changed 6 years ago by
Looking at it more closely #17572 may have erred when defining R_MAKEVARS_USER
. R_MAKEVARS_SITE
is tested for existence before being used in local/lib/R/bin/config
but the same file only test R_MAKEVARS_USER
to be defined and will assume that if it is defined it must exist. I think upstream should check for its existence too.
I am curious Emmanuel, would replacing the line R_MAKEVARS_USER="$SAGE_LOCAL/lib/R/share/Makevars.user" && export R_MAKEVARS_USER
by unset R_MAKEVARS_USER
be enough to solve your difficulties?
comment:7 Changed 6 years ago by
In fact both R_MAKEVARS_*
variables should have been unset. If R_MAKEVARS_SITE
is unset it will be set to a default value which is sane, or at least should
: ${R_MAKEVARS_SITE="${R_HOME}/etc${R_ARCH}/Makevars.site"}
Just to double check is it a correct syntax above, or is it a bug upstream and it should be ${...:=...}
instead of : ${...=...}
?
comment:8 Changed 6 years ago by
- Commit changed from 7288cdd4e8d2a04f4163dfac1249898d01e50492 to f11ac816ff496a20c3f82594925cbc979d795f80
Branch pushed to git repo; I updated commit sha1. New commits:
f11ac81 | Create Sage-specific Makevars files.
|
comment:9 Changed 6 years ago by
Second proposal :
- Do not make Darwin a special case.
- Create Sage-specific Makevars files if necessary.
The site-wide Makevars file goes into the $SAGE_LOCAL
tree, the user-specific file in his/her/its $DOT_SAGE
tree.
This is done in the sage-env
script (in order to check this at each and every Sage use, and avoid damaging a file created only at installation time.
This works for me (C)(R)(TM)... This patch needs attention of Apple users ! ==> needs_review
.
comment:10 Changed 6 years ago by
As is it should work but the creation of the file $R_MAKEVARS_SITE
is unnecessary as R
checks that it exist (from SAGE_LOCAL/lib/R/bin/config
)
if test "${site}" = "yes"; then : ${R_MAKEVARS_SITE="${R_HOME}/etc${R_ARCH}/Makevars.site"} if test -f "${R_MAKEVARS_SITE}"; then makefiles="${makefiles} -f ${R_MAKEVARS_SITE}" fi fi
The existence of $R_MAKEVARS_USER
however is not tested, only that the variable is not empty, so populating it when we define it is a really good idea.
On a separate remark on style you should quote both R_MAKEVARS_SITE
and R_MAKEVARS_USER
as they are paths that can potentially contain spaces.
comment:11 Changed 6 years ago by
Oh and I completely approve the new location of R_MAKEVARS_USER
, this is really where it belongs and make it useful for a system wide install of sage
. The previous location not so much.
comment:12 Changed 6 years ago by
- Commit changed from f11ac816ff496a20c3f82594925cbc979d795f80 to c793dd194a46f9b318681e3c6947b977f37e2c2f
Branch pushed to git repo; I updated commit sha1. New commits:
c793dd1 | Quote paths.
|
comment:13 Changed 6 years ago by
Also quote on the right hand of the two echo
commands {}
is not the same thing.
comment:14 Changed 6 years ago by
- Commit changed from c793dd194a46f9b318681e3c6947b977f37e2c2f to 08fc969d87a2dffd2167b9ca9a0efe753cbb04b5
Branch pushed to git repo; I updated commit sha1. New commits:
08fc969 | Quote paths (again).
|
comment:15 Changed 6 years ago by
François,
Good catch for the need to quote paths. I have been Windows-weaned too long ago to really think about these follies...
Skipping the site variable : I'm not so sure : the R statrup code is a bit of a fisherman's net, and I am not sure that an R package (for example) does not use it cavalierly in a situation where it does not exists. So my paranoid version takes care of that.
Would you care to formally review this ?
New commits:
08fc969 | Quote paths (again).
|
comment:16 Changed 6 years ago by
Further comment : if The Apple cimpiler (clang) really needs permanent supplementary flags to be useful with R, a patch to this effect should be submitted upstream.
If it happens only with Sage, another ticket, clearly marked as Apple-specific, should be opened to this end.
comment:17 follow-up: ↓ 20 Changed 6 years ago by
Can you use -f
instead of -a
(I actually had to look up what -a
does!)
And if the file does not exist, using >>
(for append) looks strange, better use >
.
comment:18 follow-up: ↓ 23 Changed 6 years ago by
Formally yes, that's what I am doing now :) R_MAKEVARS_USER="$DOT_SAGE/SagesRMakevars.user"}
You seem to have a lose end bracket that should go.
As to regards to clang
currently we cannot compile sage with it at this stage (and we still need a fortran compiler to complicate things). The problem with clang
which launched all those problems is that settings for another install outside of sage (probably done with clang
as a matter of fact) got pulled into sage
during while R
was being built. R
kind of has two building phases. In a first step R
proper is being built. Then standard package are being built and that's when the mix up occurred.
I also agree with Jeroen's comments.
comment:19 Changed 6 years ago by
- Commit changed from 08fc969d87a2dffd2167b9ca9a0efe753cbb04b5 to 011c140102203b82a18a89b4019929ddbf30dcff
Branch pushed to git repo; I updated commit sha1. New commits:
011c140 | Nitpicking on redirections.
|
comment:20 in reply to: ↑ 17 ; follow-up: ↓ 22 Changed 6 years ago by
Replying to jdemeyer:
Can you use
-f
instead of-a
(I actually had to look up what-a
does!)
-a
is intentional : either Makevar file could be a symlink to a machine- or project-specific Makevar file, and Sage should respect this if it has been so positioned by the user.
And if the file does not exist, using
>>
(for append) looks strange, better use>
.
That's nitpicking. But so be it (commit under way)...
New commits:
011c140 | Nitpicking on redirections.
|
comment:21 follow-up: ↓ 25 Changed 6 years ago by
Dangling right bracket }
still there.
comment:22 in reply to: ↑ 20 Changed 6 years ago by
- Status changed from needs_review to needs_work
Replying to charpent:
Replying to jdemeyer:
Can you use
-f
instead of-a
(I actually had to look up what-a
does!)
-a
is intentional : either Makevar file could be a symlink to a machine- or project-specific Makevar file, and Sage should respect this if it has been so positioned by the user.
If you think that -f
will not work with symlinks, you are mistaken. Being a file and being a symlink are independent. In particular, a symlink to an existing file is considered to be both a file and a symlink.
You really want to use -f
here, since -a
will match directories (and other weird things like devices,...) and you don't want that. Moreover, -f
is more portable than -a
.
comment:23 in reply to: ↑ 18 ; follow-up: ↓ 30 Changed 6 years ago by
Replying to fbissey:
As to regards to
clang
currently we cannot compile sage with it at this stage (and we still need a fortran compiler to complicate things).
Fortran significantly complicates things for R: it needs to link together C and Fortran code and that only works if the C and Fortran compilers are sufficiently compatible.
comment:24 Changed 6 years ago by
- Commit changed from 011c140102203b82a18a89b4019929ddbf30dcff to 7a2b534dbaef5f609e0e31e4a5336a2acb97e192
Branch pushed to git repo; I updated commit sha1. New commits:
7a2b534 | Clarifying what to do when a R_MAKEVARS* is a non-file.
|
comment:25 in reply to: ↑ 21 Changed 6 years ago by
Replying to fbissey:
Dangling right bracket
}
still there.
This zombie should be off for good now...
comment:26 follow-up: ↓ 27 Changed 6 years ago by
Did you forget an echo
?
>&2 "Warning: $R_MAKEVARS_SITE exists and is not a file : trouble ahead..."
(personally, I would just drop the -a
test, it's over-complicating things for no clear advantage)
comment:27 in reply to: ↑ 26 Changed 6 years ago by
Replying to jdemeyer:
Did you forget an
echo
?
Indeed... patch on the way.
>&2 "Warning: $R_MAKEVARS_SITE exists and is not a file : trouble ahead..."(personally, I would just drop the
-a
test, it's over-complicating things for no clear advantage)
Basic surgeon's conditioning : if you don't see it, don't cut it ! We want to avoid overwriting something in the way as long as we don't know what is it and how it landed here...
comment:28 Changed 6 years ago by
- Commit changed from 7a2b534dbaef5f609e0e31e4a5336a2acb97e192 to 98efc6ce0d3f50f9d90efa02fbf2fdea19e2630f
Branch pushed to git repo; I updated commit sha1. New commits:
98efc6c | Forgotten echo (my bad...).
|
comment:29 Changed 6 years ago by
- Status changed from needs_work to needs_review
Forgot to update status.
comment:30 in reply to: ↑ 23 Changed 6 years ago by
Replying to jdemeyer:
Replying to fbissey:
As to regards to
clang
currently we cannot compile sage with it at this stage (and we still need a fortran compiler to complicate things).Fortran significantly complicates things for R: it needs to link together C and Fortran code and that only works if the C and Fortran compilers are sufficiently compatible.
It is worse than that. R
's configure script identifies clearly how to mix C and fortran, i.e. it looks at your fortran compiler and identifies the necessary mangling. At least one of the standard R
packages that are built by default just assume gfortran
style mangling for their calls to blas/lapack functions. So whatever fortran compiler you use to build R
you have to make it behave like gfortran
.
comment:31 Changed 6 years ago by
Does anybody thinks that this patch (#18691) needs further work, or can we move to review ?
comment:32 follow-up: ↓ 34 Changed 6 years ago by
I'd rename $DOT_SAGE/SagesRMakevars.user
to $DOT_SAGE/R/Makevars.user
; rest looks ok
- no need to put Sage into the filename, its in dotsage already
- provide a place for future R-in-Sage specific stuff
comment:33 Changed 6 years ago by
- Commit changed from 98efc6ce0d3f50f9d90efa02fbf2fdea19e2630f to 86141c7dea8b16ab5af5aa54d02c4524cb396798
Branch pushed to git repo; I updated commit sha1. New commits:
86141c7 | Better place for Sage- and user-specific R stuff.
|
comment:34 in reply to: ↑ 32 Changed 6 years ago by
Replying to vbraun:
I'd rename
$DOT_SAGE/SagesRMakevars.user
to$DOT_SAGE/R/Makevars.user
; rest looks ok
- no need to put Sage into the filename, its in dotsage already
Indeed. Good point.
- provide a place for future R-in-Sage specific stuff
Nice idea ... provided that this doesn't generate a drift from "standalone R". Apple-specific difficulties with Fortran are bad enough, we don't want to find ourselves maintaining a semi-clandestine fork...
Still needs review...
comment:35 follow-up: ↓ 37 Changed 6 years ago by
While probably not necessary, I would feel better if you were using mkdir -p
as $DOT_SAGE
is no guaranteed to exist (but will probably be created by the time you get there).
comment:36 Changed 6 years ago by
- Commit changed from 86141c7dea8b16ab5af5aa54d02c4524cb396798 to 5bc911a30a43a3a4ac1579b3057f8b71d74bf6e7
Branch pushed to git repo; I updated commit sha1. New commits:
5bc911a | Appease fbissey's fears
|
comment:37 in reply to: ↑ 35 Changed 6 years ago by
Replying to fbissey:
While probably not necessary, I would feel better if you were using
mkdir -p
as$DOT_SAGE
is no guaranteed to exist (but will probably be created by the time you get there).
Done. Feeling better ?
comment:38 Changed 6 years ago by
- Reviewers set to François Bissey, Jeroen Demeyer, Volker Braun
- Status changed from needs_review to positive_review
Much better thank you. I think it is good to go now.
comment:39 Changed 6 years ago by
- Branch changed from u/charpent/trac__17572_breaks_the_installation_of_several_r_packages_ to 5bc911a30a43a3a4ac1579b3057f8b71d74bf6e7
- Resolution set to fixed
- Status changed from positive_review to closed
comment:40 Changed 6 years ago by
- Commit 5bc911a30a43a3a4ac1579b3057f8b71d74bf6e7 deleted
Starting with a clean install of Sage, this leads to an error message at the start of building (as soon as sage-env
is run):
$ make mkdir -p logs cd build && \ "../build/pipestatus" \ "./install all 2>&1" \ "tee -a ../logs/install.log" /Users/palmieri/sage-6.8.beta6/src/bin/sage-env: line 434: /Users/palmieri/sage-6.8.beta6/local/lib/R/share/Makevars.site: No such file or directory
This is because the directory $SAGE_LOCAL/lib/R/share/
does not yet exist, so the redirection
echo "## Empty site-wide Makevars file for Sage's R" > "$R_MAKEVARS_SITE"
fails. The build continues anyway, and I guess the file should be created when Sage is next run, but would it be better to create this file when installing R
(that is, in build/pkgs/r/spkg-install
)? Or at least to check if the directory exists before trying to create the file, to avoid a needless error message?
comment:41 Changed 6 years ago by
I should have flat refused the definition of R_MAKEVARS_SITE
the default is just fine, we should have unset any definition from the environment so it doesn't pollute the sage one. I have seen the message too, not hurting the build but not a nice artefact.
comment:42 Changed 6 years ago by
This ticket is closed ==> created #18835. Followup there.
Could we set
R_MAKEVARS_USER
to something like"$DOT_SAGE/R/Makevars.user"
? Would that help? (We already do this kind of thing with IPython and matplotlib.)