sugarlabs / sugar

Sugar GTK shell
GNU General Public License v3.0
252 stars 240 forks source link

Depend on empy python module #906

Closed srevinsaju closed 4 years ago

srevinsaju commented 4 years ago

Fixes #882 Fixes #896 Fix makefile dependency on /usr/bin/empy for Linux distributions which do not package a empy binary executable.

In comparison to @aperezbios 's patch on #896, I have tried to call python3 -m em so that the

I am not sure if what I have attempted to fix is with regards in the Coding guidelines. The semantics seems correct, I have tested it. Works satisfactorily. Please review @quozl @aperezbios @tchx84

quozl commented 4 years ago

Thanks. Reviewed.

Next steps;

srevinsaju commented 4 years ago

@quozl I attempted to fix that thing which you have told. Please review

quozl commented 4 years ago

Thanks, but that's not quite what I meant. The three lines of the test were these;

$PYTHON -m em -V
EMPY_VERSION=$?
AS_IF([test "${EMPY_VERSION}" = "0" ], 

And a quick search of configure.ac files on a few hundred projects shows it can also be done like this;

AS_IF([$PYTHON -m em], 

A good example is the Telepathy Salut configure.ac file which has this;

AS_IF([$PYTHON -c "import twisted.words.xish.domish, twisted.words.protocols.jabber, twisted.internet.reactor, avahi" >/dev/null 2>&1],

Note the output redirection to quash noise.

Another is from systemd;

AS_IF([$($PKG_CONFIG --exists bash-completion)],

Also, your change was incorrect, the AC is missing from AC_MSG_RESULT.

Please take the time to do it right, and test. If you don't have the time, then don't do it. :grin:

srevinsaju commented 4 years ago

I apologize for the missing AC, I remember testing the build, but yea. I attempted to create this PR with particularly no knowledge about make build systems, I just wanted to learn how to do it, and the best way to learn is probably by doing.

I am not sure what dev/null in AS_IF([$PYTHON -c "import twisted.words.xish.domish, twisted.words.protocols.jabber, twisted.internet.reactor, avahi" >/dev/null 2>&1], does, but it looks cool, and works quite efficiently too.

Tested on Arch Linux, Sugar 0.116 latest source.

chimosky commented 4 years ago

I apologize for the missing AC, I remember testing the build, but yea. I attempted to create this PR with particularly no knowledge about make build systems, I just wanted to learn how to do it, and the best way to learn is probably by doing.

You're doing a good job, I doubt @quozl meant any harm.

I am not sure what dev/null in AS_IF([$PYTHON -c "import twisted.words.xish.domish, twisted.words.protocols.jabber, twisted.internet.reactor, avahi" >/dev/null 2>&1], does, but it looks cool, and works quite efficiently too.

>/dev/nul 2>&1 redirects any output to /dev/null and any output redirected there is discarded, like @quozl said earlier it was used to "quash noise". You can read about /dev/null here.

quozl commented 4 years ago

Thanks.

Just one final typo; missed space after comma and before AC_MSG_RESULT. Has no effect other than making the code readable.

Next steps;

Answering from the comments above;

When testing, you should test both paths of the conditional statement; i.e. with and without empy installed. With the AC missing before, that path of the statement should not have worked properly, and so your test should have failed.

The /dev/null is a Linux kernel device which when opened and read immediately behaves like an empty file, and when opened and written discards everything. You'll often see it used for that purpose. In this scenario, it is enough for the configure script to say that empy is not found, and so the failure to import empy need not be reported. It saves confusion on the part of the user, which we've certainly seen from our new developers, who will spend all their time looking at a part of an error and miss the actual cause. :grin:

The 2>&1 is a shell redirection binding; it connects the standard error stream to the standard output stream. Python writes tracebacks to the standard error stream. With both streams redirected to /dev/null the traceback is intentionally lost.

(You may also be interested in /dev/zero, a Linux kernel device that when opened and read returns zero bytes without ever reporting end of file.)

srevinsaju commented 4 years ago

yup, thanks for the info, this time I tested both cases.

srevinsaju commented 4 years ago

Seems like I would not be able to test on Fedora (low on bandwidth) @chimosky if you use Fedora, could you help me test this 🥺

chimosky commented 4 years ago

Tested on fedora, works as expected. @srevinsaju apply patch to sugar-artwork so I can test.

Logs for anyone's interest; when empy is installed. When it's not installed.

PS: Logs are trimmed.

quozl commented 4 years ago

@chimosky, thanks for testing.

Thanks also to @srevinsaju for extending this to sugar-artwork.