Opened 9 years ago

Closed 9 years ago

#12568 closed defect (fixed)

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

Reported by: itaibn Owned by: GeorgSWeber
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 jpflori)

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 itaibn 9 years ago.
A patch which fixes the bug
trac_12568-reviewer.patch (949 bytes) - added by jpflori 9 years ago.
Reviewer patch; rebase on 5.0 series

Download all attachments as: .zip

Change History (18)

Changed 9 years ago by itaibn

A patch which fixes the bug

comment:1 Changed 9 years ago by itaibn

  • Status changed from new to needs_review

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

comment:2 Changed 9 years ago by jpflori

  • Authors set to Itai Bar-Natan
  • Reviewers set to Jean-Pierre Flori
  • Status changed from needs_review to positive_review

comment:3 Changed 9 years ago by jpflori

  • Status changed from positive_review to needs_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 9 years ago by jpflori

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 9 years ago by jpflori (previous) (diff)

comment:5 Changed 9 years ago by jpflori

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 9 years ago by jpflori

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 9 years ago by jpflori (previous) (diff)

comment:7 Changed 9 years ago by jpflori

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

comment:8 Changed 9 years ago by jpflori

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

comment:9 Changed 9 years ago by jpflori

  • Description modified (diff)
  • Status changed from needs_work to needs_review

comment:10 Changed 9 years ago by jdemeyer

  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 9 years ago by jdemeyer

  • Status changed from needs_review to needs_work

comment:12 Changed 9 years ago by jpflori

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 9 years ago by jpflori

Reviewer patch; rebase on 5.0 series

comment:13 Changed 9 years ago by jpflori

  • Status changed from needs_work to needs_review

comment:14 Changed 9 years ago by jpflori

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 9 years ago by jdemeyer

  • Authors changed from Itai Bar-Natan to Itai Bar-Natan, Jean-Pierre Flori
  • Reviewers changed from Jean-Pierre Flori to Jeroen Demeyer
  • Status changed from needs_review to positive_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 9 years ago by jdemeyer

  • Merged in set to sage-5.0.beta14
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.