Opened 12 years ago

Last modified 12 years ago

#9603 closed defect

Build iconv in parallel, install it on HP-UX and make it work properly on Solaris 64-bit. — at Version 36

Reported by: David Kirkby Owned by: David Kirkby
Priority: major Milestone: sage-4.6
Component: build Keywords:
Cc: Peter Jeremy, Minh Van Nguyen, Mitesh Patel Merged in:
Authors: David Kirkby Reviewers: Peter Jeremy, Leif Leonhardy
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by David Kirkby)

This patch, which started out with the sole intension of building iconv on HP-UX, with a very trivial change, now has a number of changes to generally improve the package.

  • Use $MAKE instead of make to permit parallel builds.
  • Install iconv on HP-UX in addition to Solaris and Cygwin.
  • Make this build properly 64-bit on all versions of Solaris and OpenSolaris - prior to this, there was a problem on some systems - see #9718.
  • A cleanup of spkg-install, spkg-check and SPKG.txt was also undertaken.

Dave

Change History (43)

comment:1 Changed 12 years ago by David Kirkby

Summary: Force iconv to build on HP-UX in addition to Solaris and Cygwin.Force iconv to build on HP-UX in addition to the previous Solaris and Cygwin.

comment:2 Changed 12 years ago by David Kirkby

Here we can see all tests pass on HP-UX. It should also be clear the changes only effect HP-UX and will have no effect on other platforms.

-bash-4.0$ uname -a
HP-UX hpbox B.11.11 U 9000/785 2016698240 unlimited-user license

-bash-4.0$ ./sage -f iconv-1.13.1.p3

<snip>

./check-translit . Quotes UTF-8 ASCII
./check-translit . Translit1 ISO-8859-1 ASCII
./check-translitfailure . TranslitFail1 ISO-8859-1 ASCII
./check-subst
./test-shiftseq
make[1]: Leaving directory `/home/drkirkby/sage-4.5.2.alpha0/spkg/build/iconv-1.13.1.p3/src/tests'
All the tests for iconv passed
Now cleaning up tmp files.

Changed 12 years ago by David Kirkby

Attachment: 9603.patch added

Mercurial patch to force iconv to build on HP-UX in addition to the Solaris and Cygwin it currently builds on. iconv passes all tests on HP-UX. The changes will have no effect on other platforms.

comment:3 Changed 12 years ago by David Kirkby

Cc: Peter Jeremy Minh Van Nguyen added

comment:4 Changed 12 years ago by David Kirkby

Status: newneeds_review
Summary: Force iconv to build on HP-UX in addition to the previous Solaris and Cygwin.Force iconv to build + install on HP-UX. Currently it is only installed only on Solaris and Cygwin.

The package may be found here.

http://boxen.math.washington.edu/home/kirkby/patches/iconv-1.13.1.p3.spkg

I'd appreciate a review. I realise not many people have access to HP-UX, but it should be apparent this will affect no other platform.

Dave

comment:5 Changed 12 years ago by Peter Jeremy

Reviewers: pjeremy

I am unable to test on HP-UX but visual inspection of the diffs between iconv-1.13.1.p2.spkg and http://boxen.math.washington.edu/home/kirkby/patches/iconv-1.13.1.p3.spkg looks correct. Running both spkg-install and spkg-check on FreeBSD behave as expected.

comment:6 in reply to:  5 Changed 12 years ago by David Kirkby

Replying to pjeremy:

I am unable to test on HP-UX but visual inspection of the diffs between iconv-1.13.1.p2.spkg and http://boxen.math.washington.edu/home/kirkby/patches/iconv-1.13.1.p3.spkg looks correct. Running both spkg-install and spkg-check on FreeBSD behave as expected.

Thank you Peter.

I'm attaching a log of an install on HP-UX.

I also noticed that the message saying it was being installed since the OS was Solaris or Cygwin. I changed that message to:

Installing iconv as the operating system is Cygwin, HP-UX or Solaris

Dave

Changed 12 years ago by David Kirkby

Improve a message as the package installed, to mention HP-UX too.

Changed 12 years ago by David Kirkby

Attachment: HP-UX-install.log added

Build log from a HP C3600 running HP-UX. Note SAGE_CHECK was set to "yes" so the self tests were run. They all pass

comment:7 Changed 12 years ago by David Kirkby

Owner: changed from Georg S. Weber to David Kirkby
Summary: Force iconv to build + install on HP-UX. Currently it is only installed only on Solaris and Cygwin.Force iconv to build + install on HP-UX. Currently it is only installed on Solaris and Cygwin.

comment:8 Changed 12 years ago by Leif Leonhardy

Style-wise, I would invert the test to something like:

if [ "$UNAME" = SunOS -o "$UNAME" = CYGWIN -o "$UNAME" = HP-UX ]; then

    # install spkg/run test suite, check exit code, ...

else

    # print message that iconv will not be installed/tested
    # because *the system's* one is/will be used (rather than
    # the one shipped with Sage), exit 0

fi

(I.e., clarifying the messages a bit, too.)

Dave, you're right, I do not have access to an HP-UX system, but I don't think that's necessary to give it a positive review.

I'll later take a look at the whole...

-Leif

P.S.: Peter is (already) listed as reviewer, should I than delete him in case I give it positive review (and if he hasn't yet)?

comment:9 Changed 12 years ago by Leif Leonhardy

...
Now cleaning up tmp files.
rm: directory /home/drkirkby/sage-4.5.2.alpha0/spkg/build/iconv-1.13.1.p3 not removed.  Cannot remove current directory
Making Sage/Python scripts relocatable...
...

Yet another sage-spkg bug... :|

comment:10 in reply to:  8 ; Changed 12 years ago by David Kirkby

Replying to leif:

Style-wise, I would invert the test to something like:

if [ "$UNAME" = SunOS -o "$UNAME" = CYGWIN -o "$UNAME" = HP-UX ]; then

    # install spkg/run test suite, check exit code, ...

else

    # print message that iconv will not be installed/tested
    # because *the system's* one is/will be used (rather than
    # the one shipped with Sage), exit 0

fi

(I.e., clarifying the messages a bit, too.)

Dave, you're right, I do not have access to an HP-UX system, but I don't think that's necessary to give it a positive review.

I'll later take a look at the whole...

-Leif

P.S.: Peter is (already) listed as reviewer, should I than delete him in case I give it positive review (and if he hasn't yet)?

I try to write my scripts as portable as I possibly can. I've tended to ask on comp.unix.shell where some shell scripting wizards hang out. I've just asked for comment on -o vs ||

It's true that for the bash shell, either -o or || will work, but there are a number of older shells which don't behave well in such circumstances. || is more portable than -o, and && is more portable than -a. Likewise testing for "" causes problems with some shells. I think the problems usually occur if there is more than one -o or -a on a line, which is what is needed here. I try to write my scripts which will work with any shell, though I stick #!/usr/bin/env bash at the top to be consistent with the rest of Sage.

Changing the style introduces risks that I'd rather not introduce. The risk of introducing a bug increases the more changes one makes to the script. I would not be very popular if I tried to implement something for HP-UX that happens to break on some Linux system! At the moment the script does work reliably, so I'd prefer to limit the changes that are necessary to achieve the aim.

I've tested this on Solaris, HP-UX and Linux. Peter has checked it on FreeBSD. If the style of the script is changed, so that testing needs to be repeated.

The top of spkg-install has a description of what it is doing, and a link to the ticket which resulted in the decision to make iconv install only on Solaris and Cygwing, so I'm not really sure there is need for further explanation.

If you feel there needs to be some more comments in the code, I will add them. But I do not feel changing the style is a good idea. It introduces unnecessary risks.

Perhaps Peter has some comments on this.

There's no need to delete Peter as a reviewer, as he has provided valuable input.

Dave

comment:11 in reply to:  9 ; Changed 12 years ago by David Kirkby

Replying to leif:

...
Now cleaning up tmp files.
rm: directory /home/drkirkby/sage-4.5.2.alpha0/spkg/build/iconv-1.13.1.p3 not removed.  Cannot remove current directory
Making Sage/Python scripts relocatable...
...

Yet another sage-spkg bug... :|

Yes, I've seen endless messages about being unable to delete the current directory. This is another instance where one would need to be very careful in changing things. Fixing the bug might actually introduce a bug. Adding a -f just hides the problem. I don't really know the answer to this.

I see this on both Solaris and HP-UX - can't recall if that warning is printed on Linux or not.

Dave

comment:12 in reply to:  11 ; Changed 12 years ago by Leif Leonhardy

Reviewers: pjeremyPeter Jeremy, Leif Leonhardy

Replying to drkirkby:

Replying to leif:

...
Now cleaning up tmp files.
rm: directory /home/drkirkby/sage-4.5.2.alpha0/spkg/build/iconv-1.13.1.p3 not removed.  Cannot remove current directory
Making Sage/Python scripts relocatable...
...

Yet another sage-spkg bug... :|

Yes, I've seen endless messages about being unable to delete the current directory.

This must happen on any system...

This is another instance where one would need to be very careful in changing things. Fixing the bug might actually introduce a bug. Adding a -f just hides the problem. I don't really know the answer to this.

No, the fix is harmless. The solution is not to add -f (it is already -rf btw), but to cd .. before doing so. (It seems somebody added the cd $BASEDIR later without looking at all the code below it.)

I see this on both Solaris and HP-UX - can't recall if that warning is printed on Linux or not.

See above, it's a "must have" :)

comment:13 in reply to:  12 Changed 12 years ago by Leif Leonhardy

Replying to leif:

See above, it's a "must have" :)

Oh, I think I lied. The -f seems to suppress the message on Linux.

comment:14 in reply to:  10 Changed 12 years ago by Leif Leonhardy

Replying to drkirkby:

Replying to leif:

P.S.: Peter is (already) listed as reviewer, should I than delete him in case I give it positive review (and if he hasn't yet)?

I completely missed his comment at that time, and did not notice he added his name himself there, sorry for that. (Though most tickets only list those as reviewers who actually gave positive review... - not a very good practice IMO.)

I try to write my scripts as portable as I possibly can. I've tended to ask on comp.unix.shell where some shell scripting wizards hang out. I've just asked for comment on -o vs ||

Of course a bit funny to both require bash and then try to be as portable as possible with respect to the shell (though test is actually a program, too), but I don't mind.

Changing the style introduces risks that I'd rather not introduce. The risk of introducing a bug increases the more changes one makes to the script. I would not be very popular if I tried to implement something for HP-UX that happens to break on some Linux system! At the moment the script does work reliably, so I'd prefer to limit the changes that are necessary to achieve the aim.

Hmm, my main intention was to just invert the test, because it's bad style (and inefficient and I think more error-prone) as it is now.

Of course you can use [ ... ] || [ ... ] || ... as well, though this invokes more instances of test if it's not a shell built-in.

The top of spkg-install has a description of what it is doing, and a link to the ticket which resulted in the decision to make iconv install only on Solaris and Cygwing, so I'm not really sure there is need for further explanation. If you feel there needs to be some more comments in the code, I will add them.

I meant just the messages visible to the user.

But I do not feel changing the style is a good idea. It introduces unnecessary risks.

See above. Every code change of course carries some risk (cf. the garbage in PARI's spkg-install), but if tickets were always reviewed properly, such things would (or will) not happen. In my opinion cleaning up "grown" code is important, too. (And sometimes it's even better to rewrite things from scratch.)

-Leif

comment:15 Changed 12 years ago by John Palmieri

No, the fix is harmless. The solution is not to add -f (it is already -rf btw), but to cd .. before doing so.

See #9444.

Changed 12 years ago by David Kirkby

Attachment: 9603-cleanup.patch added

Change the style of the tests, in the light of comments from the reviewer.

comment:16 Changed 12 years ago by David Kirkby

I've changed the style of the tests somewhat, which will hopefully make the logic clearer.

http://boxen.math.washington.edu/home/kirkby/patches/iconv-1.13.1.p3.spkg

I've checked it on Solaris and Linux. It installs and passes the tests on Solaris, but does not install on Linux. On Linux it correctly reports that the tests will not be run.

Is that better?

Dave

comment:17 in reply to:  16 ; Changed 12 years ago by Leif Leonhardy

Replying to drkirkby:

I've changed the style of the tests somewhat, which will hopefully make the logic clearer.

Yes, I like that. (Mike will perhaps not be amused by testing for HP-UX prior to Cygwin in spkg-install... ;-) )

There are some typos/grammatical flaws in the comments and messages.

I'd prefer using $UNAME all the time (instead of in addition `uname`).

CFLAGS are overwritten when $SAGE64=yes.

I'd use $MAKE instead of make (in both spkg-check and spkg-install). If parallel make should be disabled in some cases, I would then either do $MAKE -j1 [target] or export MAKE="$MAKE -j1" ; ... $MAKE [target].

I've still not looked at the whole; I think I'll do tomorrow...

comment:18 in reply to:  17 ; Changed 12 years ago by David Kirkby

Replying to leif:

There are some typos/grammatical flaws in the comments and messages.

I'd prefer using $UNAME all the time (instead of in addition `uname`).

CFLAGS are overwritten when $SAGE64=yes.

I'd use $MAKE instead of make (in both spkg-check and spkg-install). If parallel make should be disabled in some cases, I would then either do $MAKE -j1 [target] or export MAKE="$MAKE -j1" ; ... $MAKE [target].

I've still not looked at the whole; I think I'll do tomorrow...

Did you get a chance to look at this? I'd rather fix all the problems at once.

Dave

comment:19 in reply to:  18 ; Changed 12 years ago by Leif Leonhardy

Replying to drkirkby:

Replying to leif:

I've still not looked at the whole; I think I'll do tomorrow...

Did you get a chance to look at this?

No, I'm sorry, not yet. It'll most probably get Saturday or Sunday.

I'd rather fix all the problems at once.

Yes, I understand that.

comment:20 in reply to:  19 Changed 12 years ago by David Kirkby

Replying to leif:

Replying to drkirkby:

Replying to leif:

I've still not looked at the whole; I think I'll do tomorrow...

Did you get a chance to look at this?

No, I'm sorry, not yet. It'll most probably get Saturday or Sunday.

I'd rather fix all the problems at once.

Yes, I understand that.

Any chance of you looking at this? I'd like to get it reviewed and out of the way. What was originally the change of about 50 bytes has now taken a long time!

Dave

comment:21 Changed 12 years ago by Leif Leonhardy

Unfortunately, I'd make a lot of changes (besides those I already mentioned), in different "categories", but most of them more or less important or of cosmetic nature.

The only actual change to the code is quoting all instances of $SAGE_LOCAL, for (far) future support of spaces in $SAGE_ROOT. (In addition, one could test if spaces in it would break configure or make etc.)

In random order:

  • Remove trailing whitespace (and a superfluous semicolon in spkg-install and spkg-check). (I really hate such changes, since they make [cumulative] patches unreadable.)
  • s/== iconv ==/= iconv =/ since it is the top-level heading.
  • Add blank lines below section headings. (I think this is common practice, and makes the plain text more readable.)
  • Move "For more details ..." to (new) "Upstream Contact" section.
  • The following is perhaps (partly) obsolete, but currently completely misleading:

"spkg-install removes ALL files installed by iconv - man pages, docs, etc etc. If iconv gets updated, check these still remove all traces of iconv.

The sizes of the docs and man pages is small, so they are not work removing from the package, as they potentially risk breaking the install."

(In fact nothing is removed after make install. I don't think something has been removed from the upstream source tree; at least in spkg-install nothing gets removed ("patched") from that, and configure doesn't get any --without-... options or alike. In short, when updating the package, one should make sure all traces of previous installations of iconv still get removed prior to [re]installation. Otherwise some code has to be added to remove "useless" parts of iconv from the Sage installation after make install.)

  • One could add the usual "Building a 64-bit version of ..." message.
  • Some messages and comments need clean-up (punctuation, grammar/typos, and IMHO formulation; some messages perhaps also "layout")...

I've looked at the package yesterday, but I don't think I've forgotten something... ;-)

comment:22 Changed 12 years ago by Leif Leonhardy

P.S.: I tried to mark this ticket # long time, but that didn't work.

(Sorry I had lots of other things to do, with really higher priority. Also, not every day can be a Sage Day, and HP-UX support isn't on top of my Sage TODO list... ;-) )

comment:23 in reply to:  21 ; Changed 12 years ago by David Kirkby

Replying to leif:

Unfortunately, I'd make a lot of changes (besides those I already mentioned), in different "categories", but most of them more or less important or of cosmetic nature.

But those lots of changes should be on another ticket. They have nothing to do with fixing the HP-UX issue!

As you know, I am quite keen to improve the quality of Sage, so I will make them. But be aware I've tried to get people to make more important changes before, and William has overruled, saying that the patch fixes the problem it aims to fix, and that other changes should be on another ticket.

I'll produce a new package in a day or so.

The only actual change to the code is quoting all instances of $SAGE_LOCAL, for (far) future support of spaces in $SAGE_ROOT. (In addition, one could test if spaces in it would break configure or make etc.)

I'll try to make all changes.

In random order:

  • Remove trailing whitespace (and a superfluous semicolon in spkg-install and spkg-check). (I really hate such changes, since they make [cumulative] patches unreadable.)
  • s/== iconv ==/= iconv =/ since it is the top-level heading.
  • Add blank lines below section headings. (I think this is common practice, and makes the plain text more readable.)
  • Move "For more details ..." to (new) "Upstream Contact" section.
  • The following is perhaps (partly) obsolete, but currently completely misleading:

"spkg-install removes ALL files installed by iconv - man pages, docs, etc etc. If iconv gets updated, check these still remove all traces of iconv.

The point of that is that if you run make install, then run the package for a second time, it will clean out all the files made on a previous build. You can't do make distclean at that point as there's no makefile. But I'll remove that.

The sizes of the docs and man pages is small, so they are not work removing from the package, as they potentially risk breaking the install."

(In fact nothing is removed after make install. I don't think something has been removed from the upstream source tree; at least in spkg-install nothing gets removed ("patched") from that, and configure doesn't get any --without-... options or alike. In short, when updating the package, one should make sure all traces of previous installations of iconv still get removed prior to [re]installation. Otherwise some code has to be added to remove "useless" parts of iconv from the Sage installation after make install.)

Nothing is removed. It was just a remark. I can remove it if you feel it causes confusion.

  • One could add the usual "Building a 64-bit version of ..." message.

No problem.

  • Some messages and comments need clean-up (punctuation, grammar/typos, and IMHO formulation; some messages perhaps also "layout")...

I'll try, but lets hope there are not too many itterations of this!

I've looked at the package yesterday, but I don't think I've forgotten something... ;-)

comment:24 in reply to:  22 Changed 12 years ago by David Kirkby

Status: needs_reviewneeds_work

Replying to leif:

P.S.: I tried to mark this ticket # long time, but that didn't work.

I'm not sure of what you mean there.

(Sorry I had lots of other things to do, with really higher priority. Also, not every day can be a Sage Day, and HP-UX support isn't on top of my Sage TODO list... ;-) )

I realise that. Thanks for looking at the ticket - it's hard to get anyone to look at HP-UX tickets!

HP-UX can be useful, since where there are problems on one platform but not another, it helps to get a third perspective of it. I'm not convinced there will ever be a complete HP-UX port.

Dave

comment:25 in reply to:  23 Changed 12 years ago by Leif Leonhardy

Replying to drkirkby:

Replying to leif:

Unfortunately, I'd make a lot of changes (besides those I already mentioned), in different "categories", but most of them more or less important or of cosmetic nature.

But those lots of changes should be on another ticket. They have nothing to do with fixing the HP-UX issue!

If you don't emphasize/focus on HP-UX (i.e. e.g. add "improve iconv package"), perhaps more people will take a look at this ticket. :)

As you know, I am quite keen to improve the quality of Sage, so I will make them. But be aware I've tried to get people to make more important changes before, and William has overruled, saying that the patch fixes the problem it aims to fix, and that other changes should be on another ticket.

Perhaps sometimes this. But it really depends on whether the changes are to the Sage library or spkgs, and - more important - on the type of the changes. If major bugs have to be fixed, doing so shouldn't be delayed by cosmetic improvements. Other code changes carry the risk of introducing new issues, so it might be appropriate to separate them. Others are better done at the same time, especially to avoid conflicting patches/tickets, and I think we don't want to create lots of new spkgs in sequence fixing "minor" things step by step.

Either change 10 or 10000 bytes..., i.e. fix a single issue and open a follow-up ticket (with instructions) for all the rest, or do the whole job. My opinion. (Almost nobody will open a ticket to fix a single typo, and people seem unlikely to review "cosmetic" tickets. Similar to the apparently ever-lasting documentation problem.)

I'll produce a new package in a day or so.

Or just upload one (or more) patch(es) prior to creating a new spkg.

  • The following is perhaps (partly) obsolete, but currently completely misleading:

"spkg-install removes ALL files installed by iconv - man pages, docs, etc etc. If iconv gets updated, check these still remove all traces of iconv.

The point of that is that if you run make install, then run the package for a second time, it will clean out all the files made on a previous build. You can't do make distclean at that point as there's no makefile. But I'll remove that.

I'd simply clarify that. (And emphasize somehow this is unrelated to the next sentence/paragraph:)

The sizes of the docs and man pages is small, so they are not work removing from the package, as they potentially risk breaking the install."

Nothing is removed. It was just a remark. I can remove it if you feel it causes confusion.

(See above.) Perhaps make this an explicit, separate suggestion ("TODO:") to avoid confusion.

  • Some messages and comments need clean-up (punctuation, grammar/typos, and IMHO formulation; some messages perhaps also "layout")...

I'll try, but lets hope there are not too many itterations of this!

:) I fear that, too. Perhaps create a separate patch for that (or omit it in the first place), and let Peter or me create a reviewer patch on top of the other changes.

Changed 12 years ago by David Kirkby

Attachment: 9306-more-cleanup.patch added

Correct unquoted "$SAGE_LOCAL" and a bit of a cosmetic cleanup.

comment:26 Changed 12 years ago by David Kirkby

Status: needs_workneeds_review

I've attached a patch, and also updated the .spkg at

http://boxen.math.washington.edu/home/kirkby/patches/iconv-1.13.1.p3.spkg

Hopefully that is to your liking - it not, make a reviewer patch.

I think I'll open a few "clean-up" packages when I want to sneak in an HP-UX patch.

Dave

comment:27 Changed 12 years ago by David Kirkby

Milestone: sage-5.0sage-4.5.3

comment:28 in reply to:  22 Changed 12 years ago by David Kirkby

Replying to leif:

P.S.: I tried to mark this ticket # long time, but that didn't work.

(Sorry I had lots of other things to do, with really higher priority. Also, not every day can be a Sage Day, and HP-UX support isn't on top of my Sage TODO list... ;-) )

I realise HP-UX is not top of your priority list (it is not mine either), but this ticket could have been very simple, but it has now been open for more than a month. The original patch just added the following:

&& [ "x$UNAME" != xHP-UX ]

in a couple of files.

Since then you have asked me to make many changes to the iconv package that are unrelated to the HP-UX patch. I've done them, though I think it is fair to say that the iconv package was already one of the cleaner ones - compare it to Singular, Pari, ATLAS and many others!

But the ticket has now been open a month, and it is two weeks since you last commented. If you don't feel you will be able to review it within the next few days, please let me know and I'll consider trying to find another reviewer.

Dave

comment:29 Changed 12 years ago by Leif Leonhardy

Status: needs_reviewneeds_info

Well, meanwhile iconv was buried rather deep in one of my stacks of notes...

The latest patch looks quite fine, so my reviewer patch will be fairly small. :)

Can you confirm that reinstalling iconv still works, since you also dropped the code that removed the traces of previous iconv installations?

(I only meant the remarks in SPKG.txt were confusing, especially the second - perhaps unrelated - paragraph; see my comment above. I suggested replacing this by "When updating the package, one should make sure all traces of previous installations of iconv still get removed prior to [re]installation.", unless this has become obsolete now, which I don't know.)

Also, can you check if using $MAKE instead of make in both spkg-install and spkg-check (especially when doing a parallel build, i.e. with multiple make jobs) would work?

(You haven't [yet] changed that, but we should always use $MAKE instead of make, since the user might have set it to something else, not limited to e.g. "make -j". As noted above, in case parallel build/check doesn't work, we should use $MAKE -j1 ....)

comment:30 in reply to:  29 ; Changed 12 years ago by David Kirkby

Cc: Mitesh Patel added

Replying to leif:

Well, meanwhile iconv was buried rather deep in one of my stacks of notes...

The latest patch looks quite fine, so my reviewer patch will be fairly small. :)

Relief!!

Can you confirm that reinstalling iconv still works, since you also dropped the code that removed the traces of previous iconv installations?

Yes, it is OK

Although I added the code originally to delete previous parts of the install, I think it is perhaps not wise. It's simply going to be impossible to do this accurately all the time. The best we could do it to get it working for most people most of the time. It might be feasible if a package only installs a few files, but when many files are installed, and the installed files will probably change with operating system, it is hard to do well. I think it's best simply to not try at all. In any case, few if any other Sage packages do this. I can't think of one in fact.

Also, can you check if using $MAKE instead of make in both spkg-install and spkg-check (especially when doing a parallel build, i.e. with multiple make jobs) would work?

I will do.

(You haven't [yet] changed that, but we should always use $MAKE instead of make, since the user might have set it to something else, not limited to e.g. "make -j". As noted above, in case parallel build/check doesn't work, we should use $MAKE -j1 ....)

I realise this. However, I am well aware that making changes like that are well outside the scope of the original ticket, and potentially more risky.

If I get a ticket merged with the title Force iconv to build + install on HP-UX. Currently it is only installed on Solaris and Cygwin and that screws up a build on Linux, I am not going to be a very popular person that is for sure! Enabling parallel builds is easy to do, but difficult to test thoroughly.

Thankfully, iconv is only installed on Solaris and Cygwin, so hopefully this wont cause too many guns to be fired at me if it goes wrong! This ticket has been changed from one that was very safe (adding && [ "x$UNAME" != xHP-UX ] in a couple of places), into one which has major changes to the structure, style and possibly the inclusion of parallel builds. Really I feel those changes should have been on another ticket.

I think I'm going to change the title. And you can be sure I will deflect some of the blame at you if it goes wrong!!!

I'll check to needs review once I've tested this.

Dave

comment:31 in reply to:  30 Changed 12 years ago by Leif Leonhardy

Replying to drkirkby:

Replying to leif:

Can you confirm that reinstalling iconv still works, since you also dropped the code that removed the traces of previous iconv installations?

Yes, it is OK

Although I added the code originally to delete previous parts of the install, I think it is perhaps not wise. It's simply going to be impossible to do this accurately all the time. The best we could do it to get it working for most people most of the time. It might be feasible if a package only installs a few files, but when many files are installed, and the installed files will probably change with operating system, it is hard to do well. I think it's best simply to not try at all. In any case, few if any other Sage packages do this. I can't think of one in fact.

I thought it was some iconv flaw that previously broke reinstallations; in that case, we could perhaps as well have patched the upstream installation procedure.

Also, can you check if using $MAKE instead of make in both spkg-install and spkg-check (especially when doing a parallel build, i.e. with multiple make jobs) would work?

I will do.

(You haven't [yet] changed that, but we should always use $MAKE instead of make, since the user might have set it to something else, not limited to e.g. "make -j". As noted above, in case parallel build/check doesn't work, we should use $MAKE -j1 ....)

I realise this. However, I am well aware that making changes like that are well outside the scope of the original ticket, and potentially more risky.

Perhaps ask Mike if this would currently cause any issues on Cygwin.

If I get a ticket merged with the title Force iconv to build + install on HP-UX. Currently it is only installed on Solaris and Cygwin and that screws up a build on Linux, I am not going to be a very popular person that is for sure! Enabling parallel builds is easy to do, but difficult to test thoroughly.

Thankfully, iconv is only installed on Solaris and Cygwin, so hopefully this wont cause too many guns to be fired at me if it goes wrong!

:)

This ticket has been changed from one that was very safe (adding && [ "x$UNAME" != xHP-UX ] in a couple of places), into one which has major changes to the structure, style and possibly the inclusion of parallel builds. Really I feel those changes should have been on another ticket.

I think I'm going to change the title. And you can be sure I will deflect some of the blame at you if it goes wrong!!!

I thought "HP-UX" already went into a footnote to attract more reviewers... ;-)

(It's now actually a clean-up ticket, too.)

I'll check to needs review once I've tested this.

Ok.

comment:32 Changed 12 years ago by Leif Leonhardy

P.S.: Using make could in fact fail for people who [have to] set MAKE to e.g. gmake or /some/strange/path/to/functional/make.

comment:33 Changed 12 years ago by David Kirkby

Description: modified (diff)
Summary: Force iconv to build + install on HP-UX. Currently it is only installed on Solaris and Cygwin.Speed up installation of iconv by permitting parallel builds. Also install iconv on HP-UX

I've tested this in parallel on my Sun Ultra 27 running OpenSolaris 127 times. It builds and passes all iconv's tests every time.

My machine is under a very heavy load at the minute as I'm running make ptestlong 100 times in a loop! Needless to say iconv takes a while to install. But with MAKE to to make -j12, the time to just install (not run the tests), is

real	1m59.844s
user	0m19.226s
sys	0m9.280s
Successfully installed iconv-1.13.1.p3

So the total CPU time is 28.5 seconds, and the installation time 1m59s for a parallel build with 12 threads on this 4-core, hyperthreaded machine. (1 physical PCU, 4 cores, 8 threads).

With MAKE unset, so a serial build, the installation time rose to 3m:49s

real	3m46.106s
user	0m19.233s
sys	0m9.788s
Successfully installed iconv-1.13.1.p3

The parallel build is faster by a factor of around 1.9. The actual CPU time used remained virtually unchanged. So the parallel install seems worthwhile.

I'll repeat this on t2.math for as long as my patience permits. That has a lot more cores, but is very slow overall. I doubt I'll test as extensively. I'll then ask Mike to test on Cygwin.

I've not updated the package yet. I'll do that when I'm finished more testing.

I've changed the title, so I don't get quite as much thrown at me if this goes wrong! The content of the HP-UX changes are now about 1% I think!

Dave

comment:34 Changed 12 years ago by David Kirkby

Status: needs_infoneeds_review

OK, I'm happy with

http://boxen.math.washington.edu/home/kirkby/patches/iconv-1.13.1.p3.spkg

now. Mike Hansen has said it OK on Cygwin:

On Fri, Aug 27, 2010 at 3:21 AM, Dr. David Kirkby
<david.kirkby@onetel.net> wrote:
> > http://boxen.math.washington.edu/home/kirkby/patches/iconv-1.13.1.p3.spkg
> >
> > I don't believe there will be any issues with Cygwin, but it would be nice
> > to know.
Looks good to me.

--Mike

and I've personally tested it with parallel builds many times.

  • 127 times on OpenSolaris using my Sun Ultra 27 with 12 threads.
  • 50 times on Solaris 10 (t2.math) with 128 threads.
  • 13 times on HP-UX. Single threaded only - this is a uni-processor machine.

In each case, all iconv's tests pass.

Perhaps Leif could add a reviewer patch for anything else he feels needs addressing, as it should be very minor now.

Dave

Changed 12 years ago by David Kirkby

Replaces 'make' by '$MAKE'. Hopefully the last patch needed!

comment:35 Changed 12 years ago by David Kirkby

Status: needs_reviewneeds_work

I'm marking this as needs work, since another problem with iconv (#9718) on some 64-bit Solaris systems has been solved by the libtool developer Ralf Wildenhues. It makes sense to fix that issue at the same time.

I'll address this later.

Dave

comment:36 Changed 12 years ago by David Kirkby

Description: modified (diff)
Priority: minormajor
Status: needs_workneeds_review
Summary: Speed up installation of iconv by permitting parallel builds. Also install iconv on HP-UXBuild iconv in parallel, install it on HP-UX and make it work properly on Solaris 64-bit.

I'm now marking this as needs review.

I've changed the priority from minor to major, as this resolves a problem (#9718) on 64-bit Solaris, and there is a concerted effort to now get Sage to build on 64-bit Solaris and OpenSolaris. Prior to this, the ticket was originally to build iconv on HP-UX, which was a minor issue.

Changed 12 years ago by David Kirkby

Change the place where the flag for 64-bit builds is inserted from CFLAGS to CC.

Note: See TracTickets for help on using tickets.