Opened 11 years ago

Closed 11 years ago

Last modified 9 years ago

#10820 closed defect (fixed)

tachyon-0.98beta.p11 doesn't build on ARM

Reported by: Snark Owned by: drkirkby
Priority: major Milestone: sage-4.7
Component: porting Keywords:
Cc: Merged in: sage-4.7.alpha2
Authors: Julien Puydt Reviewers: David Kirkby
Report Upstream: Reported upstream. Developers acknowledge bug. Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by drkirkby)

I discussed ideas for a correct fix with upstream, but in the mean time, it's pretty easy to get around the problem with the attached patch.

All patches applied to this ticket are for review purposes only, and do not need to be merged to Sage - all changes are in the package.

The updated package can be found at:

http://boxen.math.washington.edu/home/kirkby/patches/tachyon-0.98.9.p3.spkg

Attachments (2)

spkg-install.patch (415 bytes) - added by Snark 11 years ago.
Patch to make tachyon work on ARM (to apply to spkg-install)
tachyon-0.98.9_for_ARM (1.1 KB) - added by Snark 11 years ago.
Mercurial patch

Download all attachments as: .zip

Change History (13)

Changed 11 years ago by Snark

Patch to make tachyon work on ARM (to apply to spkg-install)

comment:1 Changed 11 years ago by drkirkby

I can't test this, as I don't have an ARM processor, but the necessary changes should be fairly safe, as it does very little and only on one processor.

But you need to do more to get this into Sage.

sed 's/-m32//g' Make-arch > Make-arch.tmp
mv Make-arch.tmp Make-arch
  • Edit SPKG.txt, to document the change you have made. Document them under tachyon-0.98beta.p12
  • Create one Mercurial patch of all your changes.
  • Create a new package, tachyon-0.98beta.p12.spkg and post a link to the URL of that new package.
  • Change the ticket from "new" to "needs_review"

One you do all these, I will be happy to give it a positive review, even though I'm unable to actually check on an ARM processor.

Dave

comment:2 Changed 11 years ago by Snark

Ok, here is a new patch, which modifies pkg-install as you wanted and describes succinctly the changes in SPKG.txt.

Is making a new package available really needed?

comment:3 Changed 11 years ago by Snark

  • Status changed from new to needs_review

comment:4 Changed 11 years ago by drkirkby

  • Report Upstream changed from N/A to Reported upstream. Developers acknowledge bug.
  • Reviewers set to David Kirkby
  • Status changed from needs_review to needs_work

I can't tell exactly what you have done, as I have no .spkg to look at, but it looks to me as though you have copied Make-arch unmodified to the patches directory, then used 'sed' on that to change it. If I'm correct, then its a waste of disk space. The two sensible options are either to:

  • Create a patch, which can be installed with GNU patch.
  • Use 'sed' to edit the Make-arch, overwriting the source code when spkg-install is run.

The patches attached to the ticket must be in Mercurial format, and there must be a new .spkg file with all the changes committed. That way there's an exact log of all the changes in the package. If you don't know how to do that, I don't mind doing it for you this time, but in the long term you will need to find out how to do it.

Also, can you fill in the author form with your real name.

comment:5 Changed 11 years ago by Snark

  • Authors set to Julien Puydt
  • Status changed from needs_work to needs_review

What I gave is the patch that modifies the spkg's content, and it can be applied with GNU patch ; I have no clue what a "mercurial patch" is... as far as I know, all source control system use the same two patch formats, and I used the most human-readable, dubbed "unified" -- I obtained it by running "diff -urN tachyon-0.98.9.p2/ tachyon-0.98.9.p3/" (my diff being GNU's, version 3.0).

It tells exactly what I have done :

  • I added a comment in SPKG.txt (three lines) ;
  • I modified spkg-install so that for ARM it removes "-m32" from Make-arch (three lines).

The current spkg-install already does things to Make-arch -- and one of the first things it does it to just cp it : "cp ../../patches/Make-arch .", line 16. What I do is that later on, I make it realize that it should have modified it ; so I just take it again, but copy it using sed to remove the offending switch : no waste of space with respect to what the current spkg does.

If I give you the resulting spkg, you'll just have everything : the full sources, the previous changes and -- buried in the middle of that -- my own changes. Six lines lost in the of 2.4M of files, where you'll not review what I have done, but what the previous packager did!

I only want you to review the six lines I'm really responsible of : the rest is not my work. And you'll notice that out of six lines, three are for documenting my changes...

To tell more, here is how to apply my patch -- which is quite long because I have no clue how to really use an spkg :

$ cp /path/to/tachyon-0.98.9.p2.spkg ./tachyon-0.98.9.p2.tar.bz2
$ tar xaf tachyon-0.98.9.p2.tar.bz2
$ mv tachyon-0.98.9.p2 tachyon-0.98.9.p3
$ cd tachyon-0.98.9.p3
$ patch -p1 < /path/to/tachyon-0.98.9_for_ARM
$ cd ..
$ tar cjf tachyon-0.98.9.p3.tar.bz2 tachyon-0.98.9.p3
$ mv tachyon-0.98.9.p3.tar.bz2 tachyon-0.98.9.p3.spkg

The way you should read the patch is :

diff -ur tachyon-0.98.9.p2/spkg-install tachyon-0.98.9.p3/spkg-install

this tells I turned tachyon-0.98.9.p2/spkg-install into tachyon-0.98.9.p3/spkg-install

--- tachyon-0.98.9.p2/spkg-install      2011-01-13 01:43:50.000000000 +0100
+++ tachyon-0.98.9.p3/spkg-install      2011-03-12 19:08:11.124659955 +0100

this says that what is prefixed "-" will be what is removed from tachyon-0.98.9.p2/spkg-install, and what is prefixed "+" will be what is added to tachyon-0.98.9.p3/spkg-install

@@ -63,6 +63,9 @@

this says that the following chunk applies at line 63, and that something 6 lines long will become 9 lines long.

	             $MAKE linux-64-thr;;
	        ppc)
	             $MAKE linux-ppc;;
	+        armv7l)
	+            sed "s/-m32//g" ../../patches/Make-arch > Make-arch
	+            $MAKE linux-thr;;
	        *) # e.g. ppc64
	             echo "Sorry, your platform isn't supported by Tachyon and/or Sage. Exiting..."
	             exit 1

This is the code from the current file, with the "+" being the lines to add : there I'm just adding to the arch test which was falling through to an error a new case, just for "armv7l", which just modifies the Make-arch files by removing -m32... a Make-arch file which I do not really create myself, as you can see : I just make do with what is already available. This is all I modify in pkg-install.

Now there is another file I modify :

diff -ur tachyon-0.98.9.p2/SPKG.txt tachyon-0.98.9.p3/SPKG.txt
	--- tachyon-0.98.9.p2/SPKG.txt  2011-01-24 16:16:05.000000000 +0100
	+++ tachyon-0.98.9.p3/SPKG.txt  2011-03-12 19:49:13.844659953 +0100
	@@ -62,6 +62,9 @@
	 
	 == Changelog ==
	 
	+=== tachyon-0.98.9.p3 (Julien Puydt, March 12, 2011)
	+ * #10820: Made package compile on ARM
	+
	 === tachyon-0.98.9.p2 (Volker Braun, January 24, 2011)
	  * #10681: Remove "CC=cc" for the maxosx (32-bit) make target

And here again, we see that I just made the 6 lines following line 62, 9 lines, by adding the three lines prefixed by '+'.

That's all I did. And that's all I want reviewed.

comment:6 Changed 11 years ago by drkirkby

  • Status changed from needs_review to needs_work

One thing a reviewer should do is to check that all the changes are committed to the Mercuiral repository - clearly I can't do that unless you commit the changes to the repository and make an .spkg available so I can verify it.

The release manager will want to copy your new .spkg into the next Sage release. For that he needs to be able to download it from somewhere.

So despite the fact your changes are small, you must still make the 2.4 MB file available for both the reviewer and the release manager.

If you look in any .spkg, you will find a directory .hg which is the Mercurial repository. Google that.

Since you don't know how to make the changes, I'll explain it. You can probably find similar in the Sage developers guide.

First create a file $HOME/.hgrc. Here's mine. Make the obvious changes.

drkirkby@hawk:~$ cat .hgrc
[ui]
username = David Kirkby <david.kirkby@onetel.net>

[extensions]
# Enable the Mercurial queue extension.
hgext.mq =

I suggest at this point you make a backup of whatever you have done, in case you mess up.

Next you need to commit your changes to the Mercurial repository in the .spkg. Do this from the root directory of Sage:

$ EDITOR=vi # (or use emacs, vim, gedit or what other text editor you want.) 
$ export EDITOR 
$ ./sage -sh

You will get something like this, which is a subshell of Sage.

Starting subshell with Sage environment variables set.
Be sure to exit when you are done and do not do anything
with other copies of Sage!

Bypassing shell configuration files ...

SAGE_ROOT=/export/home/drkirkby/sage-4.7.alpha1
(sage subshell) hawk:sage-4.7.alpha1 drkirkby$ cd spkg/standard/tachyon-0.98.9.p3
SAGE_ROOT=/export/home/drkirkby/sage-4.7.alpha1
(sage subshell) hawk:tachyon-0.98.9.p3 drkirkby$ hg commit

At this point your editor will open. Put on one line

#10820 Add support of the ARM processor to the Tachyon package

Save the file. That will commit the changes to the Mercurial repository in the package. Next you need to create a Mercurial patch that can be attached to this ticket.

SAGE_ROOT=/export/home/drkirkby/sage-4.7.alpha1
(sage subshell) hawk:tachyon-0.98.9.p3 drkirkby$ hg export tip > $HOME/10820-ARM-support-for-Tachyon.patch
SAGE_ROOT=/export/home/drkirkby/sage-4.7.alpha1
(sage subshell) hawk:tachyon-0.98.9.p3 drkirkby$ exit
exit
Exited Sage subshell.
drkirkby@hawk:~/sage-4.7.alpha1$ 

That will create a file $HOME/10820-ARM-support-for-Tachyon.patch which should be attached to this ticket. That's the only file that needs to be attached to the ticket - it will contain all the information needed.

Next tar up the package

$ cd spkg/standard
$ tar cfj tachyon-0.98.9.p3.spkg tachyon-0.98.9.p3

and post a link to where I can download the complete package. Put that link in the ticket description too, and make a note that all patches attached to the ticket are for review purposes only, and that all changes are committed to the Mercurial repository. That helps the release manager.

I'll then review it. Whist I don't have access to an ARM processor, assuming things all look OK, I will give it a positive review.

Dave

Changed 11 years ago by Snark

Mercurial patch

comment:7 Changed 11 years ago by Snark

  • Status changed from needs_work to needs_review

The patch is now from mercurial, and I attached the full patched spkg.

comment:8 Changed 11 years ago by drkirkby

  • Description modified (diff)
  • Milestone set to sage-4.7
  • Status changed from needs_review to positive_review

That's OK now. I can't check it on ARM, but I trust you when you say it solves the problem, so positive review.

The only issue was that you are not supposed to attached .spkg's to trac, as trac is not well suited for handling large files. Instead a link should be provided to where the package can be downloaded. For this reason, I have deleted the package from the trac server, and placed it at

http://boxen.math.washington.edu/home/kirkby/patches/tachyon-0.98.9.p3.spkg

So positive review. I've set the milestone to be 4.7. Hopefully it will get in there. It is a very low risk patch, since it only effects systems with the ARM processor, so I see no reason this should not be merged soon.

Dave

comment:9 Changed 11 years ago by jdemeyer

  • Merged in set to sage-4.7.alpha2
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:10 follow-up: Changed 9 years ago by sdenton

FWIW, Tachyon is not (out of the box) building on the Raspberry Pi; it's uname is armv6l. Adding the architecture in the spkg.install alongside the armv7l line seems to work just fine; would it be possible to include the armv6l in the patch? (Or really, at this point, make a new patch to include it?)

comment:11 in reply to: ↑ 10 Changed 9 years ago by dimpase

Replying to sdenton:

FWIW, Tachyon is not (out of the box) building on the Raspberry Pi; it's uname is armv6l. Adding the architecture in the spkg.install alongside the armv7l line seems to work just fine; would it be possible to include the armv6l in the patch? (Or really, at this point, make a new patch to include it?)

sure, just open a new ticket and post your patch there.

Note: See TracTickets for help on using tickets.