Skip to content

drawpile: new recipe #2752

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

Merged
merged 7 commits into from
Aug 21, 2018
Merged

drawpile: new recipe #2752

merged 7 commits into from
Aug 21, 2018

Conversation

hako
Copy link
Contributor

@hako hako commented Jun 30, 2018

Drawpile works on Haiku, still a wip.

drawpile

SOURCE_URI="https://github.com/drawpile/Drawpile/archive/$portVersion.tar.gz"
CHECKSUM_SHA256="0ff79e9f95ca02ff016eed3118a83ca62e80a1ca93fe11e3fb8ecd22c1fe37a9"
SOURCE_DIR="Drawpile-$portVersion"
PATCHES=""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove if unneeded.

@@ -0,0 +1,74 @@
SUMMARY="A collaborative drawing program"
DESCRIPTION="Drawpile is a drawing program that lets you share\
the canvas with other users in real time."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the space at the beginning.

@@ -0,0 +1,74 @@
SUMMARY="A collaborative drawing program"
DESCRIPTION="Drawpile is a drawing program that lets you share\
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a space before that \

SUMMARY="A collaborative drawing program"
DESCRIPTION="Drawpile is a drawing program that lets you share\
the canvas with other users in real time."
HOMEPAGE="http://drawpile.net"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use https

lib:libkf5archive$secondaryArchSuffix
lib:libminiupnpc$secondaryArchSuffix
lib:libsodium$secondaryArchSuffix
lib:libgif$secondaryArchSuffix

This comment was marked as outdated.

haiku${secondaryArchSuffix}_devel
devel:libkf5archive$secondaryArchSuffix
devel:libgif$secondaryArchSuffix
devel:libminiupnpc$secondaryArchSuffix
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sort them too

cmd:g++$secondaryArchSuffix
cmd:ld$secondaryArchSuffix
cmd:make
cmd:qmake$secondaryArchSuffix
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if it is cmake based, then it shouldn't need qmake. please check it, and remove it if unneeded.

# TODO Add a rdef icon for Drawpile
mkdir -p $appsDir/Drawpile
cp bin/drawpile $appsDir/Drawpile/Drawpile
cp bin/drawpile-srv $appsDir/Drawpile/drawpile-srv
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the trailing whitespace

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job nevertheless!

Copy link
Member

@fbrosson fbrosson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cosmetic edits

SUMMARY="A collaborative drawing program"
DESCRIPTION="Drawpile is a drawing program that lets you share \
the canvas with other users in real time."
HOMEPAGE="https://drawpile.net"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-HOMEPAGE="https://drawpile.net"
+HOMEPAGE="https://drawpile.net/"

(if you are OK with this tiny change :)
Thanks!

REVISION="1"
SOURCE_URI="https://github.com/drawpile/Drawpile/archive/$portVersion.tar.gz"
CHECKSUM_SHA256="0ff79e9f95ca02ff016eed3118a83ca62e80a1ca93fe11e3fb8ecd22c1fe37a9"
SOURCE_DIR="Drawpile-$portVersion"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

 CHECKSUM_SHA256="0ff79e9f95ca02ff016eed3118a83ca62e80a1ca93fe11e3fb8ecd22c1fe37a9"
+SOURCE_FILENAME="Drawpile-$portVersion.tar.gz"
 SOURCE_DIR="Drawpile-$portVersion"
+
 ARCHITECTURES="!x86_gcc2 ?x86 x86_64"

Links:

drawpile$secondaryArchSuffix = $portVersion
app:Drawpile = $portVersion
"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

 	"
-
 REQUIRES="

(We like to have blank lines at some places, but not between PROVIDES and REQUIRES :)

devel:libsodium$secondaryArchSuffix
devel:libz$secondaryArchSuffix
"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

 	"
-
 BUILD_PREREQUIRES="

(We like to have blank lines at some places, but not between BUILD_REQUIRES and BUILD_PREREQUIRES :)

TEST()
{
make test
}

This comment was marked as outdated.

Copy link
Member

@fbrosson fbrosson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @hako for the update!

Although BUILD seems OK, INSTALL is failing:

[100%] Linking CXX executable ../../bin/drawpile-srv
[100%] Built target drawpile-srv
Collecting files to be packaged ...
cp: cannot stat 'bin/drawpile': No such file or directory
Warning: Command '['bash', '-c', '. /wrapper-script']' returned non-zero exit status 1
Error: Build has failed - stopping.

(I guess it'll be easy to fix, but can't investigate right now, so I'm marking the PR with a "Request changes" so that it won't be merged before we fix the recipe.)

@hako
Copy link
Contributor Author

hako commented Aug 19, 2018

Can confirm, I've changed the build path and it built the package successfully.


TEST()
{
make test
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @hako for the update! The app builds fine on both x86_64 and x86_gcc2 (using x86 secondary arch) and is working fine on x86_64.

haikuporter --test drawpile is not working because we're not passing -DTESTS=on to cmake.

To fix this, we can either:

  1. append a -DTESTS=on to the cmake call in BUILD and, in TEST, replace make test by make -C build test
  2. or change TEST to:
TEST()
{
	cd build
	cmake .. -DCMAKE_EXE_LINKER_FLAGS=-lnetwork -DTESTS=on
	make $jobArgs
	make test
}
  1. Of course, we can keep BUILD as it is right now and completely drop TEST.

I think scenario 1 would be nice. What do you think? Any other opinions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I modified and built the script with scenario 1 instead.

REQUIRES="
haiku$secondaryArchSuffix
lib:libgif$secondaryArchSuffix
lib:libGL$secondaryArchSuffix
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @hako for the update.
I was going to approve and merge the PR but then realized I had forgotten to check the REQUIRES.
It looks like Drawpile does not directly require 2 libs: libGL and libz:

~> objdump -x /system/apps/Drawpile/* | grep NEEDED | sort -iu | grep -Evw 'libroot|libnetwork'
  NEEDED               libgcc_s.so.1
  NEEDED               libgif.so.7
  NEEDED               libKF5Archive.so.5
  NEEDED               libminiupnpc.so.17
  NEEDED               libQt5Core.so.5
  NEEDED               libQt5Gui.so.5
  NEEDED               libQt5Multimedia.so.5
  NEEDED               libQt5Network.so.5
  NEEDED               libQt5Sql.so.5
  NEEDED               libQt5Svg.so.5
  NEEDED               libQt5Widgets.so.5
  NEEDED               libsodium.so.23
  NEEDED               libstdc++.so.6

Although the names are not case-sensitive I think it might be nice to make the caps match, and to explicitly add a few lib:libQt5* which are not yet mentioned:

 REQUIRES="
 	haiku$secondaryArchSuffix
 	lib:libgif$secondaryArchSuffix
-	lib:libGL$secondaryArchSuffix
-	lib:libkf5archive$secondaryArchSuffix
+	lib:libKF5Archive$secondaryArchSuffix
 	lib:libminiupnpc$secondaryArchSuffix
 	lib:libQt5Core$secondaryArchSuffix
 	lib:libQt5Gui$secondaryArchSuffix
+	lib:libQt5Multimedia$secondaryArchSuffix
 	lib:libQt5Network$secondaryArchSuffix
+	lib:libQt5Sql$secondaryArchSuffix
+	lib:libQt5Svg$secondaryArchSuffix
+	lib:libQt5Widgets$secondaryArchSuffix
 	lib:libsodium$secondaryArchSuffix
-	lib:libz$secondaryArchSuffix
 	"

I'm checking this right now, with haikuporter -S drawpile.

BTW, don't worry about libgcc_s.so.1 and libstdc++.so.6. haikuporter is not complaining about them, so my guess is that we don't need to mention lib:libgcc_s$secondaryArchSuffix and lib:libstdc++$secondaryArchSuffix.

Copy link
Member

@fbrosson fbrosson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@fbrosson fbrosson changed the title drawpile: new recipe wip drawpile: new recipe Aug 21, 2018
@fbrosson fbrosson dismissed extrowerk’s stale review August 21, 2018 14:35

The comments have been addressed.

@fbrosson fbrosson merged commit a3e1dea into haikuports:master Aug 21, 2018
@hako hako deleted the drawpile branch August 21, 2018 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants