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

Version info in ogg.h #8

Closed
stopiccot opened this issue Jun 7, 2015 · 12 comments
Closed

Version info in ogg.h #8

stopiccot opened this issue Jun 7, 2015 · 12 comments

Comments

@stopiccot
Copy link
Contributor

Many other projects (zlib, png, SDL2, ...) embed version info in header file. I think ogg could also benefit from that. Yes, I'm aware of version info in pc files but that's not an option on Windows.

@erikd
Copy link
Member

erikd commented Jun 7, 2015

I don't think its a good idea to have it in the header file, mainly because the header and the actual lib can be out-of-sync with each other. Instead, I would suggest a function like:

const char * ogg_version_str (void);

@stopiccot
Copy link
Contributor Author

Actually never liked this approach. In my opinion if you have not corresponding versions of library and header it's already something really wrong. That way you can't trust version in pc file also.

@stopiccot
Copy link
Contributor Author

Somebody else's thoughts on this?

@rillian
Copy link
Contributor

rillian commented Jun 11, 2015

...if you have not corresponding versions of library and header it's already something really wrong. That way you can't trust version in pc file also.

I don't see why the header is any more or less trustworthy than the pc file?

Somebody else's thoughts on this?

I tend to agree with erikd, but I don't think either make sense for libogg. It's quite mature, and unlike codecs is unlikely to have per-release bugs an embedded version string would help track down in the output. What benefit were you hoping to get by adding version information to the header?

If you want make sure the library and header are in sync you need both. For example, libpng does this. In practice no one checks both and compares, so I think it's not worth adding the api surface.

@stopiccot
Copy link
Contributor Author

I don't see why the header is any more or less trustworthy than the pc file?

A little missunderstanding here. I'm not saying that pc file is not trustworthy. I'm saying that pc file cannot be used on Windows and OS X. So it would be great to have version info both in header and pc files.

What benefit were you hoping to get by adding version information to the header?

I just like to see library versions while configuring projects like:

-- Found ZLIB: /usr/lib/libz.dylib (found version "1.2.5") 

@tdaede
Copy link

tdaede commented Jun 15, 2015

It looks like FindZLib just does a regexp on the .h to find the version.

Source packages of ogg have a configure script, and devel versions have a configure.ac file. This script has a PACKAGE_VERSION line, as well as several OGG_LIB_* lines. Couldn't you parse that?

@stopiccot
Copy link
Contributor Author

@tdaede And now imagine someone configuring ogg-dependent project on Windows. In many cases they will use prebuilt binaries pacakges that contain lib and h files only. Or prebuilt framework on OS X. Or even installed devel package from apt-get on Debian. In all this cases you obviously can't parse configure script. Header file seems the only common denominator across all platforms for me.

@tdaede
Copy link

tdaede commented Jun 17, 2015

On Linux (and OS X when using homebrew), the .pc file will be installed and should always be the mechanism used.

We don't distribute official packages for Windows, and until recently most users of libogg were building it themselves in their own projects. So these prebuilt binary packages don't exist yet. Presumably we can also define what goes in them.

Putting it in a .h file would be okay, but it is slightly weird as it is intended for the build system, not programs using libogg. Does windows have a native version of .pc, maybe provided by OneGet/NuGet? We could also bundle a VERSION file in the binary packages or something.

@stopiccot
Copy link
Contributor Author

OS X when using homebrew

While homebrew is awesome for using on developer machines you can't actually link against libraries from homebrew in apps you plan to distribute. You simply can't ask users to install homebrew and run brew install ogg for your app to work. And many OS X developers tend to use frameworks for their dependencies.

until recently most users of libogg were building it themselves in their own projects

And many of them still do. And most of them build ogg once and than copy header and library to some kind of "prebuilt dependencies" folder.

@stopiccot
Copy link
Contributor Author

Btw maybe we can simply add version to THIS FILE IS PART OF THE.. disclaimer. And a simple configuration step to keep it updated alongside with copyright year

@rillian
Copy link
Contributor

rillian commented Jun 18, 2015

I don't think we're convinced the benefits here outweigh the drawbacks. Closing as wontfix.

@rillian rillian closed this as completed Jun 18, 2015
@tdaede
Copy link

tdaede commented Jun 18, 2015

So, I asked on #cmake:

<TD-Linux> is there a standard way to specify a library's version to cmake? like pkg-config, but on windows
<TD-Linux> some projects specify a version in their headers, which is parsed by some cmake regexp, but that is hardly a standard pattern
<ngladitz> the equivalent to pkg-config in cmake would be package configuration files which may also come with package version files
<TD-Linux> ngladitz, ah, I see, thanks! this will also work even if the project wasn't built with cmake, correct?
<ngladitz> yes e.g. Qt5 provides package configuration files but isn't build with cmake

Does this seem like a good solution? If so, you can reopen this issue or make a new one about implementing this.

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

No branches or pull requests

4 participants