Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#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 jhpalmieri

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.)

comment:2 Changed 6 years ago by charpent

  • Branch set to u/charpent/trac__17572_breaks_the_installation_of_several_r_packages_

comment:3 Changed 6 years ago by charpent

  • 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 :

  1. 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,..
  1. Limit #17572 to Darwin machines (= current damage control patch) and find some apple-specific way to fix the problem of non-installing R packages.
  1. 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:

7288cddDamage control : limit #17572 to Darwin OS.
Last edited 6 years ago by charpent (previous) (diff)

comment:4 Changed 6 years ago by vbraun

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 fbissey

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 fbissey

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 fbissey

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 git

  • Commit changed from 7288cdd4e8d2a04f4163dfac1249898d01e50492 to f11ac816ff496a20c3f82594925cbc979d795f80

Branch pushed to git repo; I updated commit sha1. New commits:

f11ac81Create Sage-specific Makevars files.

comment:9 Changed 6 years ago by charpent

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.

Last edited 6 years ago by charpent (previous) (diff)

comment:10 Changed 6 years ago by fbissey

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.

Last edited 6 years ago by fbissey (previous) (diff)

comment:11 Changed 6 years ago by fbissey

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 git

  • Commit changed from f11ac816ff496a20c3f82594925cbc979d795f80 to c793dd194a46f9b318681e3c6947b977f37e2c2f

Branch pushed to git repo; I updated commit sha1. New commits:

c793dd1Quote paths.

comment:13 Changed 6 years ago by fbissey

Also quote on the right hand of the two echo commands {} is not the same thing.

comment:14 Changed 6 years ago by git

  • Commit changed from c793dd194a46f9b318681e3c6947b977f37e2c2f to 08fc969d87a2dffd2167b9ca9a0efe753cbb04b5

Branch pushed to git repo; I updated commit sha1. New commits:

08fc969Quote paths (again).

comment:15 Changed 6 years ago by charpent

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:

08fc969Quote paths (again).

comment:16 Changed 6 years ago by charpent

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: Changed 6 years ago by jdemeyer

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: Changed 6 years ago by fbissey

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 git

  • Commit changed from 08fc969d87a2dffd2167b9ca9a0efe753cbb04b5 to 011c140102203b82a18a89b4019929ddbf30dcff

Branch pushed to git repo; I updated commit sha1. New commits:

011c140Nitpicking on redirections.

comment:20 in reply to: ↑ 17 ; follow-up: Changed 6 years ago by 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.

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:

011c140Nitpicking on redirections.

comment:21 follow-up: Changed 6 years ago by fbissey

Dangling right bracket } still there.

comment:22 in reply to: ↑ 20 Changed 6 years ago by jdemeyer

  • 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: Changed 6 years ago by 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.

comment:24 Changed 6 years ago by git

  • Commit changed from 011c140102203b82a18a89b4019929ddbf30dcff to 7a2b534dbaef5f609e0e31e4a5336a2acb97e192

Branch pushed to git repo; I updated commit sha1. New commits:

7a2b534Clarifying what to do when a R_MAKEVARS* is a non-file.

comment:25 in reply to: ↑ 21 Changed 6 years ago by charpent

Replying to fbissey:

Dangling right bracket } still there.

This zombie should be off for good now...

comment:26 follow-up: Changed 6 years ago by jdemeyer

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 charpent

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 git

  • Commit changed from 7a2b534dbaef5f609e0e31e4a5336a2acb97e192 to 98efc6ce0d3f50f9d90efa02fbf2fdea19e2630f

Branch pushed to git repo; I updated commit sha1. New commits:

98efc6cForgotten echo (my bad...).

comment:29 Changed 6 years ago by charpent

  • Status changed from needs_work to needs_review

Forgot to update status.

comment:30 in reply to: ↑ 23 Changed 6 years ago by fbissey

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 charpent

Does anybody thinks that this patch (#18691) needs further work, or can we move to review ?

Last edited 6 years ago by charpent (previous) (diff)

comment:32 follow-up: Changed 6 years ago by 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
  • provide a place for future R-in-Sage specific stuff

comment:33 Changed 6 years ago by git

  • Commit changed from 98efc6ce0d3f50f9d90efa02fbf2fdea19e2630f to 86141c7dea8b16ab5af5aa54d02c4524cb396798

Branch pushed to git repo; I updated commit sha1. New commits:

86141c7Better place for Sage- and user-specific R stuff.

comment:34 in reply to: ↑ 32 Changed 6 years ago by charpent

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: Changed 6 years ago by 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).

comment:36 Changed 6 years ago by git

  • Commit changed from 86141c7dea8b16ab5af5aa54d02c4524cb396798 to 5bc911a30a43a3a4ac1579b3057f8b71d74bf6e7

Branch pushed to git repo; I updated commit sha1. New commits:

5bc911aAppease fbissey's fears

comment:37 in reply to: ↑ 35 Changed 6 years ago by charpent

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 fbissey

  • Authors changed from charpent to Emmanuel Charpentier
  • 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 vbraun

  • 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 jhpalmieri

  • 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 fbissey

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 charpent

This ticket is closed ==> created #18835. Followup there.

Note: See TracTickets for help on using tickets.