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: | sage-6.8 |
Component: | packages: standard | Keywords: | r-project |
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 sage-env
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 spkg-src.
|
b46fa1a | Restored ORIGINAL non-functional spkg-src as par Nathaann Cohen's request.
|
85de560 | Condition Makevars file creation to existence of the relevant trees.
|
comment:5 follow-up: ↓ 6 Changed 7 years ago by
- Description modified (diff)
comment:6 in reply to: ↑ 5 Changed 7 years ago by
comment:7 follow-up: ↓ 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 Sage-specific 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 follow-up: ↓ 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 sage-devel
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 --warn-long 67.6 src/sage/structure/dynamic_class.py # 2 doctests failed ----------------------------------------------------------------------
However, this doctest passes standalone :
charpent@asus16-ec:/usr/local/sage-6.8$ sage -t --long --warn-long 67.6 src/sage/structure/dynamic_class.py Running doctests with ID 2015-07-03-19-21-20-3619480c. Git branch: mabranche Using --optional=mpir,python2,sage,scons Doctesting 1 file. sage -t --long --warn-long 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.