Opened 7 years ago
Closed 7 years ago
#18835 closed defect (fixed)
Fix #18691 fix to #17572 fix to R.
Reported by:  charpent  Owned by:  

Priority:  minor  Milestone:  sage6.8 
Component:  packages: standard  Keywords:  rproject 
Cc:  Merged in:  
Authors:  Emmanuel Charpentier  Reviewers:  John Palmieri, Jeroen Demeyer 
Report Upstream:  N/A  Work issues:  
Branch:  18a1262 (Commits, GitHub, GitLab)  Commit:  18a1262794650e2ffca46bb1e70fd2910a7df645 
Dependencies:  Stopgaps: 
Description (last modified by )
#18691 is broken on fresh install because the sageenv
script is invoked at a point where $SAGE_LOCAL
or $DOT_SAGE
do not exist yet.
Change History (23)
comment:1 Changed 7 years ago by
 Description modified (diff)
comment:2 Changed 7 years ago by
 Priority changed from major to blocker
comment:3 Changed 7 years ago by
 Branch set to u/charpent/fix__18691_fix_to__17572_fix_to_r_
comment:4 Changed 7 years ago by
 Commit set to 85de56082fbf10607b33092490cde9be0d1190b1
Should this
if [ d "$SAGE_LOCAL" ] ; then R_MAKEVARS_SITE="$SAGE_LOCAL/lib/R/share/Makevars.site" && export R_MAKEVARS_SITE
be
if [ d "$SAGE_LOCAL/lib/R/share" ] ; then export R_MAKEVARS_SITE="$SAGE_LOCAL/lib/R/share/Makevars.site"
New commits:
9a9c4cc  Original tarball dropped in place.

769fc49  Restored spkgsrc.

b46fa1a  Restored ORIGINAL nonfunctional spkgsrc as par Nathaann Cohen's request.

85de560  Condition Makevars file creation to existence of the relevant trees.

comment:5 followup: ↓ 6 Changed 7 years ago by
 Description modified (diff)
comment:6 in reply to: ↑ 5 Changed 7 years ago by
comment:7 followup: ↓ 10 Changed 7 years ago by
 Priority changed from blocker to minor
I lowered the priority. The build is not broken, but a needless error message is (prominently) displayed when building from scratch.
comment:8 Changed 7 years ago by
I know this is not marked as needing review yet, but the proposed change
if [ d "$SAGE_LOCAL" ] ; then
is not good enough: you need to check whether the directory $SAGE_LOCAL/lib/R/share
exists before doing echo foo > "$R_MAKEVARS_SITE"
.
comment:9 Changed 7 years ago by
I managed to hose my branch. I still have to learn how to revert certain commits.
Stay tuned (but not until tomorrow or late tonight). And accept my apologies.
Emmanuel Charpentier
comment:10 in reply to: ↑ 7 Changed 7 years ago by
I lowered the priority. The build is not broken, but a needless error message is (prominently) displayed when building from scratch.
Got it! Thanks for clarifying.
comment:11 Changed 7 years ago by
 Commit changed from 85de56082fbf10607b33092490cde9be0d1190b1 to cc32c78a108268d196b008c082d44c9761c6ee9a
comment:12 Changed 7 years ago by
 Commit changed from cc32c78a108268d196b008c082d44c9761c6ee9a to 0bbcb051e26f06836c56c0cff50016d0199426ba
Branch pushed to git repo; I updated commit sha1. New commits:
0bbcb05  Condition creating Sagespecific R files to the existence of their destination trees.

comment:13 Changed 7 years ago by
 Status changed from new to needs_review
Reading the git
doc was ... instructive ...
This one seems good (builds from make distclean
without error and passes all tests of make ptestlong
==> needs_review
.
comment:14 followup: ↓ 15 Changed 7 years ago by
 Status changed from needs_review to needs_work
I think git
might get confused when merging this...
Please just remove the bad commits instead of reverting them.
comment:15 in reply to: ↑ 14 Changed 7 years ago by
Replying to jdemeyer:
I think
git
might get confused when merging this...Please just remove the bad commits instead of reverting them.
How do you do that ?
comment:16 Changed 7 years ago by
There are many possibilities, I guess
git rebase i
would be the easiest. Then you can edit, squash, remove, reorder commits...
Of course, this qualifies as "rewriting history" which is something you normally should not do for a ticket under review. However, in this case, it is the best solution.
comment:17 Changed 7 years ago by
 Commit changed from 0bbcb051e26f06836c56c0cff50016d0199426ba to 18a1262794650e2ffca46bb1e70fd2910a7df645
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
18a1262  Condition Makevars file creation to existence of the relevant trees.

comment:18 Changed 7 years ago by
 Status changed from needs_work to needs_review
Done. I learned something useful. needs_review
, again.
However, something worries me. I've read again and again and again (in the Developer's guide, on sagedevel
threads, on various ticket brawls...err...discussions) thar "rewriting history" was, a priori a bad idea. I suppose that, in the present case, it is admissible because nobody has yet merged this contribution in anything else. Is this correct ?
Update : this, on top of 6.8beta6, builds with no errors and passes testlong successfully
comment:19 Changed 7 years ago by
comment:20 Changed 7 years ago by
Update : on top of 6.8beta7, this gives one ptestlong failure :
 sage t long warnlong 67.6 src/sage/structure/dynamic_class.py # 2 doctests failed 
However, this doctest passes standalone :
charpent@asus16ec:/usr/local/sage6.8$ sage t long warnlong 67.6 src/sage/structure/dynamic_class.py Running doctests with ID 201507031921203619480c. Git branch: mabranche Using optional=mpir,python2,sage,scons Doctesting 1 file. sage t long warnlong 67.6 src/sage/structure/dynamic_class.py [69 tests, 0.23 s]  All tests passed!  Total time for all tests: 0.3 seconds cpu time: 0.2 seconds cumulative wall time: 0.2 seconds
So I think it's a glitch possibly due to high load.
Still needs_review
comment:21 Changed 7 years ago by
I get the same doctest failure in plain 6.8.beta7. I don't think it's related to this ticket.
comment:22 Changed 7 years ago by
 Reviewers set to John Palmieri, Jeroen Demeyer
 Status changed from needs_review to positive_review
This looks good to me now.
comment:23 Changed 7 years ago by
 Branch changed from u/charpent/fix__18691_fix_to__17572_fix_to_r_ to 18a1262794650e2ffca46bb1e70fd2910a7df645
 Resolution set to fixed
 Status changed from positive_review to closed
Changing priority, just change back if I misunderstand the severity of the problem.