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

lut3d - fix for 4 digits gmic version #4121

Merged
merged 1 commit into from
Jan 15, 2020

Conversation

phweyland
Copy link
Contributor

Sorry, you have not seen my message asking for waiting a bit ...
I had forgotten the 4 digits version issue

@TurboGit
Copy link
Member

I did not see the message indeed, let's merge this now. Thanks.

@TurboGit TurboGit self-assigned this Jan 15, 2020
@TurboGit TurboGit self-requested a review January 15, 2020 18:30
@TurboGit TurboGit added the bugfix pull request fixing a bug label Jan 15, 2020
@TurboGit TurboGit added this to the 3.0.1 milestone Jan 15, 2020
@TurboGit TurboGit merged commit 1befa7d into darktable-org:master Jan 15, 2020
@phweyland phweyland deleted the gmic-ver-fix branch January 15, 2020 18:39
@parafin
Copy link
Member

parafin commented Jan 15, 2020

What is the point of lut3d_gmic_version function now? It won't return runtime version of the gmic library, but the version dt was compiled with, since gmic_version is a macros, not a variable.

@phweyland
Copy link
Contributor Author

phweyland commented Jan 15, 2020

What is the point of lut3d_gmic_version function now? It won't return runtime version of the gmic library, but the version dt was compiled with, since gmic_version is a macros, not a variable.

I wanted to keep the gmic version along with the compressed clut.
But you are right, if the run time gmic version is different this may be useless.

For windows, the libgmic library is added to dt binaries distribution. So no issue here.
For linux, I've seen dt uses the installed one ... Here we have a strongest issue. dt can crash if the run time version is not the same as the build time one. Is that correct ?
But that is general problem. How the coherence between build time and run time libraries is controlled on linux ?

@phweyland
Copy link
Contributor Author

I find that to keep gmic version can be interesting if at some point gmic changes some part of compression algorithm. That's a provision. No role today, just trying to keep coherence. Nothing critical.

I can add back the run time version.

I can also remove the version. In that case there are 3 possibilities:

  • remove the parameter and the revert back to version 1 (some current edits can fail)
  • transform the parameter to a spare one and let the version 2
  • remove the parameter and increment again the version.

What do you think ?

@parafin
Copy link
Member

parafin commented Jan 15, 2020

So, about library versions, usually backwards compatibility is assumed unless ABI is changed. It means that it's OK to build with older library version and then upgrade a library without recompiling the program. ABI compatibility breakage is handled by changing an ABI version (suffix after .so extension in the filename), so re-linking (and re-compilation) of the program is required. ABI is considered to be changed if some function changed its signature or if some symbol (e.g. function) is removed. Library can add a new function in newer version without breaking ABI, but if user downgrades it to the version below required for the program, the program won't start (or will segfault later) because of unresolved symbols. It's user's/distribution's responsibility not to allow it. There's also symbol versions, but it's more advanced topic.

About saving the version in case behaviour of the library changes - usually it's assumed that any change in behaviour shouldn't break the programs using the library. Otherwise library should provide API to detect such situations, which it seems gmic doesn't do. So it's a question for gmic developers, not us, what do they think of the scenario you described. Also, what do you want to do if you detect version change? Display a message and refuse to work?

I would suggest to remove gmic version from iop params by adding version 3.

@phweyland
Copy link
Contributor Author

Thanks for the explanation @parafin
I'll remove the version then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix pull request fixing a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants