calc: new recipe for a C-style arbitrary precision calculator

[raefaldhia]

No description provided.

[fbrosson]

Cosmetic edits: whitespaces vs tabs.
BTW, thanks @raefaldhia for this PR, even if it is still work in progress.

[fbrosson]

The trailing whitespaces are at the end of this line.
(There are also multiple whitespaces in BUILD that should be replaced by tabs.)

[fbrosson]

The last 4 cmd:* items in BUILD_PREREQUIRES have multiple whitespaces instead of tabs.

[fbrosson]

Multiple whitespaces here too.

[fbrosson]

A few non-printable chars I did not notice earlier.

[fbrosson]

We have a Ctrl-V Ctrl-V Ctrl-Z between make -f Makefile.simple and INCDIR=$includeDir, and a Ctrl-X after LIBDIR=$libDir.

BTW, I forgot to mention that we usually add one extra tab for continuation lines:

	make -f Makefile.simple INCDIR=$includeDir \
		BINDIR=$binDir \
		LIBDIR=$libDir \
		EXTRA_CFLAGS="-D_HAIKU_ -DUSE_TERMIOS -DHAVE_UID_T"

Hmm, __HAIKU__ gets defined automatically. Maybe we don't need to define _HAIKU_, unless the sources test for it.

[raefaldhia]

What is Ctrl-V Ctrl-Z do in Pe? look like that's my typo because in haiku to copy and paste use ALT sometimes it make me confused hehe and how could you see that? i see nothing

[fbrosson]

I don't use Pe. I use vi (or vim, to be precise) everywhere ;-) Sometimes I use Emacs, but vim loads much faster.
BTW, vim displays all non-printable chars.
If you are not used to vim then I can tell you that learning vim is a very good investment. Vim is available everywhere. Well, to tell the truth, you might find some systems without vi, but you'll always have ed (and probably pico or nano). And since learning ed is automatic if you learn vi, vi is the best choice if you want to use a single editor.
BTW, vi is the only editor which allows to save&quit with only 2 keys: ZZ
Isn't this great? ;-)

[pulkomandy]

You can change the keyboard mapping in Haiku preferences/keymap if you want ctrl to be used for copy and paste.

[fbrosson]

I noticed a few non-printable chars in 3 different lines.
The first ones are to the right of cmd:calc$secondaryArchSuffix: a Ctrl-X and a Ctrl-S.
I guess you love emacs ;-)

[raefaldhia]

What is non-printable chars?

[fbrosson]

Mainly, anything between Control-A to Control-Z.
Vim shows them as ^A to ^Z. You can also find them in a file by running:

cat -v myfile | diff - myfile

[fbrosson]

If your editor does not display non-printable chars then you can pipe it through cat -v to find where these non-printable chars are:

cat -v myfile > myfile2

diff myfile myfile2

Then you can edit myfile2 and look for all occurences of ^ to remove these occurences as well as the first char that appears to its right, In your case these will be X, S, V, V, Z and X.
(Of course, after that you just need to rename the new file and resume work on it.)

Sorry, I can't resist writting that with vim you would not even need all this ;-)
BTW, vim is the most powerful editor... after Emacs.

[fbrosson]

Oops, I forgot to mention something in my previous review:
You can remove 2 empty lines in order to follow the guidelines.

[fbrosson]

According to the guidelines we do not usually have an empty line between PROVIDES and REQUIRES.

[fbrosson]

I guess you meant:

	make -f Makefile install

In which case you could replace it by:

	make install

[fbrosson]

According to the guidelines we do not usually have an empty line between BUILD_REQUIRES and BUILD_PREREQUIRES.

(This is the second empty line I was refering to in my last review... I forgot to clic on a button...)

[raefaldhia]

ok thanks for the code review :D

[fbrosson]

• _x86 needs to be removed from the name of the patchset.
• LN= ln needs to be replaced by LN= ln -s in the Makefile.

There are still other changes to do before INSTALL can work as expected. I think you need to pass to make install in INSTALL the same parameters you passed to make all in BUILD.
I'll let you dig this ;-)

[fbrosson]

Although haikuporter -e calc_x86 generates calc_x86-2.12.5.3.patchset, you should rename that file to calc-2.12.5.3.patchset. For your convenience, here's how I would do so:

cd /path/to/haikuports/sci-mathematics/calc/patches
git mv calc_x86-2.12.5.3.patchset calc-2.12.5.3.patchset
git commit --amend calc_x86-2.12.5.3.patchset calc-2.12.5.3.patchset

Then you should change PATCHES to:

PATCHES="calc-$portVersion.patchset"

These changes will allow the package to be also built on x86 primary arch and x86_64.

[fbrosson]

To fix the issue about the hard links, you can insert this block here:

PATCH()
{
	sed -i \
		-e '/^COL=/ s/=.*/=/' \
		-e 's/^LN= ln$/& -s/' \
		Makefile
}

You could also avoid this method and, instead, change the "LN= ln" into "LN= ln -s" in the Makefile and regenerate the patchset.
Feel free to chose whatever method you prefer. Both are OK ;-)

[fbrosson]

I've updated the PATCH function above. It sets COL to the empty string... and solves the issue about col not being available!

[fbrosson]

I realized that setting COL to the empty string was what was making the build fail when I tried to build the shared libs. (It was complaining about -b not being found... because it was calling $COL -b.)
So the way you handled this in the patchset is much better!
Your idea to pass LN="ln -s" to make (instead of patching the Makefile as I had suggested) is also great! Well done!

BTW, I'm now trying to build the package with the shared libs.

[raefaldhia]

"BTW, I'm now trying to build the package with the shared libs."

Do you mean about calc-dynamic-only?

[fbrosson]

Should be INSTALL() (you forgot an L).

[raefaldhia]

@fbrosson do you know what's the replacement for /usr/share?
omg there's no cmd:ldconfig too

[fbrosson]

I think it is $dataDir (which will later be visible as /system/data/) but I need to check.

When you build calc_x86-2.12.5.3 you get in haikuports/sci-mathematics/calc/work-x86-2.12.5.3 a file called wrapper-script which has plenty of paths. I often look there. I know, I should refer to the docs instead...

[raefaldhia]

Is it done now?

[fbrosson]

A few fixes.
BTW, you did a good job!
(The recipe will still need some polishing, but I need help from more experienced contributors to answer some questions I have.)

[fbrosson]

In order to follow the guidelines, could you please change DESCRIPTION to:

DESCRIPTION="Calc is an interactive calculator which provides for easy large \
numeric calculations, but which also can be easily programmed for difficult or \
long calculations. It can accept a command line argument, in which case it \
executes that single command and exits. Otherwise, it enters interactive mode. \
In this mode, it accepts commands one at a time, processes them, and displays \
the answers. In the simplest case, commands are simply expressions which are \
evaluated."

(The text is unchanged. I've only moved the line breaks, to make all lines as long as possible, but still <= 80 chars long.)

[fbrosson]

Should be:

COPYRIGHT="1999-2016 Landon Curt Noll"

(updated year and removed double white spaces)

[fbrosson]

There should be a tab before the closing double-quotes.

[fbrosson]

There should be a tab before the closing double-quotes.

[fbrosson]

The 2 calls to prepareInstalledDevelLib should be combined into a single one to prepareInstalledDevelLibs:

	prepareInstalledDevelLibs \
		libcalc \
		libcustcalc

[fbrosson]

Good news: it builds fine on x86_gcc2 primary arch!
This means you can (and even should) remove the ! in !x86_gcc2 in both ARCHITECTURES and SECONDARY_ARCHITECTURES ;-)

ARCHITECTURES="x86_gcc2 x86 x86_64"
SECONDARY_ARCHITECTURES="x86_gcc2 x86"

[fbrosson]

Since the Makefile has a check target that launches some tests, you can (and probably should) add a TEST function:

TEST()
{
	make check
}

The TEST function allows to call hp --test calc (after it has been built) to run the tests.
BTW, I've launched the tests with a build for x86_gcc2 primary arch, and I got quite some output, but now it is stuck on:

...
8830: pi() ^ asin(-2) == power(pi(), asin(-2)
8831: (3+4i) ^ (2+3i) == power(3+4i, 2+3i)
8832: ln(-10) ^ (2+3i) == power(ln(-10), 2+3i)
8833: (pi()*1i) ^ asin(-2) == power(pi()*1i, asin(-2))
8834: (exp(1)+pi()*1i) ^ asin(-2) == power(exp(1)+pi()*1i, asin(-2))
8835: Ending test_somenew

8900: Starting test of calc resource functions by Christoph Zurnieden
8901: read -once "test8900"
8902: about to run test8900(1,,8903)

[raefaldhia]

@fbrosson After playing arround i found that the problem caused by perms try to grant write,exec,read perms to all .cal file or try first to 8900.cal

[raefaldhia]

but still there's another 4 error

[fbrosson]

Is your timezone UTC+7 ?
It looks like you have set your computer (or VM) to the UTC timezone and its clock is 7 hours in the future.

I think you should set your timezone to "GMT+07.00".
(Right click on the digital clock and choose "Time preferences...")

GitHub uses the timestamps of your commits to display them at the appropriate positions between our comments. So if your clock is in the future then your commits will allways appear on the bottom of this page (unless we wait 7 hours after your last commit update to add comments).
That said, this is a minor issue, of course!

[fbrosson]

Cool, you made all the changes and the recipe is almost good.
Can you move TEST after INSTALL? (see the guidelines ;-)

I think we don't need to list every cmd:* in BUILD_PREREQUIRES. The most used ones are always available.
You can remove chmod, mkdir, rm, touch, rmdir, cp, cat and ln.
Let's see if other contributors see other cmd:* that we can omit.

[raefaldhia]

Ok done

[fbrosson]

@raefaldhia: Cool, you have correctly set your timezone. Thanks!

I remember now the rule to decide which cmd:* (in BUILD_PREREQUIRES) should be considered as implicit: all commands available in coreutils. To list them:

package list -p /system/packages/coreutils-*-*.hpkg | sed -n -e 's|^bin/||p'

@korli, could you please confirm that we can/should skip in BUILD_PREREQUIRES any cmd:* that is in the coreutils package? Thanks!

[fbrosson]

cosmetic and style changes.

[fbrosson]

After all, maybe it is better to have a single line:

	prepareInstalledDevelLibs libcalc libcustcal

Leaving the command alone and then putting two libs together in the same line is not very common.
(The reason why we sometimes put one library per line is when we have too many or when their names match a nice pattern.)

[fbrosson]

It would be better to protect the * because if, for some reason and during the build, someone does a "touch foobar.cal" in the directory where the find command is launched, then it will not visit any subdirectory. I would also recomment chmod +w instead of chmod 755. That's why I would use:

	find "*.cal" | xargs chmod +w

or

	find \*.cal | xargs chmod +w

Both work. Take the one you prefer!
EDIT: We need to protect the * because there are *.cal files in both $dataDir/calc and some of its subirectories. Not protecting the * would cause the find command to not visit any subdirectory.

[raefaldhia]

find "*.cal" no such file or directory . .

[raefaldhia]

same thing happened for find \*.cal

[fbrosson]

Sorry, it should be:

	find . -name \*.cal

[raefaldhia]

updated

[fbrosson]

Or

	find -name \*.cal

The reason why I forgot the -name is that until recently I did not know that the path was not mandatory, so I had made a mental patch in my brain: "Don't forget that find may also be called without the path". But I must have turned it into the incorrect idea that "find could be called without the -name parameter. I guess I need some sleep ;-)

[raefaldhia]

Ayylmao, updated

[fbrosson]

I don't know if it is worth, but I think it would look nicer with an additional -type f:

	find -type f -name \*.cal | xargs chmod +w

(My rationale here is that we can instruct find to not try to compare any directory name against the *.cal pattern.)

[fbrosson]

You forgot to remove the pipe. It should be:

++	fi > $@

[raefaldhia]

Actually i was unsure about that because i'm not too good at BASH.

[raefaldhia]

Messing around with Makefile and calc-dynamic-only, i got linker error, undefined reference then
when i changed the makefile line ifeq ($(target),Linux) to ifeq ($(target),**Haiku**) and build it with calc_x86 arch it successed, i don't even know what i've done ¯_(ツ)_/¯ , i just thinking that because haiku have /posix directory that contains headers for linux-like then any software that can be compiled on linux also apply on haiku.

for gcc2 seems like we only need to edit some code, how could you produce implicit declaration of function, i just produce undefined reference

[fbrosson]

That's cool, it looks like you have found the right thing to do! Congratulations! I did not get close to that myself!
Regarding the cscript directory, yes, your suggestion looks good. But we need to make sure the contents are the same when we compare its files between a primary arch and a secondary arch. We might also hit the "read-only" problem on secondary arch builds, as explained in my previous message. So we need to be careful with this.
Hmm, in your screen capture I see some thing strange. You have:

• lib/x86/libcustcalc.so.2.12.5.3
• lib/x86/libcalc.so.2.12.5.3

which seems OK, but you also seem have:

• lib/calc/cscript/

That's a little bit strange, because if you are passing SCRIPTDIR=$libDir/calc/cscript (and building for x86 secondary arch) then you would have lib/x86/calc/cscript instead of lib/calc/cscript.
But I guess you have already made the required changes to teach calc_x86 that the cscript will be in lib/calc/cscript for both primary and secondary arch builds. You are an expert!

I need to test your recipe and I'll come back.
If the shared libs is working fine then this is a great step!
This task is definitely very interesting and exciting!

[fbrosson]

Minor changes for the calc_data layout change.
(Everything looks ok but I still need to check a few things.)

[fbrosson]

Shall we use this string?

SUMMARY_data="Calc standard resource files"
Oops, it cannot start with the name of the package (this is a haikuporter rule I had forgotten about.) So we could use:

SUMMARY_data="Standard resource files for calc"

(It's taken inspired from the very first line of the README file in the calc directory.)

[fbrosson]

You can drop the double quotes here if you wish, because there are no spaces in the definition of $includeDir.
(If in doubt about any of these variables, you can look for their definition in work-2.12.5.3/wrapper-script or work-x86-2.12.5.3/wrapper-script.)
Alternatively, you can add double quotes around $libDir, $binDir and all other parameters, to have a uniform style, but I guess the best choice is to drop them where they are not required (i.e. for the INCDIR parameter.)
(Of course, in any case, we need to keep the double quotes for LN, READLINE_EXTRAS and EXTRA_CFLAGS.)

[fbrosson]

Same remark as above: You can drop the double quotes here if you wish.

[fbrosson]

Can you please replace this block:

	if [ -z "$secondaryArchSuffix" ]; then
		packageEntries data \
			$dataDir/calc
	fi

by these two blocks:

	if [ -z "$secondaryArchSuffix" ]; then

		# Shorten some paths in the man page.
		sed -i \
			-e "s|#!$binDir/calc\>|#!/$relativeBinDir/calc|" \
			-e "s|$prefix/|/system/|" \
			$manDir/man1/calc.1

		# Some files in $dataDir/calc/help/ have code snippets with
		# long shebangs that we can shorten. Currently, these files are
		# cscript, full, libcalc and usage.
		sed -i -e "1 s|$binDir/calc |/$relativeBinDir/calc |" \
			$dataDir/calc/help/*

		packageEntries data \
			$dataDir/calc
	else
		rm -rf $dataDir/calc $manDir
	fi

	# Shorten the paths to calc in the shebangs of the files in cscript.
	sed -i -e "1 s|$binDir/calc |/$relativeBinDir/calc |" \
		$libDir/calc/cscript/*

The changes in the first block will ensure that we don't ship the /system/data/calc/ in the base package of secondary arch builds. It also fixes the shebangs in a few files in /system/data/calc/data/help/.

The second block fixes the shebangs in the scripts that will go to /system/lib/calc/cscripts/ (on primary arch) or /system/lib/x86/calc/cscripts/ (on x86 secondary arch).

[fbrosson]

The reason why we need to remove the $dataDir/calc directory when building for a secondary architecture is that during a secondary arch build haikuporter does not know we have a calc_data sub-package, so it thinks that everything we did not move to the calc_devel package should go to the calc base package.
Removing that dir is therefore required otherwise /system/data/calc/ will also be in the calc_x86 hpkg.

[raefaldhia]

@fbrosson, have you trying to install the package that contain the calc_data ? I have some problem when installing the package, and today until friday i have some exam to do, i'am not sure i could fix this issue at that day, i'll do some easier task because i couldn't be productive during exam, so would you let me to do another tasks first? And I suggest don't merge this PR until i'am back.

[fbrosson]

OK, no problem. I'll validate you task on GCI and keep this PR open.
(I had no issues with the calc_data, but I guess this was because I added a couple of minor changes, as explained in my last review).

[raefaldhia]

Updated, what actually this recipe needs, i'm forget, i'll post some TODO to PR desc. calc-dynamic-only now working.

[fbrosson]

Sorry for this late reply. (I was AFK all day.)
Thanks for your update!

I've played with the recipe a week ago and I also found how to build the shared libs.
The first method requires patching the Makefile.
The second method, if I'm not mistalen, is easier: we only need to pass a parameter to make to make it think we are building on Linux. I need to retest this to make sure it works fine.

I guess you have also found how to build the shared lib.
So I suggest that we do everything in this PR.
Feel free to suggest your changes with updates to this PR.
I'll give you hints if you wish ;-)
The goal is to have a polished recipe with everything working, all in a single commit with you as the author ;-)

I need to review the work of anotther student and then I'll come back to this PR. Thanks!

[fbrosson]

More polishing to replace $prefix by shorter paths.

[fbrosson]

Can you please replace:

		sed -i -e "1 s|$binDir/calc |/$relativeBinDir/calc |" \
			$dataDir/calc/help/*

by

		sed -i \
			-e "s|$binDir/calc\>|/$relativeBinDir/calc|" \
			-e "s|$prefix/|/system/|g" \
			$dataDir/calc/help/*

(The last line is unchanged. I'm only copying it here to provide the full command.)

[fbrosson]

Can you please insert the following block here?

	# Shorten the paths to bin/calc in several shebangs.
	sed -i -e "1 s|$binDir/calc\>|/$relativeBinDir/calc|" \
		$libDir/calc/cscript/*

[fbrosson]

BTW, I think it might be possible to simplify the patch by passing the appropriate parameters to the make command, like it is already the case for HAVE_USTAT, instrad of patching the Makefile. I did not try that yet, however.

[raefaldhia]

@fbrosson Some of them that i patched in makefile ex: HAVE_STDLIB_H instead commandline because there's bug, it wont apply to the makefile when i patch it with commandline.

[fbrosson]

OK, no problem.

With the changes in my last review we'll have a really polished recipe anyway ;-)

[fbrosson]

Can you please insert the following block here?

Oops, you did not place the block in the right location but inside the test that only applies when we are building for a primary arch.
The block needs to be outside of (and after) the if [ -z "$secondaryArchSuffix" ]; then .. else ... fi
Here's a bigger context:

	if [ -z "$secondaryArchSuffix" ]; then
		# Shorten some paths in the man page.
		sed -i \
			-e "s|#!$binDir/calc\>|#!/$relativeBinDir/calc|" \
			-e "s|$prefix/|/system/|" \
			$manDir/man1/calc.1

		# Some files in $dataDir/calc/help/ have code snippets with
		# long shebangs that we can shorten. Currently, these files are
		# cscript, full, libcalc and usage.
		sed -i \
			-e "s|$binDir/calc\>|/$relativeBinDir/calc|" \
			-e "s|$prefix/|/system/|g" \
			$dataDir/calc/help/*

		packageEntries data \
			$dataDir/calc
	else
		rm -rf $dataDir/calc $manDir
	fi

	# Shorten the paths to bin/calc in several shebangs.
	sed -i -e "1 s|$binDir/calc\>|/$relativeBinDir/calc|" \
		$libDir/calc/cscript/*

	packageEntries devel \
		$developDir
}

Actually, when we click on a line to add a comment or a review,, that line is shown as the last line of the conext.

[raefaldhia]

Oops sorry

[fbrosson]

Thanks for your update!
May I suggest 2 minor style changes?

BTW, in the commit message you might wish to replace:

Adding new recipe for calc c-style arbitary calculator and its patches to build it on haikuports.

by

Adding new recipe for calc, a c-style arbitary precision calculator.

Alternatively, you might wish to use just the following commit title (and no detailed commit message at all):

calc: new recipe for a c-style arbitary precision calculator.

[fbrosson]

I think it might be a good idea to replace:

	make target=Haiku \
		INCDIR="$includeDir" \

by

	make \
		target=Haiku \
		INCDIR=$includeDir \

or maybe just drop the double quotes around $includeDir, or add double quotes around $libDir and $binDir to have a consistent style.

[fbrosson]

I think it might be a good idea to replace:

		INCDIR="$includeDir" \

by

		INCDIR=$includeDir \

in order to have a consistent style with the way we're passing $libDir and $binDir.

[fbrosson]

Thanks @raefaldhia for your great work!

To the project leaders: Shall we merge this PR?

[raefaldhia]

@fbrosson Thank you too for all of your guide and correction xd.

[pulkomandy]

To the project leaders: Shall we merge this PR?

Why not?

@fbrosson: btw, there is not a very clear "project leaders" at HaikuPorts. With all the work you are doing here, you are part of it I think. So if you don't have any more improvements to suggest, let's merge it. We can always come back and fix things as needed.

[fbrosson]

Thanks @pulkomandy for your kind reply ;-)

Well, I was just wondering if other contributors had noticed anything that could be improved before the merge, like, for instance, simplifying the patch by passing some extra parameters to the make command instead of changing them in the Makefile.

Well, I guess there is already so much great work in this PR (thanks to @raefaldhia) that, as you say, we can merge it now. It even has all the polishing we could identify, regarding the long /packaging/-style paths.

[fbrosson]

Now that this PR has been merged, the branch it is based on is no longer needed.
You should therefore click "Delete branch" for this PR to get rid of an unneeded branch in your repo. Thanks!
GitHub help: Deleting unused branches