#16919 closed defect (fixed)
mistake in sage/src/bin/sagebdist, OSX app is always built 32bit
Reported by:  dfriedan  Owned by:  

Priority:  minor  Milestone:  sage6.4 
Component:  build  Keywords:  OSX, sagebdist 
Cc:  iandrus  Merged in:  
Authors:  Daniel Friedan  Reviewers:  Dima Pasechnik, KarlDieter Crisman, Ivan Andrus, Frédéric Chapoton 
Report Upstream:  N/A  Work issues:  
Branch:  6ffb3f6 (Commits, GitHub, GitLab)  Commit:  
Dependencies:  Stopgaps: 
Description (last modified by )
The file sage/src/bin/sagebdist contains a mistake:
'ARCHES' should be 'ARCHS'.
The effect of the mistake is that the OS X application is always built as 32bit ('i386'). This can be seen in the OS X 10.9 app version currently being distributed officially by running the unix command
$
file /Applications/Sage6.3.app/Contents/MacOS/Sage
which returns /Applications/sage6.3.app/Contents/MacOS/Sage: MachO executable i386
This mistake is probably not terribly significant for running sage code, since all the files in sage/local/{bin,lib}/ are 64bit.
(2) There is a second, more obscure mistake in sagebdist. sagebdist uses uname m
to determine the target architecture. Some older 64bit Macintoshes can only boot into a 32bit system. These machines run 64bit Sage perfectly well.
They can make and build 64bit Sage perfectly well. The problem is that
uname m
returns 'i386' on these machines, so sagebdist uses 'i386' as the target architecture.
Attached is modified version of sagebdist 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 sagebdist:
(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 64bit OSX machine that boots into a 32bit system, one would do
$ export SAGE_APP_TARGET_ARCH=x86_64 $ ./sagebdist
(3) I also added some code to append 'app' to the name of the .dmg file produced by sagebdist. 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 sagebdist. One would do
$ export SAGE_APP_GZ=no $ export SAGE_APP_DMG=no $ ./sagebdist
in order to skip the final compression.
Attachments (1)
Change History (29)
Changed 7 years ago by
comment:1 Changed 7 years ago by
Welldone!
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 7 years ago by
 Cc iandrus added
comment:3 Changed 7 years ago by
 Branch set to u/vbraun/osx_bdist_always_32bit
comment:4 followups: ↓ 6 ↓ 7 Changed 7 years ago by
 Commit set to da01d6d9d46caa9389dea1d091249ea7e68c8c54
looks good to me.. somebody wants to try it on their mac?
New commits:
da01d6d  Fix OSX arch detection in sagebdist

comment:5 Changed 7 years ago by
 Status changed from new to needs_review
comment:6 in reply to: ↑ 4 ; followup: ↓ 9 Changed 7 years ago by
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 notdmg. Returning to else
would remove the need to document that one, at any rate.
comment:7 in reply to: ↑ 4 Changed 7 years ago by
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 7 years ago by
 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 ; followup: ↓ 10 Changed 7 years ago by
 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" ]; thensince the message is pretty clear that the only two options are dmg and notdmg. 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 7 years ago by
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" ]; thensince the message is pretty clear that the only two options are dmg and notdmg. 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 7 years ago by
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 7 years ago by
 Branch changed from u/vbraun/osx_bdist_always_32bit to public/ticket/16919
 Commit changed from da01d6d9d46caa9389dea1d091249ea7e68c8c54 to c65de4f5be9488e3040f42bc84eccd44720da5d9
comment:13 Changed 7 years ago by
 Reviewers set to Dmitrii Pasechnik, KarlDieter 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 7 years ago by
I'll add these docs now. If not me then who, if not now then when...
comment:15 Changed 7 years ago by
 Commit changed from c65de4f5be9488e3040f42bc84eccd44720da5d9 to a6896adfca7be8e63f6e2a6c9da7f5791bb3965f
Branch pushed to git repo; I updated commit sha1. New commits:
a6896ad  added docs for envvars

comment:16 followup: ↓ 19 Changed 7 years ago by
 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 7 years ago by
typo "terminal version fo Sage"
comment:18 Changed 7 years ago by
 Commit changed from a6896adfca7be8e63f6e2a6c9da7f5791bb3965f to 913f9d244c9fa8eddc4eeffef542f656cfb7c338
Branch pushed to git repo; I updated commit sha1. New commits:
913f9d2  typo fixed

comment:19 in reply to: ↑ 16 Changed 7 years ago by
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 7 years ago by
 Commit changed from 913f9d244c9fa8eddc4eeffef542f656cfb7c338 to 6ffb3f6d5cb9263468faea4a01855a2ed0890f7f
Branch pushed to git repo; I updated commit sha1. New commits:
6ffb3f6  ticks added

comment:21 Changed 7 years ago by
Thx.
Sounds like everyone has used the new bdist script successfully, Mac and Linux? Anything left?
comment:22 Changed 7 years ago by
 Status changed from needs_review to positive_review
not any more now :)
comment:23 Changed 7 years ago by
 Branch changed from public/ticket/16919 to 6ffb3f6d5cb9263468faea4a01855a2ed0890f7f
 Resolution set to fixed
 Status changed from positive_review to closed
comment:24 followup: ↓ 26 Changed 6 years ago by
 Commit 6ffb3f6d5cb9263468faea4a01855a2ed0890f7f deleted
Why did you remove execute permissions from the sagebdist script? How did anybody manage to run a script that is not executable??
comment:25 Changed 6 years ago by
Followup: #17346
comment:26 in reply to: ↑ 24 Changed 6 years ago by
Why did you remove execute permissions from the sagebdist 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 6 years ago by
Please no discussion on closed tickets, any further information should go to #17346
comment:28 Changed 6 years ago by
 Reviewers changed from Dmitrii Pasechnik, KarlDieter Crisman, Ivan Andrus, Frédéric Chapoton to Dima Pasechnik, KarlDieter Crisman, Ivan Andrus, Frédéric Chapoton
candidate replacement for sage/src/bin/sagebdist