Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clear /tmp/ECL* after building on Solaris + sort out minor SAGE64 issue. #8951

Closed
sagetrac-drkirkby mannequin opened this issue May 11, 2010 · 20 comments
Closed

Clear /tmp/ECL* after building on Solaris + sort out minor SAGE64 issue. #8951

sagetrac-drkirkby mannequin opened this issue May 11, 2010 · 20 comments

Comments

@sagetrac-drkirkby
Copy link
Mannequin

sagetrac-drkirkby mannequin commented May 11, 2010

As reported here

http://groups.google.com/group/sage-devel/browse_thread/thread/54544d8649bd027a

there is a problem when temp files created during the build of ECL. A correct fix requires changes made to ECL source code, but as a temporary fix, it may be sufficient to remove /tmp/ECL* after building ecl.

Upstream: Fixed upstream, in a later stable release.

CC: @williamstein @sagetrac-mvngu [email protected] @kcrisman @nbruin

Component: porting: Solaris

Author: David Kirkby

Reviewer: Nils Bruin

Issue created by migration from https://trac.sagemath.org/ticket/8951

@sagetrac-drkirkby sagetrac-drkirkby mannequin added this to the sage-4.5 milestone May 11, 2010
@sagetrac-drkirkby sagetrac-drkirkby mannequin self-assigned this May 11, 2010
@sagetrac-drkirkby
Copy link
Mannequin Author

sagetrac-drkirkby mannequin commented May 12, 2010

comment:1

I've attached a patch. It removes the files only on Solaris. It also has a couple of minor corrections

  • It no longer reports a 32-bit build. Previously it reported a 32-bit build unless a 64-bit build was requested, but on some systems the default is 64-bit, so the message was incorrect.
  • Checks for SAGE64 being 'yes' and not 'yes' and '1' as before, as there is other code in Sage which prevents '1' being used.

These two are just very minor changes - the main change is to delete /tmp/ECL*

What I find a bit odd, is that I don't see these tmp files on my own Sun Blade 2000 SPARC, which runs Solaris 10 update 8 (05/2009). I can only assume they are created them removed by ecl, so why this should not happen on 't2' is a mystery.

http://boxen.math.washington.edu/home/kirkby/Solaris-fixes/ecl-10.2.1.p0.spkg

Dave

@sagetrac-drkirkby
Copy link
Mannequin Author

sagetrac-drkirkby mannequin commented May 12, 2010

Author: David Kirkby

@kcrisman
Copy link
Member

comment:2

Note #8808 as another ECL spkg update.

Also, the patch about the 'yes' and '1' still says

# Compile for 64-bit if SAGE64 is set to 'yes' or '1' 

although of course the code behaves as Dave describes... but maybe that's ok?

@sagetrac-drkirkby
Copy link
Mannequin Author

sagetrac-drkirkby mannequin commented May 14, 2010

comment:3

I'll create a new package based on Williams at #8808, which address all issues about ECL

@sagetrac-drkirkby sagetrac-drkirkby mannequin changed the title Clear /tmp/ECL* after building ECL Update ECL, clear /tmp/ECL* after building + sort out minor SAGE64 issue. May 14, 2010
@sagetrac-drkirkby sagetrac-drkirkby mannequin changed the title Update ECL, clear /tmp/ECL* after building + sort out minor SAGE64 issue. Clear /tmp/ECL* after building on Solaris + sort out minor SAGE64 issue. May 14, 2010
@sagetrac-drkirkby
Copy link
Mannequin Author

sagetrac-drkirkby mannequin commented May 14, 2010

Attachment: clear-ECL-from-tmp.patch.gz

Mercurial patch - removes tmp files, based on latest verison of ecl

@sagetrac-drkirkby
Copy link
Mannequin Author

sagetrac-drkirkby mannequin commented May 14, 2010

comment:5

Here's a revised ecl-10.4.1.

http://boxen.math.washington.edu/home/kirkby/patches/ecl-10.4.1/ecl-10.4.1.spkg

It includes

  • The latest release of ecl - taken from the package posted at upgrade ecl from 10.2.1 to 10.4.1 #8808
  • Removes /tmp/ECL* on Solaris.
  • Corrected minor issue with SAGE64, which would have in theory worked if SAGE64 was set to '1', but earlier bits of sage force SAGE64 to be only 'yes' or 'no', so there was no point checking for this.
  • Comment in the code about this SAGE64 change is now more accurate.

I've tested this on sage.math and it works fine.

real	2m20.869s
user	1m47.690s
sys	0m23.480s
Successfully installed ecl-10.4.1
kirkby@sage:~/sage-4.4.2.alpha0$ uname -a
Linux sage.math.washington.edu 2.6.24-26-server #1 SMP Tue Dec 1 18:26:43 UTC 2009 x86_64 GNU/Linux

Note, this code to remove /tmp/ECL is not a perfect solution. Once ECL is fixed, this should be removed.*
Dave

@sagetrac-drkirkby
Copy link
Mannequin Author

sagetrac-drkirkby mannequin commented May 14, 2010

comment:6

Note to release manager

Dave

@jasongrout
Copy link
Member

comment:7

Replying to @sagetrac-drkirkby:

Note to release manager

Both of these patches are included in your spkg, right? In that case, the release manager would just use your spkg, and wouldn't apply any patches anywhere.

@jasongrout
Copy link
Member

comment:8

As a reminder here, this should only be merged simultaneously with the new maxima at #8731.

@jasongrout
Copy link
Member

comment:9

As pointed out in #8808, #8645 actually built the new spkg correctly, so we should have started with the spkg from #8645 instead of the spkg from #8808. I've posted a new spkg which removes the directories that #8645 removed, as per the spkg instructions.

@jasongrout
Copy link
Member

comment:10

New spkg up at http://sage.math.washington.edu/home/jason/ecl-10.4.1.spkg

I'm not sure who can review it, since I fixed it, to follow Nils' spkg, but David also fixed something on it.

Maybe kcrisman can review it, in conjunction with the new maxima update?

@sagetrac-drkirkby
Copy link
Mannequin Author

sagetrac-drkirkby mannequin commented May 14, 2010

comment:11

Replying to @jasongrout:

Replying to @sagetrac-drkirkby:

Note to release manager

Both of these patches are included in your spkg, right? In that case, the release manager would just use your spkg, and wouldn't apply any patches anywhere.

My package should work fine.

My reason for saying to add #8808 first is because my patch was based on the patch applied in #8088, so assumed the wording of SPKG.txt in #8088 as a starting point and used the Mercurial repository in #8088 as a starting point. Also, since William updated the package, he should get credit for that ticket. It has already got positive review.

From the point of view of actual code, it would have made no difference whatsoever.

Anyway, you have now posted another version, based on #8645. Someone needs to review it. I looked over spkg-install and SPKG.txt and they look OK to me, but I can hardly review it.

@jasongrout
Copy link
Member

comment:12

I positive reviewed #8808, which was a mistake. That's why I posted a new spkg, giving credit to Nils, William, and you, but using #8645 as a base for your patch. Well, actually I just took your spkg and removed the appropriate directories, since that should end up with the same final result.

If Nils agrees that the changes are good (i.e., the right directories were removed, I think we can mark this as positive review, because then at least 3 pairs of eyes will have looked at the spkg.

@nbruin
Copy link
Contributor

nbruin commented May 18, 2010

comment:13

I confirm that the src directories in
/home/nbruin/ecl-10.4.1.spkg
and
/home/jason/ecl-10.4.1.spkg
agree according to diff -r. Differences are only in
spkg-install and SPKG.txt, as expected (and in .hg).

@mwhansen
Copy link
Contributor

mwhansen commented Jun 6, 2010

Reviewer: Nils Buin

@kcrisman
Copy link
Member

kcrisman commented Jun 6, 2010

Changed reviewer from Nils Buin to Nils Bruin

@sagetrac-drkirkby
Copy link
Mannequin Author

sagetrac-drkirkby mannequin commented Jun 18, 2010

comment:16

Replying to @jasongrout:

As a reminder here, this should only be merged simultaneously with the new maxima at #8731.

Jason,

what is there about this ticket that means it can only be merged simultaneously with the new Maxima at #8731? That Maxima ticket has not been updated for 3 weeks. If ECL & Maxima need to be updated together, that leaves a whole load of issues unresolved about ECL. Some I can think of are

It seems to me we have three choices here:

  • Update ECL, without updating Maxima, which I think you are saying is not possible.
  • Update both Maxima and ECL to the latest versions quickly.
  • Apply all the other very small changes to the ECL 10.2.1 that is in Sage now. So leaving updating ECL to 10.4.1 until a later date.

I've created #9264 to update ECL to 10.4.1 and apply all changes.

It might however be wiser to create another ticket to just apply all the small changes to ECL 10.2.1.

Dave

@sagetrac-drkirkby
Copy link
Mannequin Author

sagetrac-drkirkby mannequin commented Jun 21, 2010

comment:19

#9264 solves this issue, and several others related to ECL. Hence #9264, which has positive review, can be merged whilst solving not just this issue, but others related to building packages in parallel and building on OpenSolaris.

@rlmill rlmill mannequin added r: duplicate and removed p: major / 3 labels Jun 25, 2010
@rlmill rlmill mannequin closed this as completed Jun 25, 2010
@rlmill rlmill mannequin removed this from the sage-4.5 milestone Jun 25, 2010
@wjp
Copy link
Mannequin

wjp mannequin commented Jan 17, 2011

Changed upstream from Reported upstream. Developers acknowledge bug. to Fixed upstream, in a later stable release.

@wjp
Copy link
Mannequin

wjp mannequin commented Jan 17, 2011

comment:22

For what it's worth, jjgarcia reports the original issue with /tmp/ECLINIT.c being used instead of a unique tempfile has been fixed in ECL. It appears to be in the latest ECL release already, but I haven't checked personally if it now works on Solaris.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants