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

Add version header #110

Open
evpobr opened this issue Jan 10, 2019 · 14 comments
Open

Add version header #110

evpobr opened this issue Jan 10, 2019 · 14 comments

Comments

@evpobr
Copy link
Contributor

evpobr commented Jan 10, 2019

The only way to detect installed Opus version without pkg-config (Windows platform) is parsing some version header and reading values from defines, e.g OPUS_VERSION_MAJOR, OPUS_VERSION_MAJOR etc.

So, can opus_version.h public header be added to libopus?

@jmvalin
Copy link
Member

jmvalin commented Jan 10, 2019

The real question is why do you need the version at compile time in the first place?

@evpobr
Copy link
Contributor Author

evpobr commented Jan 10, 2019

I need it to find opus package with CMake and retrieve its version. Find module uses pkg-config when possible. But when it is missing (Windows), the common way is to parse public header with version defines (if provided). Bundled with CMake FindZLIB module uses this approach, and many other find modules.

If Opus will ever have CMake support, all information could be retrieved from installed CMake package module on any platform. But CMake support is still missing.

@jmvalin
Copy link
Member

jmvalin commented Jan 10, 2019

There's someone working on CMake support. That being said, the idea is that getting the version from the header should not be needed. Instead, it's best to query actual features. For example, if you want to know if you can disable phase inversion, you can use #ifdef OPUS_SET_PHASE_INVERSION_DISABLED rather than attempt to see in what version that got added.

Note that in some cases, the header version may differ from the run-time version (especially with dynamic libraries), so the run-time version is available through an API call.

@evpobr
Copy link
Contributor Author

evpobr commented Jan 10, 2019

There's someone working on CMake support. That being said, the idea is that getting the version from the header should not be needed.

I'm not sure we will see CMake support soon. As i see development is stopped.

it's best to query actual features. For example, if you want to know if you can disable phase inversion, you can use #ifdef OPUS_SET_PHASE_INVERSION_DISABLED rather than attempt to see in what version that got added.

It is better, indeed. But pkg-config uses version, and CMake just follows the same way. Maybe it is not optimal, but it simple and it works.

Also, our test results differs depending on libopus version. Don't know what feature it is, but we know the version.

Note that in some cases, the header version may differ from the run-time version (especially with dynamic libraries), so the run-time version is available through an API call.

We tried. Vcpkg build of Opus retrieves "unknown" version. I guess it is because Vcpkg downloads sources from Opus repo as tar.gz, not clones it. So .git directory is missing and genversion.bat script fails.

There is fallback in genversion.bat to use package_version file if found. But it fails again. version.h is generated, but PACKAGE_VERSION is set to "unknown".

@jmvalin
Copy link
Member

jmvalin commented Jan 11, 2019

In release tarballs, the version is set without any script, so it shouldn't be a problem. When cloning, the version scripts should work. If someone makes a tar.gz of the current master without any of the version info, then there's nothing we can do about it.

@evpobr
Copy link
Contributor Author

evpobr commented Jan 12, 2019

If someone makes a tar.gz of the current master without any of the version info, then there's nothing we can do about it.

So, if we dowload tar.gz from GitHub Releases page and we have "unknown" version reported by library built with Visual Studio it is OK?

@evpobr
Copy link
Contributor Author

evpobr commented Jan 12, 2019

But idea about download tarball is good... It has package_version and works! Now i know how to fix Vcpkg opus port 😄

@evpobr
Copy link
Contributor Author

evpobr commented Jan 12, 2019

About version header - if you add it, many CMake developers will thank you. You can make it standalone, do not include in any other public header. No harm to anything IMHO.

Why? Because even if CMake project is megred, it doesn't mean every maintrainer will use CMake build to create Opus packages. So cmake package script will not be installed.

@jmvalin
Copy link
Member

jmvalin commented Jan 12, 2019

You don't understand, the contents of the git repo does not include the version number anywhere. The version is always extracted from the git tags (and commits), which is the only way it can be accurate. Otherwise it's guaranteed to be messed up, out of sync, ... (we've been there).

@evpobr
Copy link
Contributor Author

evpobr commented Jan 12, 2019

The version is always extracted from the git tags (and commits), which is the only way it can be accurate.

I understand. That's why Vcpkg build failed: no .git and no package_version.

Otherwise it's guaranteed to be messed up, out of sync, ... (we've been there).

Ok, it looks reasonable. And still no problem with version header.

Keep template opus_version.h.in in repo. Create opus_version.h and fill values at configuration stage. Add opus_version.h to .gitignore to be sure it is never commited.

@jmvalin
Copy link
Member

jmvalin commented Jan 15, 2019

OK, back to the start. It seems like I don't actually understand your problem. What is it that a header version would solve? Especially considering that Opus 1.0 through 1.3 will never have one.

@evpobr
Copy link
Contributor Author

evpobr commented Jan 16, 2019

I need this

find_package(Opus 1.3)

in my CMake project to work on any platfrom.

API version is important when configuring project, right?

On Unix-like systems i can run PkgConfig and retrive all the information i want.

On Windows i can find include directory and library path with CMake commands, but not API version. PkgConfig doesn't works properly here.

So what i can do?

Some projects define version information in headers:

#define ZLIB_VERSION "1.2.11"
#define ZLIB_VERNUM 0x12b0
#define ZLIB_VER_MAJOR 1
#define ZLIB_VER_MINOR 2
#define ZLIB_VER_REVISION 11
#define ZLIB_VER_SUBREVISION 0

Then i can parse it and set my OPUS_VERSION variable at last.

Everything works, find_package(Opus 1.3) fails if package version < 1.3, everybody happy.

@jmvalin
Copy link
Member

jmvalin commented Jan 17, 2019

OK, I think I get the problem. Now, the issue I see with your solution is how you handle older versions. You won't ever be able to see query version numbers 1.0 through 1.3 that way. The first you'd be able to query is 1.4 and it's likely more than a year away (could be multiple years). Maybe there's another way -- ideally one that would work for existing versions.

@evpobr
Copy link
Contributor Author

evpobr commented Jan 19, 2019

Maybe there's another way -- ideally one that would work for existing versions.

I think this is common way. According to find modules bundled with CMake.

Now, the issue I see with your solution is how you handle older versions.

In my case Vcpkg always have the last version, so it is not problem. Anyway, if opus_export.h is not required by any other public header, package manager may just generate & install this file for Opus <= 1.3.

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

2 participants