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

Fix:cmake:Fix the way we set the LIB_DIR when LIBDIR is set #574

Merged
merged 1 commit into from
Jun 3, 2018

Conversation

aerostitch
Copy link
Contributor

@aerostitch aerostitch commented May 3, 2018

Previously we were able to set LIBDIR to standard values like lib and the /navit folder would be added to it later on.
With #215 there have been some enhancements in the handling of LIB_DIR when LIBDIR is not set by setting LIB_DIR directly to lib/navit or lib64/navit. The problem is when you do set LIBDIR, then just the last folder of the provided path is taken as it was before. This causes 2 issues:

  • /navit is never added to it so you end up adding your libs to lib or lib64 directly
  • as explained in the cmake documentation section, the LIBDIR could be more than 1 folder level

So I propose we don't change the LIBDIR provided and set LIB_DIR to ${LIBDIR}/navit if LIBDIR is provided but not LIB_DIR (with that change we'll be able to have the same behavior as before #215).

@metalstrolch could you test that this behavior would do fine on Sailfish OS please?

Note: the file had a bunch of mixed spaces and tabs so I used vim to reindent everything to tabs.

Thanks
Joseph

@metalstrolch
Copy link
Contributor

OK, I'll check if this still works. But would you mind redoing the patch without the indention fix? It's impossible to find what you actually changed. Or at least do one commit fixing only the indention, and then another doing the change. That's one of the consequences i pointed to in #569

CMakeLists.txt Outdated
MESSAGE(STATUS " LIB_DIR (highest subdirectory if LIBDIR) is set to '${LIB_DIR}'")
ENDIF (CMAKE_SIZEOF_VOID_P EQUAL 4)
ELSE (NOT LIBDIR)
SET (LIB_DIR ${LIBDIR}/navit CACHE PATH "Navit lib path")
Copy link
Contributor

@metalstrolch metalstrolch May 3, 2018

Choose a reason for hiding this comment

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

Traditionally I would not and never want navit to add any sub directory if specifying LIBDIR maually. So if you want it to be "lib/navit" then one should set it to "lib/navit" and not just "lib". Otherwise it could never be "lib/something". If one wants the libraries reside just in "/lib" that's the way to do it.
It's fine for the default to be "lib/navit". So I think it was OK as it was before.

But now I'm confused. Why strippes the cmake script all starting folder names from LIB_DIR if LIBDIR is set? So When do i use LIBDIR and when LIB_DIR?

Sailfish build sets LIB_DIR directly and doesn't care for LIBDIR. And that (still) results in a working binary.
You can try yourself too if you want, by just getting the cmake command out of sailfish's spec file. There's nothing sailfish specific in setting odd paths.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

LIBDIR is the standard GNU and cmake variable that tells the root of the directory where ALL the libs should be (in a subfolder per package usually). See the link I put in the original description.

LIB_DIR is where Navit itself should have its library, so it is supposed to be $LIBDIR/navit.

Copy link
Contributor

Choose a reason for hiding this comment

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

The added navit directory is exactly my problem. On sailfish this would not be allowed. That's why I don't like adding a hard coded path here. If anything is manually provided we should just use that.
Whithout overriding LIB_DIR directly it's impossible to get rid of it. That's why I added the possibility to override LIB_DIR directly then. And used just that for Sailfish in the end (before I decided not to use modules at all)

With your change now, it's impossible again to put the .so files directly into /usr/lib because it always adds "navit" with no way to overome that. This way we would need another var to make it:
LIB_DIR = ${LIBDIR}/${LIB_ADD_DIR}
And then get rid of the hardcoded "navit" directories in the "LIBDIR not set" block above as well.
So:
Step 1: determine LIBDIR if not set
Step 2: make LIB_DIR equal to LIBDIR/LIB_ADD_DIR

Then for Sailfish I could use LIBDIR=share/ LIB_ADD_DIR="harbour-navit/lib/" to get the requested share/harbour-navit/lib. With -DCMAKE_INSTALL_PREFIX:PATH=/usr this makes the silly but required path /usr/share/harbour-navit/lib

Copy link
Contributor Author

@aerostitch aerostitch May 3, 2018

Choose a reason for hiding this comment

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

I think the fact that you added LIB_DIR is awesome, don't get me wrong and I think it's a perfectly correct use-case and we should keep it like this.
This change is more about the distros that use the GNU standards, to get the retro-compatibility with existing build systems.
The reason LIBDIR is overwritten in Debian is that we store the libs in lib even for 64bits versions.

If we go with a LIBDIR_SUFFIX or LIB_ADD_DIR I think we should probably set it to PACKAGE_NAME by default and then be able to overwrite it.
But that would cause an issue if a distro decides to install all the library inside lib as the LIB_ADD_DIR variable would need to stay unset.
So I think the best way to do it is what you did with allowing the overwrite of LIB_DIR and leave the old default (which added the navit path) like this PR does.

@aerostitch
Copy link
Contributor Author

aerostitch commented May 3, 2018

I just saw your comment about the indentation stuff, sorry for that. If I split the commit now I'm afraid we'll loose the current discussion thread that we have so let me summarize it outside of the code review comment and I'll do the split right after.

  • LIBDIR is (at least in GNU standards) the name of the folder to suffix to ${INSTALL_DIR} where all libraries should be. According to cmake documentation it is: object code libraries (lib or lib64 or lib/<multiarch-tuple> on Debian)
  • LIB_DIR is a good way you added to overwrite where the navit libaries should be (by default it is either lib/navit, lib64/navit or ${LIBDIR}/navit but thanks to your change it can be anything else you set when running cmake)

I think also that we should change the hardcoded navit default suffix with the PACKAGE variable (I would even have said PACKAGE_NAME initially but for whatever reason this one is set to navit-git, so not that).
Did I miss something?

@aerostitch
Copy link
Contributor Author

Commit split.

@aerostitch
Copy link
Contributor Author

Rebased from trunk and removed the un-necessary re-formatting now that trunk has them.

@aerostitch aerostitch merged commit f759dfe into navit-gps:trunk Jun 3, 2018
@aerostitch aerostitch deleted the fix_libdir branch June 3, 2018 17:00
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.

3 participants