Skip to content

Conversation

@altendky
Copy link
Contributor

@altendky altendky commented Mar 25, 2021

Draft for:

The first line of this output caused the exception below.

$ ldd qt/6.0.2/gcc_64/plugins/sqldrivers/libqsqlpsql.so
qt/6.0.2/gcc_64/plugins/sqldrivers/libqsqlpsql.so: /lib/x86_64-linux-gnu/libpq.so.5: no version information available (required by qt/6.0.2/gcc_64/plugins/sqldrivers/libqsqlpsql.so)
        linux-vdso.so.1 (0x00007fffd2f0d000)
        libpq.so.5 => /lib/x86_64-linux-gnu/libpq.so.5 (0x00007ff5a9a5a000)
        libQt6Sql.so.6 => /home/altendky/repos/qt-applications/qt/6.0.2/gcc_64/plugins/sqldrivers/../../lib/libQt6Sql.so.6 (0x00007ff5a980b000)
        libQt6Core.so.6 => /home/altendky/repos/qt-applications/qt/6.0.2/gcc_64/plugins/sqldrivers/../../lib/libQt6Core.so.6 (0x00007ff5a9001000)
        libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007ff5a8fde000)
        libstdc++.so.6 => /lib/x86_64-linux-gnu/libstdc++.so.6 (0x00007ff5a8dfd000)

<snip>

Long winded traceback redacted by removing piles of wheel-building layers.

  Traceback (most recent call last):                                                                                                                                                                                              

<snip>

    File "./build.py", line 218, in linuxdeployqt_substitute_list_source
      for dependency in lddwrap.list_dependencies(
    File "/tmp/pip-build-env-ueu0a2p1/overlay/lib/python3.9/site-packages/icontract/_checkers.py", line 648, in wrapper
      result = func(*args, **kwargs)
    File "/tmp/pip-build-env-ueu0a2p1/overlay/lib/python3.9/site-packages/lddwrap/__init__.py", line 220, in list_dependencies
      dependencies = _cmd_output_parser(cmd_out=out)  # type: List[Dependency]
    File "/tmp/pip-build-env-ueu0a2p1/overlay/lib/python3.9/site-packages/lddwrap/__init__.py", line 259, in _cmd_output_parser
      dep = _parse_line(line=line)
    File "/tmp/pip-build-env-ueu0a2p1/overlay/lib/python3.9/site-packages/lddwrap/__init__.py", line 165, in _parse_line
      raise RuntimeError(
  RuntimeError: Expected 2 parts in the line but found 9: /tmp/pip-req-build-lyab9hdl/build/qt_applications-t5l6p3ia/qt/6.0.2/gcc_64/plugins/sqldrivers/libqsqlpsql.so: /lib/x86_64-linux-gnu/libpq.so.5: no version information available (required by /tmp/pip-req-build-lyab9hdl/build/qt_applications-t5l6p3ia/qt/6.0.2/gcc_64/plugins/sqldrivers/libqsqlpsql.so)

@altendky
Copy link
Contributor Author

I am not particularly knowledgeable about ldd. This submission is based on 'it works in my case' (altendky/qt-applications#11). I understand that there may be other important considerations.

@mristin
Copy link
Collaborator

mristin commented Mar 25, 2021

Hi @altendky ,
Thanks for reporting the issue and sending the code. Let me have a better look at it tomorrow. I'd like to add a test or two to your pull request.

@altendky
Copy link
Contributor Author

Sorry, I meant to swing back around and look at the test setup and add something. Got distracted... Anyways, note added to the OP and I'll look at that tomorrow maybe.

Thanks for your consideration.

@mristin
Copy link
Collaborator

mristin commented Mar 26, 2021

@altendky could you please grant me permissions to add commits to your pull request? (Please see https://docs.github.com/en/github/collaborating-with-issues-and-pull-requests/committing-changes-to-a-pull-request-branch-created-from-a-fork)

If you want to look into the test cases yourself, this is of course even better :-).

@altendky
Copy link
Contributor Author

I don't know if I changed from default or what, but edits are allowed by default on PRs I create.

image

And yes, I planned on including tests. I'll take a stab and see if they are up to snuff. :]

@altendky altendky mentioned this pull request Mar 26, 2021
2 tasks
@altendky altendky marked this pull request as ready for review May 7, 2021 17:44
@altendky
Copy link
Contributor Author

altendky commented May 7, 2021

@mristin, alrighty, now the _real_ PR is ready for consideration... Of course, the follow up is: what is the release schedule here? Not nagging, I appreciate all the reviews. Just looking to understand so I can plan my path forward accordingly.

Cheers, thanks for all the work.

Copy link
Collaborator

@mristin mristin left a comment

Choose a reason for hiding this comment

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

Please see the comments. We need to figure out a more precise way to determine whether a dependency is missing or not.

soname=soname, path=dep_path, found=found, mem_address=mem_address)
else:
if len(parts) != 2:
if line[0] not in {' ', '\t'}:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm very worried that this line might introduce a lot of hard-to-track bugs. I suppose you want to cover the ldd output from #14:

qt/6.0.2/gcc_64/plugins/sqldrivers/libqsqlpsql.so: /lib/x86_64-linux-gnu/libpq.so.5: no version information available (required by qt/6.0.2/gcc_64/plugins/sqldrivers/libqsqlpsql.so)

?

In my opinion, missing leading indention is not an indication enough for missing dependencies.

Why not check for no version information available similar how I check for not found at line 126?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bah, my GitHub inbox is getting trashed by another project. Glad I checked back... Yeah, I don't know my way around what the ldd 'format' really is. It would be nice to have some 'external' indicator that wouldn't be part of a file name, but I guess in this case we kind of have to just hope that the library file name doesn't contain weird phrases like we will check for. At least odds are decent that people won't build libraries named no version information available. :]

Change incoming. Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alrighty, I'm good with this but also remembered a bit along the way. I guess the 'why not', without claiming it is sufficient to be 'right', is that entering messages one at a time instead of catching all 'message' lines vs. 'dependency' lines generically ends up making more changes. But sure, this is a undefined formatting and we are (I am?) just extrapolating guesses as to what works out best. Anyways, it's obviously not that hard to add ignores.

If you are good with my adjustment per your request, then I'm almost good with this. I'll follow up with one final CI run of my project using this (qt-applications) to make sure it still works as desired in the field.

@mristin
Copy link
Collaborator

mristin commented May 9, 2021

Of course, the follow up is: what is the release schedule here?

@altendky I can release a patch version as soon as we get this pull request in.

@altendky altendky mentioned this pull request May 11, 2021
7 tasks
Copy link
Collaborator

@mristin mristin left a comment

Choose a reason for hiding this comment

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

Please see the minor remarks.

Copy link
Collaborator

@mristin mristin left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM!

@mristin
Copy link
Collaborator

mristin commented May 11, 2021

@altendky ready for merge?

@altendky
Copy link
Contributor Author

@mristin, did one last test build, go for it. Cheers. :]

@mristin mristin merged commit d0d93e2 into Parquery:master May 11, 2021
@mristin
Copy link
Collaborator

mristin commented May 11, 2021

@altendky thanks a lot!

@altendky altendky deleted the ignore_messages branch May 11, 2021 21:33
mristin added a commit that referenced this pull request May 12, 2021
* Ignore not-indented ldd output (#14)
* Added handling of static libraries (#13)
@mristin mristin mentioned this pull request May 12, 2021
mristin added a commit that referenced this pull request May 12, 2021
* Ignore not-indented ldd output (#14)
* Added handling of static libraries (#13)
@mristin
Copy link
Collaborator

mristin commented May 12, 2021

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants