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

Foreign library versioning #4074

Merged
merged 8 commits into from
Nov 26, 2016
Merged

Foreign library versioning #4074

merged 8 commits into from
Nov 26, 2016

Conversation

abooij
Copy link
Contributor

@abooij abooij commented Nov 1, 2016

See #4052

@mention-bot
Copy link

@abooij, thanks for your PR! By analyzing the history of the files in this pull request, we identified @edsko, @dcoutts and @phadej to be potential reviewers.

@abooij abooij force-pushed the foreign-soname branch 12 times, most recently from 5841300 to 2fad4d2 Compare November 4, 2016 13:58
@abooij
Copy link
Contributor Author

abooij commented Nov 4, 2016

@ezyang The code is now ready for review. What's missing is documentation: I'll write something in Cabal/doc/developing-packages.rst. Anywhere else? Is the code documented clearly?

After review I can clean up the history a little bit. Or just make it one big commit?

@ezyang
Copy link
Contributor

ezyang commented Nov 5, 2016

You're not done until there's documentation ;) I think something nestled with the existing foreign library docs would make sense.

Copy link
Contributor

@ezyang ezyang left a comment

Choose a reason for hiding this comment

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

After having finally read up on how libtool versioning works, I'm a bit torn about adopting it wholesale for Cabal.

The good thing is that it is "standard".

The bad things... well, it's hella confusing. Here's a quip from https://autotools.io/libtool/version.html

To set the version of the library, libtool provides the -version-info parameter, which accepts three numbers, separated by colons, that are called respectively, current, revision and age. Both their name and their behaviour, nowadays, have to be considered fully arbitrary, as the explanation provided in the official documentation is confusing to say the least, and can be, in some cases, considered completely wrong.

It continues:

The main reason for which libtool uses this scheme for version information, is that it allows to have multiple version of a given library installed, with both link editor and dynamic linker choosing the latest available at build and run time. With modern distributions packaging standards, this situation should not be happening anyway.

TBH, I'm not even sure how libtool style version numbers help in this case.

More fun:

So.... I am seriously tempted to ditch libtool numbers, and just use the good, old, simple, "here is the version number that will get put in the SONAME", and tell users to update the major if they break ABI-compatibility, and minor if they add a new interface. (Honestly, we should probably have an option to compute the patchlevel automatically from the Cabal version.) As a comparison, CMake parses out major.minor from the soname and uses that on Windows: https://cmake.org/cmake/help/cmake2.6docs.html#prop_tgt:SOVERSION

But I am open to being convinced.

when (isJust (foreignLibELFVersion flib)) $ do
let (Platform _ os) = hostPlatform lbi
when (os /= Linux) $ die
"Can't install foreign-library symlink on non-Linux OS"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it true that it should be impossible to get here (the checks during configure should catch it?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I can add a comment to that effect.

#ifndef mingw32_HOST_OS
-- createSymbolicLink file1 file2 creates a symbolic link
-- named file2 which points to the file file1.
createSymbolicLink name (dstDir </> flibBuildName lbi flib)
Copy link
Contributor

Choose a reason for hiding this comment

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

NB: it's name, not dst, because the symlink will be relative to the directory it's created in, i.e. distDir. Tricky!

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 will add a comment to that effect.

(Linux, ForeignLibNativeShared)
= case foreignLibELFVersion flib of
Nothing -> "lib" ++ nm <.> "so"
Just v -> "lib" ++ nm <.> "so" <.> show (head (versionNumbers v))
Copy link
Contributor

Choose a reason for hiding this comment

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

I am very glad you got rid of this 'head' subsequently ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While I agree, it couldn't have resulted in errors, since a user can only set v to a non-null version.

@@ -42,16 +53,81 @@ data ForeignLib = ForeignLib {
}
deriving (Generic, Show, Read, Eq, Typeable, Data)

data LibVersionInfo = LibVersionInfo Int Int Int deriving (Data, Eq, Generic, Typeable)
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs a comment (esp https://www.gnu.org/software/libtool/manual/html_node/Libtool-versioning.html). And although in the standard format the parameters are positional, it would be far better if we had some accessors here to make it clear.)

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 will refer to libtool more clearly here.
I can also add these accessors. There is also libVersionInfoCRA which gives you a tuple of the three numbers, but I can see how that is not sufficient and/or clear enough.

@@ -878,20 +878,20 @@ flibTargetName lbi flib =
platformOS :: Platform -> OS
platformOS (Platform _arch os) = os

-- If a foreign lib foo has elf-version 3.2.1, it should be built
-- If a foreign lib foo has lib-version-info 5:1:2, it should be built
-- as libfoo.so.3.2.1
Copy link
Contributor

Choose a reason for hiding this comment

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

(This comment is pretty confusing if you're not fluent in libtool's versioning scheme)

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 will add [an appropriate reference to] an explanation.

@abooij
Copy link
Contributor Author

abooij commented Nov 5, 2016

@ezyang Thanks for your review.

So.... I am seriously tempted to ditch libtool numbers, and just use the good, old, simple, "here is the version number that will get put in the SONAME", and tell users to update the major if they break ABI-compatibility, and minor if they add a new interface. [...] But I am open to being convinced.

Well, the advantage of version-info over version numbers is that it is more likely we will be able to parse version-info into the data required for non-Linux OSes. For example, we can't reliably deduce an age from major.minor.build (as people will just be randomly bumping version numbers). But it is important that we know both "current" and "current-age" on OSX. So while I agree with your worries about version-info, if there is any potential future wish for OSX support, this is the more general solution.

I can add documentation on how to set these values, if that'd help (basically repeating the three golden rules of version-info).

Honestly, we should probably have an option to compute the patchlevel automatically from the Cabal version.

No! ABI versions are NOT equal to package versions, and this is already an important distinction made in C. This does cause problems. And besides, in my use case this is absolutely undesirable.

@ezyang
Copy link
Contributor

ezyang commented Nov 5, 2016

No! ABI versions are NOT equal to package versions, and this is already an important distinction made in C. This does cause problems. And besides, in my use case this is absolutely undesirable.

Even the revision level? According to the libtool guidance, you are supposed to bump revision whenever there is a source code change. I guess, maybe not if the C library didn't change (even though the hs library did)?

@abooij
Copy link
Contributor Author

abooij commented Nov 5, 2016

(Also as discussed on IRC:)
For example: there's no reason why the ABI changes with every release of the package. So then there's no reason to even bump the "revision" field. And one might want to make a major change to the haskell code without modifying the ABI (in which case you would update the package version but not the ABI version), or vice versa (in which case you would increase the major version of the ABI, but only make an incremental version bump for the Haskell package). ABI versions are, to some extent, independent of release versions.

We need to build the library under a different name to set the SONAME,
and can't set it directly, since GHC overrides -soname linker options.
When a foreign-library has a version, during installation, we need to
install appropriate symlinks. This is very platform-dependent.
Libtool sets library version data by the -version-info flag, which takes
current[:revision[:age]] data. This is then parsed into a
major.minor.build version number. We copy this approach so that library
versioning may be generalised to other operating systems than Linux.
@ezyang
Copy link
Contributor

ezyang commented Nov 26, 2016

This looks reasonable.

@abooij
Copy link
Contributor Author

abooij commented Nov 26, 2016

I wrote some documentation. So this work now seems ready. Final feedback welcome.

@abooij abooij changed the title [WIP] Foreign library versioning Foreign library versioning Nov 26, 2016
@ezyang
Copy link
Contributor

ezyang commented Nov 26, 2016

Ship it!

@abooij abooij merged commit 8744e30 into haskell:master Nov 26, 2016
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.

None yet

3 participants