Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#16919 closed defect (fixed)

mistake in sage/src/bin/sage-bdist, OSX app is always built 32-bit

Reported by: dfriedan Owned by:
Priority: minor Milestone: sage-6.4
Component: build Keywords: OSX, sage-bdist
Cc: iandrus Merged in:
Authors: Daniel Friedan Reviewers: Dima Pasechnik, Karl-Dieter Crisman, Ivan Andrus, Frédéric Chapoton
Report Upstream: N/A Work issues:
Branch: 6ffb3f6 (Commits) Commit:
Dependencies: Stopgaps:

Description (last modified by dimpase)

The file sage/src/bin/sage-bdist contains a mistake:

'ARCHES' should be 'ARCHS'.

The effect of the mistake is that the OS X application is always built as 32-bit ('i386'). This can be seen in the OS X 10.9 app version currently being distributed officially by running the unix command

$ file /Applications/Sage-6.3.app/Contents/MacOS/Sage

which returns /Applications/sage-6.3.app/Contents/MacOS/Sage: Mach-O executable i386

This mistake is probably not terribly significant for running sage code, since all the files in sage/local/{bin,lib}/ are 64-bit.

(2) There is a second, more obscure mistake in sage-bdist. sage-bdist uses uname -m to determine the target architecture. Some older 64-bit Macintoshes can only boot into a 32-bit system. These machines run 64-bit Sage perfectly well. They can make and build 64-bit Sage perfectly well. The problem is that uname -m returns 'i386' on these machines, so sage-bdist uses 'i386' as the target architecture.

Attached is modified version of sage-bdist which corrects both mistakes. See below for a description of my modifications.

I am NOT going to submit this via 'git trac'. As a neophyte, I don't want the responsibility of modifying the build script.

changes in the attached version of sage-bdist:

(1) 'ARCHES' -> 'ARCHS'

(2) I added an environment variable SAGE_APP_TARGET_ARCH to override uname -m. If one is building Sage on an older 64-bit OSX machine that boots into a 32-bit system, one would do

$ export SAGE_APP_TARGET_ARCH=x86_64 $ ./sage-bdist

(3) I also added some code to append '-app' to the name of the .dmg file produced by sage-bdist. At present, this has to be done by hand.

(4) I also added an environment variable (SAGE_APP_GZ=no) to prevent the final compression stage, to save time during debugging sage-bdist. One would do

$ export SAGE_APP_GZ=no $ export SAGE_APP_DMG=no $ ./sage-bdist

in order to skip the final compression.

Attachments (1)

sage-bdist-TARGET_ARCH-app (4.4 KB) - added by dfriedan 5 years ago.
candidate replacement for sage/src/bin/sage-bdist

Download all attachments as: .zip

Change History (29)

Changed 5 years ago by dfriedan

candidate replacement for sage/src/bin/sage-bdist

comment:1 Changed 5 years ago by dimpase

Well-done!

One still has to convert your attachment into a git branch and put it on trac. :-) This does not add any responsibility, as it will still be reviewed etc, before making it into Sage proper.

comment:2 Changed 5 years ago by kcrisman

  • Cc iandrus added

comment:3 Changed 5 years ago by vbraun

  • Branch set to u/vbraun/osx_bdist_always_32bit

comment:4 follow-ups: Changed 5 years ago by vbraun

  • Commit set to da01d6d9d46caa9389dea1d091249ea7e68c8c54

looks good to me.. somebody wants to try it on their mac?


New commits:

da01d6dFix OSX arch detection in sage-bdist

comment:5 Changed 5 years ago by vbraun

  • Authors set to Daniel Friedan
  • Status changed from new to needs_review

comment:6 in reply to: ↑ 4 ; follow-up: Changed 5 years ago by kcrisman

looks good to me.. somebody wants to try it on their mac?

Seems likely to me as well, though I'd prefer to have Ivan also check out the part that Xcode will see first. I definitely won't have time to actually try a bdist with this in the next week.

Also, there are now TWO new undocumented variables. I think that it makes sense to have an example of the appropriate new one introduced here in the installation guide or wherever such variables are documented. Secondly, I fail to see the point of

elif [ "$SAGE_APP_GZ" != "no" ]; then

since the message is pretty clear that the only two options are dmg and not-dmg. Returning to else would remove the need to document that one, at any rate.

Last edited 5 years ago by kcrisman (previous) (diff)

comment:7 in reply to: ↑ 4 Changed 5 years ago by dimpase

Replying to vbraun:

looks good to me.. somebody wants to try it on their mac?

I can try this tonight, assuming my laptop still boots in OSX...

comment:8 Changed 5 years ago by iandrus

  • Status changed from needs_review to positive_review

I tried it OMM (10.9) and everything seems to be in order.

comment:9 in reply to: ↑ 6 ; follow-up: Changed 5 years ago by kcrisman

  • Status changed from positive_review to needs_work

Also, there are now TWO new undocumented variables. I think that it makes sense to have an example of the appropriate new one introduced here in the installation guide or wherever such variables are documented. Secondly, I fail to see the point of

elif [ "$SAGE_APP_GZ" != "no" ]; then

since the message is pretty clear that the only two options are dmg and not-dmg. Returning to else would remove the need to document that one, at any rate.

I haven't heard a response to this, though.

comment:10 in reply to: ↑ 9 Changed 5 years ago by kcrisman

Replying to kcrisman:

Also, there are now TWO new undocumented variables. I think that it makes sense to have an example of the appropriate new one introduced here in the installation guide or wherever such variables are documented. Secondly, I fail to see the point of

elif [ "$SAGE_APP_GZ" != "no" ]; then

since the message is pretty clear that the only two options are dmg and not-dmg. Returning to else would remove the need to document that one, at any rate.

I haven't heard a response to this, though.

Sorry, I didn't see the full description, because I just was thinking about the original discussion. Both of these now are clear to me but still I think they need to be documented.

comment:11 Changed 5 years ago by kcrisman

I can't test whether changing SAGE_APP_TARGET_ARCH will change anything but things built okay on 10.7 and of course the other changes are very salutary. I will remark that I then got a segmentation fault while trying to start it, but I did bdist from a bdisted binary so that is probably the problem.

comment:12 Changed 5 years ago by chapoton

  • Branch changed from u/vbraun/osx_bdist_always_32bit to public/ticket/16919
  • Commit changed from da01d6d9d46caa9389dea1d091249ea7e68c8c54 to c65de4f5be9488e3040f42bc84eccd44720da5d9

I have taken the opportunity to correct two typos, and use $UNAME consistently.


New commits:

f246980Merge branch 'u/vbraun/osx_bdist_always_32bit' into 6.4.b6
c65de4ftrac #16919 two typos and usage of UNAME

comment:13 Changed 5 years ago by kcrisman

  • Reviewers set to Dmitrii Pasechnik, Karl-Dieter Crisman, Ivan Andrus, Frédéric Chapoton

Thanks. I still say "needs work" due to missing documentation of these new environment variables - I think it's the installation guide where these ordinarily appear?

comment:14 Changed 5 years ago by dimpase

I'll add these docs now. If not me then who, if not now then when...

comment:15 Changed 5 years ago by git

  • Commit changed from c65de4f5be9488e3040f42bc84eccd44720da5d9 to a6896adfca7be8e63f6e2a6c9da7f5791bb3965f

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

a6896adadded docs for envvars

comment:16 follow-up: Changed 5 years ago by dimpase

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

done. apparently SAGE_APP_BUNDLE existed before, but wasn't documented either.

comment:17 Changed 5 years ago by chapoton

typo "terminal version fo Sage"

comment:18 Changed 5 years ago by git

  • Commit changed from a6896adfca7be8e63f6e2a6c9da7f5791bb3965f to 913f9d244c9fa8eddc4eeffef542f656cfb7c338

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

913f9d2typo fixed

comment:19 in reply to: ↑ 16 Changed 5 years ago by kcrisman

done. apparently SAGE_APP_BUNDLE existed before, but wasn't documented either.

Quite. They are also in the script itself help messages, but that is not the greatest - thanks very much!

If you look at the built doc the outcome one might expect happens from

`i386` and `x86_64`

instead of

``i386`` and ``x86_64``

so adding those characters would be good.

comment:20 Changed 5 years ago by git

  • Commit changed from 913f9d244c9fa8eddc4eeffef542f656cfb7c338 to 6ffb3f6d5cb9263468faea4a01855a2ed0890f7f

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

6ffb3f6ticks added

comment:21 Changed 5 years ago by kcrisman

Thx.

Sounds like everyone has used the new bdist script successfully, Mac and Linux? Anything left?

comment:22 Changed 5 years ago by dimpase

  • Status changed from needs_review to positive_review

not any more now :)

comment:23 Changed 5 years ago by vbraun

  • Branch changed from public/ticket/16919 to 6ffb3f6d5cb9263468faea4a01855a2ed0890f7f
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:24 follow-up: Changed 5 years ago by vbraun

  • Commit 6ffb3f6d5cb9263468faea4a01855a2ed0890f7f deleted

Why did you remove execute permissions from the sage-bdist script? How did anybody manage to run a script that is not executable??

comment:25 Changed 5 years ago by vbraun

Followup: #17346

comment:26 in reply to: ↑ 24 Changed 5 years ago by kcrisman

Why did you remove execute permissions from the sage-bdist script? How did anybody manage to run a script that is not executable??

I dunno - I just bdisted three times in the past few days with no problem.

comment:27 Changed 5 years ago by vbraun

Please no discussion on closed tickets, any further information should go to #17346

comment:28 Changed 5 years ago by kcrisman

  • Reviewers changed from Dmitrii Pasechnik, Karl-Dieter Crisman, Ivan Andrus, Frédéric Chapoton to Dima Pasechnik, Karl-Dieter Crisman, Ivan Andrus, Frédéric Chapoton
Note: See TracTickets for help on using tickets.