coin3d / pivy

python bindings to coin3d
ISC License
53 stars 37 forks source link

fails to run if qmake is not called "qmake" #44

Closed J-Dunn closed 5 years ago

J-Dunn commented 5 years ago

Hi,

on Fedora 29 qmake has a suffix to distinguish various versions. In my case it's qmake-qt4.

when setup.py is run it crashes out

  File "/usr/lib64/python2.7/subprocess.py", line 1047, in _execute_child
    raise child_exception
AttributeError: 'NoneType' object has no attribute 'rfind'

It appears that this test for qmake has never been tested on a system where it is NOT present.

The hack for Fedora is to edit the line to look for 'qmake-qt4' but the code should handle this better than crashing out.

looooo commented 5 years ago

I guess you mean cmake. Pivy does not depend on qmake. But yes the build of pivy now depends on cmake. But only if you build coin with cmake. If not please use the setup_old.py .

J-Dunn commented 5 years ago

Why do you assume I mean something else instead of reading code I posted ?

I'm running a python script, which is calling another python script, which is looking for QMAKE not cmake. Because the code is not well tested, when it does not find the executable it is expecting it craps out with a crash instead of closing with a suitable error message.


class QtInfo(object):
    def __init__(self, qmake_command=None):
        if qmake_command:
            self._qmake_command = qmake_command
        else:
            self._qmake_command = [find_executable("qmake-qt4"),]
        self._dict = {}

Why it is looking qmake is not my business. I'm just flagging a bug and providing a solution for a specific platform where this crashes in a messy way. Maybe someone who is a collaborator could fix it in a clean way using this information.

Maybe some should be testing the result of find_executable() before trying to use it as an object?

looooo commented 5 years ago

ah now I see, forgot about this stuff. I hope this makes you happy: https://github.com/FreeCAD/pivy/pull/45/commits/db67d4dd75bec582f61377a143fc736c4dc27f58

J-Dunn commented 5 years ago

Well maybe you need to test your commits before committing. You seem to have a typo there and it probably does not work. Like I suggested elsewhere, there seems to be a habit of writing code to test for things which never gets run in a context where it can be in the fail condition, or no thought is given to whether the result is valid, like the null object causing a mess when it gets dereferenced later.

You should also test because IIRC when I tried a similar get() someone else suggested, it did not exist.

Obviously running the code before a commit would be a good way not to introduce more bugs.

J-Dunn commented 5 years ago
   if config_dict.get['SOQT_FOUND','false'] == 'false':
TypeError: 'builtin_function_or_method' object has no attribute '__getitem__'
looooo commented 5 years ago

Can't see my mistake. And again: I can't test this because I do not run your system. And I don't want to try out all possible system configurations.

btw.: commits are tested with travis: https://travis-ci.org/FreeCAD/pivy/builds/461811025?utm_source=github_status&utm_medium=notification

looooo commented 5 years ago

Ah maybe you meant this typo: https://github.com/FreeCAD/pivy/pull/45/commits/9deaae4c4e7a5a803137e805af14b305716d1762

J-Dunn commented 5 years ago

yep, I thought the typo was so obvious, I would not need to spell it out. Have you tried to run it on your platform at least, mine does not accept the get(), as posted above.

J-Dunn commented 5 years ago

It did work with std code once I had fixed other errors. I think you posted somewhere that this "appeared" to be SoQt from msg but that it was something unrelated.

                         Welcome to Pivy 0.6.5a0!
                 Building Pivy has never been so much fun!

Platform...linux2
Python version...2.7.15
Checking for swig...
'/bin/swig'
Checking for SWIG version...
3.0.12
calling: cmake 

checking for COIN via cmake
COIN_FOUND: true
COIN_VERSION: 4.0.0
COIN_INCLUDE_DIR: /usr/local/include
COIN_LIB_DIR: /usr/local/lib64

checking for SOQT via cmake
SOQT_FOUND: false
SOQT_VERSION: 
SOQT_INCLUDE_DIR: 
SOQT_LIB_DIR: 

disable soqt, because cmake couldn't find it

coin-features are not supported in this version
Preparing Inventor headers:.
looooo commented 5 years ago

it's not config_dict.get['SOQT_FOUND','false'] but config_dict.get('SOQT_FOUND','false')

J-Dunn commented 5 years ago

ah, my bad. Subtle but important difference. Thanks. In any case I have just run the unmodified code and it completed. The failure at that line seems to induced by something else. But it's probably more robust done that way.

The return of the search for 'qmake' that I flagged probably needs a test too, to ensure it is not empty null object.

J-Dunn commented 5 years ago

This is what I found was needed to fix the "cstddef" errors. Pretty simple.

cd pivy-master/fake_headers
touch cstddef cstdarg cassert