Page MenuHomeFreeBSD

New port: net-im/telegram-desktop: Telegram Desktop messaging app
ClosedPublic

Authored by yuri on Dec 11 2017, 3:47 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Nov 15, 4:26 AM
Unknown Object (File)
Wed, Nov 13, 10:41 AM
Unknown Object (File)
Mon, Nov 4, 5:59 AM
Unknown Object (File)
Sat, Nov 2, 5:50 AM
Unknown Object (File)
Wed, Oct 30, 8:31 AM
Unknown Object (File)
Wed, Oct 30, 8:30 AM
Unknown Object (File)
Wed, Oct 30, 8:30 AM
Unknown Object (File)
Wed, Oct 30, 8:30 AM
Subscribers

Diff Detail

Repository
rP FreeBSD ports repository
Lint
No Lint Coverage
Unit
No Test Coverage
Build Status
Buildable 13465
Build 13691: arc lint + arc unit

Event Timeline

tcberner added inline comments.
net-im/telegram-desktop/Makefile
17

^ USES=python:build ?

19

^ how are alsa and pulse only build depends? does it not link against libasound.so or libpulse.so?

26

^ why does it not build with ninja?

net-im/telegram-desktop/files/gyp-patches
9

^I would sed in %%QT_BINDIR%% instead of %%LOCALBASE%%/bin

101

^
I would sed in %%QT_INCDIR%% and not %%LOCALBASE%%/include/qt5

112

^ %%QT_LIBDIR%%

137

%%QMAKESPEC%% otherwise this will likely break with gcc

149

if you hardcode the path, use %%QT_BINDIR%%/moc or %%MOC%%

net-im/telegram-desktop/files/patch-Telegram_SourceFiles_main.cpp
26

^%%QT_PLUGINDIR%%? or do they want ${PREFIX}/bin/lib/qt5/plugins?

This revision now requires changes to proceed.Dec 11 2017, 5:55 AM
yuri marked 9 inline comments as done.Dec 11 2017, 6:10 AM
yuri added inline comments.
net-im/telegram-desktop/Makefile
19

Executable doesn't link with these. They load some things dynamically.

26

ninja breaks with bad scripts.

net-im/telegram-desktop/files/patch-Telegram_SourceFiles_main.cpp
26

This one isn't %%FOO%%-based. There is a mistake, it was meant to be ../lib

yuri marked 3 inline comments as done.

.

yuri marked 3 inline comments as done.Dec 11 2017, 6:10 AM
net-im/telegram-desktop/Makefile
19

It also loads gtk dynamically.

yuri marked an inline comment as done.Dec 11 2017, 6:15 AM
net-im/telegram-desktop/Makefile
19

but how does that make any sense? if you need/want alsa&pulse at runtime, add it to the run depends, also :)

net-im/telegram-desktop/files/patch-Telegram_SourceFiles_main.cpp
26

conditions still aply :D -- I would sed in %%QT_PLUGINDIR%% instead of this whole thing "garbling a path together"-thingy -- also this should not be necesssary [tm].

yuri marked an inline comment as done.Dec 11 2017, 6:23 AM
yuri added inline comments.
net-im/telegram-desktop/Makefile
19

I am not really familiar with this app. But it can make sense if these are optional runtime dependencies. This means that it will use them if they are present, and will also happily work without them. At least that's what the submitter claims. And I myself did link projects this way.

yuri marked an inline comment as done.

.

yuri marked an inline comment as done.Dec 11 2017, 6:37 AM
yuri added inline comments.
net-im/telegram-desktop/files/patch-Telegram_SourceFiles_main.cpp
26

Conditionals apply in regular patches?

yuri marked 3 inline comments as done.Dec 11 2017, 6:38 AM
net-im/telegram-desktop/files/patch-Telegram_SourceFiles_main.cpp
26

I meant an error in the patch does not negate, that just using QT_PLUGINDIR is what is needed :)

yuri marked an inline comment as done.Dec 11 2017, 7:30 AM

I also tried to run it, and it works fine.

So, the ball is on your side. :)

You need to change the %%QT_PLUGIN_DIR%% thingy, as the current patch assumes, that qt5-libraires are installed in ${PREFIX}/bin/../lib/qt5 --which may not be the case. However, %%QT_PLUGIN_DIR%% will point to the correct path (and if not, then everything is broken anyway :) ).

Note, the applications gets installed to $PREFIX and Qt lies in $LOCALBASE which may be different, so the relative path shenanigans are wonky (of course its correct for 99.995% of the cases).

You need to change the %%QT_PLUGIN_DIR%% thingy, as the current patch assumes, that qt5-libraires are installed in ${PREFIX}/bin/../lib/qt5 --which may not be the case. However, %%QT_PLUGIN_DIR%% will point to the correct path (and if not, then everything is broken anyway :) ).

I just changed.

net-im/telegram-desktop/Makefile
16

@${PY_FLAVOR}

19

This goes against everything we do in the Ports tree. Software must not tentatively try to use stuff that may, or not, be there.

If you build with alsa or pulseaudio, then also add it as run dependencies, so that it is used.

Or make them options so that users can choose which they want.

yuri marked 2 inline comments as done.Dec 11 2017, 5:24 PM
yuri added inline comments.
net-im/telegram-desktop/Makefile
19

I made alsa and pulseaudio LIB_DEPENDS.

yuri marked an inline comment as done.

.

net-im/telegram-desktop/Makefile
33–34

Does this really need gtk3 and qt5 ? It feels strange to require both.

yuri marked 2 inline comments as done.Dec 12 2017, 5:33 PM
yuri added inline comments.
net-im/telegram-desktop/Makefile
33–34

They link to gtk for notifications to work on gtk systems. I guess, they try to cover both gtk2 and gtk3 based systems. For example, the qTox port also links to gtk20 for this reason.

yuri added inline comments.
net-im/telegram-desktop/Makefile
19

Software must not tentatively try to use stuff that may, or not, be there.

@mat , Please look at USE_GNOME=glib20 gtk30 in this case. It only uses gtk to enable desktop notifications in case of gtk-based systems. Otherwise, this is a Qt app.

Don't you think this is good to load gtk dynamically and ignore failures? This eliminates unnecessary dependencies. It doesn't have to install gtk when it isn't really needed just to enable notification that aren't shown anyway.

So, it should really be USE_GNOME=glib20:build gtk30:build. Unfortunately, USE_GNOME=gtk30:build is broken.

net-im/telegram-desktop/Makefile
19

What are you talking about ? I simply asked a question, which you answered, leave it at that.

Of course USE_GNOME=gtk30:build does not do what you ask, gtk30 is a shared library, it must not be used at build time only, otherwise the software will end up being broken in the end. (I don't care that it "dynamically links it at run-time" if it needs it, it needs it.)

This comment was removed by yuri.
yuri marked 2 inline comments as done.Dec 15 2017, 6:01 AM

This is some impressive work! Fix up those MKDIR's and go for it.

net-im/telegram-desktop/Makefile
78

${MKDIR}

80

${MKDIR}

83

${MKDIR}

This revision was not accepted when it landed; it landed in state Needs Review.Dec 18 2017, 11:36 PM
This revision was automatically updated to reflect the committed changes.
yuri marked an inline comment as done.

This is some impressive work! Fix up those MKDIR's and go for it.

This is mostly Henry Hu's. -)

Telegram is famous, and has some non-trivial content like some shady organizations' chans, and has features requiring the server pull, like offline messages.

But security-wise I still think serverless messengers are much more secure, ex. qTox/Bitmessage/Ring.