Opened 11 years ago

Closed 11 years ago

#12568 closed defect (fixed)

make doesn't work properly for targets 'test' and 'micro_release'

Reported by: Itai Bar-Natan Owned by: Georg S. Weber
Priority: major Milestone: sage-5.0
Component: build Keywords: make test micro_release
Cc: Merged in: sage-5.0.beta14
Authors: Itai Bar-Natan, Jean-Pierre Flori Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by Jean-Pierre Flori)

I am using sage 4.8 with GNU make 3.81. When I am in SAGE_ROOT, if I type the commands make test or make micro_release, the shell returns an error. For make micro_release, the it returns the following output:

. local/bin/sage-env && local/bin/sage-micro_release
local/bin/sage-env: 289: Bad substitution
mkdir: cannot create directory `//matplotlib-1.0.1': Permission denied
local/bin/sage-env: 475: Syntax error: "(" unexpected
make: *** [micro_release] Error 2

For make test, it returns a longer output which ends with:

. local/bin/sage-env && sage-maketest
local/bin/sage-env: 289: Bad substitution
mkdir: cannot create directory `//matplotlib-1.0.1': Permission denied
local/bin/sage-env: 475: Syntax error: "(" unexpected
make: *** [test] Error 2

The problem appears to be because make runs each command as a sh script, but that sage-env is a bash script. A similar error message appears if I type . local/bin/sage-env in interactive sh.

Apply only trac_12568-reviewer.patch

Attachments (2)

12568-fix.patch (711 bytes) - added by Itai Bar-Natan 11 years ago.
A patch which fixes the bug
trac_12568-reviewer.patch (949 bytes) - added by Jean-Pierre Flori 11 years ago.
Reviewer patch; rebase on 5.0 series

Download all attachments as: .zip

Change History (18)

Changed 11 years ago by Itai Bar-Natan

Attachment: 12568-fix.patch added

A patch which fixes the bug

comment:1 Changed 11 years ago by Itai Bar-Natan

Status: newneeds_review

I should have done this a while ago since I already have a patch.

comment:2 Changed 11 years ago by Jean-Pierre Flori

Authors: Itai Bar-Natan
Reviewers: Jean-Pierre Flori
Status: needs_reviewpositive_review

comment:3 Changed 11 years ago by Jean-Pierre Flori

Status: positive_reviewneeds_work

I was a little too fast on this. Are we sure that bash will be installed on every system sage supports (and is it a sage dependency) ? I fear that explicitely calling bash might fail even if a compatible shell is installed.

I think a better solution would be modify the sage-env header to something like "#! /usr/bin/env bash". By the way, this file should be executable.

comment:4 Changed 11 years ago by Jean-Pierre Flori

Moreover the sage-env script is in the spkg/bin directory and not in the local/bin one (edit: this is a recent change #11073).

Finally the micro_release target segfault on my machine...

Last edited 11 years ago by Jean-Pierre Flori (previous) (diff)

comment:5 Changed 11 years ago by Jean-Pierre Flori

I propose to let the test target behave like the other ones, i.e. call "sage -t ..." (in the end it calls sage-maketest anyway, but at least sage-env is properly called by the sage script).

For the micro_release one, I'm not sure we can do much better than the solution Itai proposed without calling sage-micro_release from the sage script to properly set SAGE_ROOT (i.e. through an option or something like that).

comment:6 Changed 11 years ago by Jean-Pierre Flori

Not sure why make micro_release now and not before... It does so while trying to strip libpython which is expected: http://trac.sagemath.org/sage_trac/ticket/11743#comment:3

What's more mysterious is why it tries to strip libpython now. It was updated to 2.7 in the 5.0 series but is still readonly.

In fact it did already segfault with 4.8. It's just that because I used system atlas libs with my 4.8 install, it did fail before.

Last edited 11 years ago by Jean-Pierre Flori (previous) (diff)

comment:7 Changed 11 years ago by Jean-Pierre Flori

My bad once more. sage-env is only meant to be sourced (and only once) and should not be executable: #10469

comment:8 Changed 11 years ago by Jean-Pierre Flori

Ok here is a slightly reworked patch making the test target like the other ones.

comment:9 Changed 11 years ago by Jean-Pierre Flori

Description: modified (diff)
Status: needs_workneeds_review

comment:10 Changed 11 years ago by Jeroen Demeyer

  1. Obviously, /usr/bin/env bash should be simply bash.
  1. Then, if you don't use sage-maketest anymore (which is probably a good thing), remove the sage-maketest script.

comment:11 Changed 11 years ago by Jeroen Demeyer

Status: needs_reviewneeds_work

comment:12 Changed 11 years ago by Jean-Pierre Flori

I think the sage-maketest script is used when you call "sage -testall" which is called by some make targets so is still necessary.

Changed 11 years ago by Jean-Pierre Flori

Attachment: trac_12568-reviewer.patch added

Reviewer patch; rebase on 5.0 series

comment:13 Changed 11 years ago by Jean-Pierre Flori

Status: needs_workneeds_review

comment:14 Changed 11 years ago by Jean-Pierre Flori

In fact the testall option is not present in the make targets, but is defined and documented by spkg/bin/sage. Anyway, I don't think here is the place to remove it.

comment:15 Changed 11 years ago by Jeroen Demeyer

Authors: Itai Bar-NatanItai Bar-Natan, Jean-Pierre Flori
Reviewers: Jean-Pierre FloriJeroen Demeyer
Status: needs_reviewpositive_review

make micro_release is indeed broken. It's not surprising, as it changes the python library while python is running:

strip "/tmp/local/sage/local/lib/libpython2.7.so"
bash: line 1:  5817 Segmentation fault      local/bin/sage-micro_release

but that's not relevant for this ticket.

Everything else seems to work, so positive_review.

comment:16 Changed 11 years ago by Jeroen Demeyer

Merged in: sage-5.0.beta14
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.