Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

depqbf 5.01 #4655

Closed
wants to merge 1 commit into from
Closed

depqbf 5.01 #4655

wants to merge 1 commit into from

Conversation

ilovezfs
Copy link
Contributor

@ilovezfs ilovezfs commented Sep 9, 2016

Created with brew bump-formula-pr.

@BrewTestBot BrewTestBot added the in progress Stale bot should stay away label Sep 9, 2016
@ilovezfs ilovezfs added do not merge upstream issue An upstream issue report is needed labels Sep 9, 2016
@ilovezfs
Copy link
Contributor Author

ilovezfs commented Sep 9, 2016

iMac-TMP:~ joe$ otool -L /usr/local/Cellar/depqbf/5.01/lib/libqdpll.1.0.dylib
/usr/local/Cellar/depqbf/5.01/lib/libqdpll.1.0.dylib:
    /usr/local/opt/depqbf/lib/libqdpll.so.1 (compatibility version 0.0.0, current version 0.0.0)
    /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1226.10.1)

@ilovezfs
Copy link
Contributor Author

@UniqMartin I'm surprised this doesn't cause some kind of audit failure ...

@UniqMartin
Copy link
Contributor

I guess it's too infrequent of an occurrence to have an audit check for this. Most projects rely on their build system to figure out an appropriate extension for a shared object on the respective platform. Another thing is that macOS uses both .dylib and .so as an extension. The former is for things to be recorded in the dependencies of an executable and more often seen, but the latter is the preferred extension for things loaded dynamically at run time (e.g. via dlopen) unless they are bundled with other resources as a .bundle. Not sure if this addresses your comment …

@ilovezfs
Copy link
Contributor Author

@UniqMartin right, sorry for not being more explicit. I'm referring specifically to the fact that a file with the install_name doesn't exist.

/usr/local/Cellar/depqbf/5.01/lib/libqdpll.1.0.dylib:
    /usr/local/opt/depqbf/lib/libqdpll.so.1 

Upstream changed the build for us so that it produces a file with .dylib not .so, but otool -L still shows /usr/local/opt/depqbf/lib/libqdpll.so.1 for the install_name (since the part of the build that sets the install_name wasn't also changed correspondingly), which just doesn't exist. Yet for some reason brew audit doesn't seem to notice there's no such file.

@UniqMartin
Copy link
Contributor

Sorry for being slow to process the problem. I finally understood what the issue is you've been trying to point out. IMO, that's indeed a check that would make sense. We currently only check for the existence of the linked libraries, but don't check the sanity of the install name of the inspected dylib.

The underlying problem should be fixed upstream by modifying these lines of the makefile to adjust what gets passed to -Wl,$(SONAME), and to make the build rule directly produce the dylib file instead of copying from the .so.$(VERSION) file after the fact. Of course the formula will also need to setup a symbolic link from libqdpll.$(MAJOR).dylib to libqdpll.$(VERSION).dylib as the makefile sadly doesn't do this, too. The changes should be fairly straightforward, but I guess I can come up with a patch if need be.

@ilovezfs
Copy link
Contributor Author

The changes should be fairly straightforward, but I guess I can come up with a patch if need be.

No, you shouldn't need to fix this, but I did want to make sure you were aware that bogus install_name values are skating right passed audit :)

@UniqMartin
Copy link
Contributor

No, you shouldn't need to fix this, but I did want to make sure you were aware that bogus install_name values are skating right passed audit :)

Thanks! I'll add it to my ever growing TODO list, but I can't promise when I'll find the time to tackle this. Unfortunately, I'm still catching up with what has been going on in the past 2-3 weeks (and a lot has happened!) and I have to deal with my already existing but neglected PRs before I can start creating new ones for relatively minor stuff like this. 🙈

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
in progress Stale bot should stay away upstream issue An upstream issue report is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants