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

Changed the SOVERSION to major.minor. #812

Merged
merged 1 commit into from
Mar 12, 2015
Merged

Changed the SOVERSION to major.minor. #812

merged 1 commit into from
Mar 12, 2015

Conversation

eXpl0it3r
Copy link
Member

Here are is the change as discussed.

@binary1248
Copy link
Member

👍

@eXpl0it3r
Copy link
Member Author

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

@eXpl0it3r eXpl0it3r self-assigned this Mar 2, 2015
@eXpl0it3r
Copy link
Member Author

Since on Linux the shared libraries use -major.minor.so as suffix, I was wondering if we should apply the same naming for Windows DLLs as well, i.e. -major.minor.dll?

@zsbzsb
Copy link
Member

zsbzsb commented Mar 2, 2015

I would agree with that change to the dll naming convention.

@MarioLiebisch
Copy link
Member

I was wondering if we should apply the same naming for Windows DLLs as well

Not sure about that. Doing that is very, very uncommon. Especially considering that Windows will always try to load dependencies straight from the executable's own folder anyway (so "bring your own stuff" is a lot easier to do).

Also don't forget STL incompatibilities between toolchains and their versions. Just because you've got the same version DLL doesn't mean it's really compatible.

@zsbzsb
Copy link
Member

zsbzsb commented Mar 2, 2015

Also don't forget STL incompatibilities between toolchains and their versions. Just because you've got the same version DLL doesn't mean it's really compatible.

Well... this is already the situation. You can't do much at all to fix this. Unless we started appending the compiler and it's version in the name also (never ever seen this done).

@MarioLiebisch
Copy link
Member

Correct. So I'd just stick with the current approach and leave the Windows file names as-is.

@eXpl0it3r
Copy link
Member Author

Any more comments on this (-2.2.dll vs -2.dll)?

@binary1248
Copy link
Member

DLL hell is bad enough. I guess if we can prevent the dynamic linker from loading an incompatible (or simply the wrong) library, it wouldn't harm. -2.2.dll reveals the ABI version the best without having to resort to more elaborate methods.

@zsbzsb
Copy link
Member

zsbzsb commented Mar 3, 2015

I say that DLLs should have the major and minor, but also think we should save that discussion for another time if it stalls this PR.

After all, we do gain a few advantages with that change and don't really loose much at all.

@eXpl0it3r
Copy link
Member Author

Given the other open issues, we're not in a hurry with the change. So I'm still interested in further comments.

@eXpl0it3r
Copy link
Member Author

Added the change for the DLLs, but I'm still open to comments about.

@zsbzsb
Copy link
Member

zsbzsb commented Mar 9, 2015

This is about all I can think of regarding the change to dll naming.

Pros

  • More information available about version of SFML (useful when solving linker errors, missing symbols [time division anyone?] determining installed version, ect)
  • Can help solve some side affects of DLL hell as it can prevent older versions of SFML being loaded that don't match if the required version is not present
  • MS even adds versions to their DLLs - msvcp[60|100|110|120].dll
  • Can be added to CSFML and also help out bindings so when they dynamically load SFML they will load the right version.

Cons

  • Maybe breaks some existing linker/install/packaging scripts
  • Changing how it currently is
  • Is there anything else?

@eXpl0it3r eXpl0it3r force-pushed the bugfix/soversion branch 2 times, most recently from 6944b86 to 2a2dca6 Compare March 10, 2015 12:28
@eXpl0it3r
Copy link
Member Author

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

@eXpl0it3r
Copy link
Member Author

With this PR, we'll only change the soversion. The DLL stuff will be discussed elsewhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants