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.

Sign up for GitHub

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

Jump to bottom

Lib versioning #92

Merged
merged 10 commits into from
Oct 14, 2022
Merged

Lib versioning #92

merged 10 commits into from
Oct 14, 2022

Conversation

wentasah
Copy link
Contributor

Hi @darrylb123,

I've created a draft pull request from your lib-versioning branch to make it easier to discuss about the code. Treat this as an answer to your email :-)

I'll add comments to the code in a minute.

wentasah
wentasah commented Sep 19, 2022
View reviewed changes
Copy link
Contributor Author

@wentasah wentasah left a comment

Choose a reason for hiding this comment

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

Here are my comments.

Makefile Outdated
rm -f gitversion.h
$(MAKE) -C usbrelay_py clean


install: usbrelay libusbrelay.so
install -d $(DESTDIR)$(LIBDIR)
install -m 0755 libusbrelay.so $(DESTDIR)$(LIBDIR)
install -m 0755 libusbrelay.so.$(USBLIBVER) $(DESTDIR)$(LIBDIR)
( cd $(DESTDIR)$(LIBDIR); ln -s libusbrelay.so.$(USBLIBVER) libusbrelay.so ; ln -s libusbrelay.so.$(USBLIBVER) libusbrelay.so.$(USBMAJOR) )
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As you wrote in the email, these symlinks are usually created by ldconfig. I think it's OK to do it this way, but maybe, the following would be simpler:

Suggested change
( cd $(DESTDIR)$(LIBDIR); ln -s libusbrelay.so.$(USBLIBVER) libusbrelay.so ; ln -s libusbrelay.so.$(USBLIBVER) libusbrelay.so.$(USBMAJOR) )
$(LDCONFIG) -n $(DESTDIR)$(LIBDIR)

At the top of the makefile, you would add something like:

LDCONFIG = /sbin/ldconfig

This works on Debian. If that works also on Fedora, we can leave it like this. Otherwise, we may add some logic to autodetect ldconfig location.

This also works, if the link already exists. ln -s fails, but if you want to stay with ln, we should change this to ln -sf.

usbrelay_py/setup.py Outdated
libraries= ['usbrelay'],
library_dirs= ['./','/usr/lib','/usr/lib64','/usr/lib/x86_64-linux-gnu','/usr/lib/aarch64-linux-gnu','/usr/lib/arm-linux-gnueabihf','..'],
libraries= [':libusbrelay.so.' + str(USBMAJOR)],
library_dirs= ['./','/usr/lib','/usr/lib64','/usr/lib/x86_64-linux-gnu','/usr/lib/aarch64-linux-gnu','/usr/lib/arm-linux-gnueabihf','../'],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think most of those libraries are not necessary. This is the reason why your build tried to link against wrong library.

Suggested change
library_dirs= ['./','/usr/lib','/usr/lib64','/usr/lib/x86_64-linux-gnu','/usr/lib/aarch64-linux-gnu','/usr/lib/arm-linux-gnueabihf','../'],
library_dirs= ['../'],

Makefile Outdated
@@ -24,10 +25,11 @@ all: usbrelay libusbrelay.so


libusbrelay.so: libusbrelay.c libusbrelay.h
$(CC) -shared -fPIC $(CPPFLAGS) $(CFLAGS) $< $(LDFLAGS) -o $@ $(LDLIBS)
$(CC) -shared -fPIC -Wl,-soname,$@.$(USBMAJOR) $(CPPFLAGS) $(CFLAGS) $< $(LDFLAGS) -o $@.$(USBLIBVER) $(LDLIBS)
ln -s libusbrelay.so.$(USBLIBVER) libusbrelay.so.$(USBMAJOR)
Copy link
Contributor Author

@wentasah wentasah Sep 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'd use: $(LDCONFIG) -n . (see above).

@darrylb123
Copy link
Owner

darrylb123 commented Sep 20, 2022 via email

@darrylb123
Copy link
Owner

I've incorparated your suggestions into the lib_versioning branch.
I have also been getting the fedora devel package to work.
The Fedora devel package has the libusbrelay.so link and libusbrelay.h
I use ldconfig to make the soname link and it all seems to work correctly and builds correct fedora packages too.

wentasah
wentasah commented Oct 3, 2022
View reviewed changes
Copy link
Contributor Author

@wentasah wentasah left a comment

Choose a reason for hiding this comment

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

I think it looks quite OK, but see a few comments below.

Makefile

install -m 0755 libusbrelay.so.$(USBLIBVER) $(DESTDIR)$(LIBDIR)
$(LDCONFIG) -n $(DESTDIR)$(LIBDIR)
( cd $(DESTDIR)$(LIBDIR) ;ln -sr libusbrelay.so.$(USBLIBVER) libusbrelay.so )
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure this is a good idea. It would be useful if we want to use old usbrelay binary with a new libusbrelay, but this can cause problems in the future. I'd simply drop this line.

usbrelay_py/setup.py Outdated

module1 = Extension(
'usbrelay_py',
libraries= ['usbrelay'],
library_dirs= ['./','/usr/lib','/usr/lib64','/usr/lib/x86_64-linux-gnu','/usr/lib/aarch64-linux-gnu','/usr/lib/arm-linux-gnueabihf','..'],
libraries= [':libusbrelay.so.' + str(USBMAJOR)],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What does the colon mean? I cannot find it documented here.

rpm/usbrelay.spec

%files devel
%{_includedir}/libusbrelay.h
%{_libdir}/libusbrelay.so
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think devel packages should contain any .so files, even if they are just symlinks.

@darrylb123
Copy link
Owner

darrylb123 commented Oct 3, 2022 via email

@darrylb123
Copy link
Owner

darrylb123 commented Oct 3, 2022 via email

@wentasah
Copy link
Contributor Author

wentasah commented Oct 4, 2022 via email

@darrylb123
Copy link
Owner

darrylb123 commented Oct 4, 2022 via email

@wentasah
Copy link
Contributor Author

wentasah commented Oct 4, 2022

If you just do that, the python package requires the .so and therefore breaks library versioning. Been there, done that :)

Really? I tried that and it works well. When I run:

cd usbrelay_py
python3 setup.py build -v

I see:

gcc -shared [-L...] build/temp.linux-x86_64-cpython-310/src/libusbrelay_py.o -L../ -lusbrelay -o build/lib.linux-x86_64-cpython-310/usbrelay_py.cpython-310-x86_64-linux-gnu.so

Note that I removed unimportant -L flags for brevity. You can see that it links the usbrelay library with -lusbrelay. Then, when I check, which library is actually needed by the produced Python extension, I see the correct version:

$ objdump -x build/lib.linux-x86_64-cpython-310/usbrelay_py.cpython-310-x86_64-linux-gnu.so | grep NEEDED
  NEEDED               libusbrelay.so.1
  NEEDED               libc.so.6

I suggest when we get to try to do a windows port we may need a different makefile or some other tricks. I had thought we would still use the same tool chain on windows so it mightn't matter.

We may try using the same compiler, but my preliminary research (via Google) suggests that it's often problematic. Even the official documentation (https://docs.python.org/3/extending/windows.html?highlight=msvc#using-dlls-in-practice) says "Windows Python is built in Microsoft Visual C++; using other compilers may or may not work."

@darrylb123
Copy link
Owner

darrylb123 commented Oct 4, 2022 via email

@darrylb123
Copy link
Owner

WIthout the so link it will not build
gcc -shared -Wl,-z,relro -Wl,--as-needed -Wl,-z,now -Wl,--build-id=sha1 -g -Wl,-z,relro -Wl,--as-needed -Wl,-z,now -Wl,--build-id=sha1 -g build/temp.linux-x86_64-cpython-310/src/libusbrelay_py.o -L../ -L/usr/lib64 -lusbrelay -o build/lib.linux-x86_64-cpython-310/usbrelay_py.cpython-310-x86_64-linux-gnu.so
/usr/bin/ld: cannot find -lusbrelay

With the link it works properly
I've added making the so link in the makefile and fixed setup.py as you suggested.

I assume that adding the soname field to the library properly is what resolved the problem I had last time I tried.

@darrylb123
Copy link
Owner

Michal,
Mark has successfully built the libversioning branch for Fedora. I will merge it and create a release when you're happy with it.
Mark will use the release to submit for review.

@wentasah
Copy link
Contributor Author

I'm currently traveling without access to hardware and much time for testing, but I think that the current state of this branch should work well.

@wentasah wentasah marked this pull request as ready for review October 13, 2022 09:03
@darrylb123
Copy link
Owner

darrylb123 commented Oct 13, 2022 via email

@darrylb123 darrylb123 merged commit 199c0ec into master Oct 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@wentasah @darrylb123