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

[libut]support shared build and more #84

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

marguerite
Copy link

  • tweak Makefile to build static and shared version of libut
  • support build with RPM_OPT_FLAGS
  • add a pkgconfig file for libut.
  • install headers into /usr/include/uthash instead of /usr/include, which is better.

These commits were tested on openSUSE.

all: $(OBJS)
SHELL=/usr/sh
VERSION=2.0.1
MAJOR=2
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: I'd prefer to see these variables inlined into the places where they're currently being used, to keep the code as straightforward as possible. I infer from context that MAJOR is always supposed to be the major version, e.g. if VERSION became 3.0.0 then MAJOR should become 3, but I don't really understand why. This would probably be easier to understand if half of the logic weren't located 30 lines away.

Copy link
Author

Choose a reason for hiding this comment

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

You mean...to use "2.0.1" directly instead of $(VERSION)? then 2.0.1 and 2 will become magic numbers, you'll have to change all of them in this Makefile at version updates. Or you just want to move them down to the place where they are about to be used right away?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, make them magic numbers. Ideally, when it's time to bump the version, I want to be able to git grep 2.0.1 and find exactly all the places that need adjustment; and when it's not time to bump the version, I want the code to be simple and straightforward, with as few named entities and as little cognitive overhead as possible.

Copy link
Author

Choose a reason for hiding this comment

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

ok.

@marguerite
Copy link
Author

@Quuxplusone

Hi, I've reseted my branch and made everything desirable. If you can make a final decision on -fPIC inclusion, we'll have this PR mergable.

Marguerite

@besser82
Copy link

@marguerite

Well, I've made these changes as well in my current Fedora-specfile. The only thing, which I can see on your changes, that is a bad idea, is the fact you are using the "regular" version of uthash for the so-versioning.

According to the manuals to linux/gnu LD, the so-versioning works differently. libut.so.2.0.1 means in versioning terms of so-libs, that it is recently version 2, the latest compatibility is so-version 0 (which makes it incompatible with so-version 1) and the earliest compatibility version is so-version 1 (contradiction to so-version 0).

See Libtool's versioning scheme for further details on how so-libs are versioned correctly.

@Quuxplusone
Copy link
Collaborator

FWIW, my opinions on the subject are still

  • The right approach is simply not to package the non-header-only bits
  • Debian seems to be doing it "right" (last time I checked anyway)
  • OpenSUSE and Fedora are welcome to try out a different approach, via patches applied "downstream" from here
  • I'd like to watch this different approach and see whether any OpenSUSE/Fedora packages start acquiring dependencies on the non-header-only bits (as opposed to sticking with the Debian-packaged, header-only bits such as "uthash.h"). If people try and fail, that might uncover issues with the current patch; and if people never try, then the current patch might be unnecessary (in that Debian-style header-only packaging would meet everyone's needs just as well).

@marguerite
Copy link
Author

@besser82 Thanks! I'll change it to 2.0.1 as yours.

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.

3 participants