Trac #17572 breaks the installation of several R packages.

[charpent]

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.

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

[charpent]

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

2. Limit #17572 to Darwin machines (= current damage control patch) and find some apple-specific way to fix the problem of non-installing R packages.

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

[vbraun]

If the packages trip over missing files then we can just create them at the end of the R spkg-install

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

[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?

[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 : ${...=...}?

[git]

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

​f11ac81

Create Sage-specific Makevars files.

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

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

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

[git]

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

​c793dd1

Quote paths.

[fbissey]

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

[git]

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

​08fc969

Quote paths (again).

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

​08fc969

Quote paths (again).

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

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

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

[git]

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

​011c140

Nitpicking on redirections.

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

​011c140

Nitpicking on redirections.

[fbissey]

Dangling right bracket } still there.

[jdemeyer]

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.

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

[git]

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

​7a2b534

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

[charpent]

Replying to fbissey:

Dangling right bracket } still there.

This zombie should be off for good now...

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

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

[git]

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

​98efc6c

Forgotten echo (my bad...).

[charpent]

Forgot to update status.

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

[charpent]

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

[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

[git]

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

​86141c7

Better place for Sage- and user-specific R stuff.

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

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

[git]

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

​5bc911a

Appease fbissey's fears

[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 ?

[fbissey]

Much better thank you. I think it is good to go now.

[jhpalmieri]

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?

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

[charpent]

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