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:

Status badges

Description (last modified by charpent)

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

  • Description modified (diff)

comment:2 Changed 7 years ago by kcrisman

  • Priority changed from major to blocker

Changing priority, just change back if I misunderstand the severity of the problem.

comment:3 Changed 7 years ago by charpent

  • Branch set to u/charpent/fix__18691_fix_to__17572_fix_to_r_

comment:4 Changed 7 years ago by jdemeyer

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

9a9c4ccOriginal tarball dropped in place.
769fc49Restored spkg-src.
b46fa1aRestored ORIGINAL non-functional spkg-src as par Nathaann Cohen's request.
85de560Condition Makevars file creation to existence of the relevant trees.

comment:5 follow-up: Changed 7 years ago by charpent

  • Description modified (diff)

Jeroen : I'm afraid you mixed this ticket with #18820. I do not understand what the hell 9a9c4cc, 769fc49 and b46fa1a, which belong to #18820, are doing in the present #18835.

Please tell me how to undo this mess...

comment:6 in reply to: ↑ 5 Changed 7 years ago by jdemeyer

Replying to charpent:

Jeroen : I'm afraid you mixed this ticket with #18820.

I didn't do anything, I just made a comment...

Please tell me how to undo this mess...

Just cherry-pick the relevant commit on top of #18691.

comment:7 follow-up: Changed 7 years ago by jhpalmieri

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

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 charpent

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 kcrisman

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 git

  • Commit changed from 85de56082fbf10607b33092490cde9be0d1190b1 to cc32c78a108268d196b008c082d44c9761c6ee9a

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

94275f8Revert "Restored ORIGINAL non-functional spkg-src as par Nathaann Cohen's request."
652a004Revert "Restored spkg-src."
cc32c78Revert "Original tarball dropped in place."

comment:12 Changed 7 years ago by git

  • Commit changed from cc32c78a108268d196b008c082d44c9761c6ee9a to 0bbcb051e26f06836c56c0cff50016d0199426ba

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

0bbcb05Condition creating Sage-specific R files to the existence of their destination trees.

comment:13 Changed 7 years ago by charpent

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

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

comment:14 follow-up: Changed 7 years ago by jdemeyer

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

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 jdemeyer

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 git

  • Commit changed from 0bbcb051e26f06836c56c0cff50016d0199426ba to 18a1262794650e2ffca46bb1e70fd2910a7df645

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

18a1262Condition Makevars file creation to existence of the relevant trees.

comment:18 Changed 7 years ago by charpent

  • 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

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

comment:19 Changed 7 years ago by charpent

  • Authors set to Emmanuel Charpentier

comment:20 Changed 7 years ago by charpent

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 jhpalmieri

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 jhpalmieri

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

  • 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
Note: See TracTickets for help on using tickets.