MightyCreak / diffuse

Diffuse is a graphical tool for comparing and merging text files. It can retrieve files for comparison from Bazaar, CVS, Darcs, Git, Mercurial, Monotone, RCS, Subversion, and SVK repositories.
http://mightycreak.github.io/diffuse/
GNU General Public License v2.0
269 stars 45 forks source link

SyntaxError on /usr/share/diffuse/diffuse/utils.py:188 or :191 #137

Closed Philipp91 closed 2 years ago

Philipp91 commented 2 years ago

I'm on Linux Mint (ok, I'll admit I'm somehow getting this from testing/main and not a stable Debian branch):

$ sudo apt-get install diffuse
...
diffuse (0.7.3-1) wird eingerichtet ...
File "/usr/share/diffuse/diffuse/utils.py", line 188
startupinfo=info) as proc
^
SyntaxError: invalid syntax

python3 /usr/share/diffuse/diffuse/utils.py gives the same error, but python ... gives a different one, which leads me to believe that Python 3 is correctly being used by dpkg.

This is the line in question (it's line 191 now, but otherwise the same). If I download the file from master, I get the same SyntaxError.

Philipp91 commented 2 years ago

I wonder why the continuous integration system doesn't catch this. It didn't exist back on Nov 20 when the bug was merged in #117, but nowadays it just succeeds anyway.

Philipp91 commented 2 years ago

FTR pip install -r requirements.dev.txt from here fails for me.

My suspicion is that Meson/Flatpak only copy around the files without running them through the Python interpreter. This may be due to a misconfiguration or due to a lack of unit tests (if those build systems were to run unit tests automatically -- no idea).

MightyCreak commented 2 years ago

Hi!

Thanks for contributing!

I wonder why it does works on my machine though. How come the added parentheses fails on your machine?

I've added the parentheses for readability reasons. It allows to easily see when the function header finishes and the function body starts.

I prefer this:

    with (subprocess.Popen(
        cmd,
        stdin=subprocess.PIPE,
        stdout=subprocess.PIPE,
        stderr=subprocess.PIPE,
        cwd=dn,
        startupinfo=info) as proc
    ):
        proc.stdin.close()
        proc.stderr.close()
        fd = proc.stdout
        # read the command's output
        s = fd.read()
        fd.close()
        if proc.wait() not in success_results:
            raise IOError('Command failed.')
        return s

Over this:

    with subprocess.Popen(
        cmd,
        stdin=subprocess.PIPE,
        stdout=subprocess.PIPE,
        stderr=subprocess.PIPE,
        cwd=dn,
        startupinfo=info) as proc:
        proc.stdin.close()
        proc.stderr.close()
        fd = proc.stdout
        # read the command's output
        s = fd.read()
        fd.close()
        if proc.wait() not in success_results:
            raise IOError('Command failed.')
        return s
MightyCreak commented 2 years ago

Oh I know why.. They added the possibility to use parentheses in Python 3.10.

Documentation:

MightyCreak commented 2 years ago

About that:

FTR pip install -r requirements.dev.txt from here fails for me.

What's happening? I've just ran it a few minutes ago and it works perfectly here. Could you paste the output?

Philipp91 commented 2 years ago

The first two posts here were the error and the solution. Maybe sudo apt install libcairo2-dev could be done automatically or be documented somewhere here?

Also maybe https://mightycreak.github.io/diffuse/development.html should link to https://github.com/MightyCreak/diffuse/blob/master/docs/developers.md ?

It doesn't look like Meson runs the linter at all -- at least I get no error when following the Meson instructions up to the install command.

MightyCreak commented 2 years ago

Maybe sudo apt install libcairo2-dev could be done automatically or be documented somewhere here?

It can't be done automatically because every Linux distributions don't have the same package managers, but I sure can add it to the documentation. From the CI/CD script, I think what we actually need is libgirepository1.0-dev. I'll need to do some tests in a clean Debian image to see which dependencies are truly needed (as I have several projects, I've installed a couple of dev packages, so it's not always obvious which are needed by this specific project)

Also maybe https://mightycreak.github.io/diffuse/development.html should link to https://github.com/MightyCreak/diffuse/blob/master/docs/developers.md ?

Maybe.. The docs/ folder is pretty new, I decided to create it when the main README.md was starting to become too big. I should take a look at the website as well. I think it would be interesting to migrate to a static generator that would automatically take the pages from the docs/.

It doesn't look like Meson runs the linter at all -- at least I get no error when following the Meson instructions up to the install command.

Indeed it doesn't. I wouldn't add it in the "install" pass but rather in the "test" pass. That said, the best is to run it directly within the IDE (still haven't figured that out ATM). If you know how, contributions are very welcome ;)

MightyCreak commented 2 years ago

@Philipp91 I've added the distributions dependencies in the documentation: https://github.com/MightyCreak/diffuse/blob/master/docs/developers.md

Philipp91 commented 2 years ago

Great, thank you!