Skip to content
This repository has been archived by the owner on Feb 21, 2021. It is now read-only.

CMakeLists chain doesn't honour path variables. #485

Closed
lr-morales opened this issue Jun 10, 2016 · 15 comments
Closed

CMakeLists chain doesn't honour path variables. #485

lr-morales opened this issue Jun 10, 2016 · 15 comments

Comments

@lr-morales
Copy link
Member

At this moment, some CMakeLists.txt have hardcoded some of the installation paths, like /usr/local/ for base install path and /usr/lib/python2.7 for the location of Python. This would make users who need or want different destination paths for JdeRobot or with a non-standard python locations unable to change the paths for their installations in an easy way.

For the base install path, the CMake variable CMAKE_INSTALL_PREFIX should be used, as this is the variable that - under the view of CMake - marks the path prefix of main installation for a project.
In case of other path dependencies like the python one, we should use non-hardcoded methods when possible; i. e. worst case for python would be "asking the interpreter where the libraries should be"... meaning that the python interpreter location should be located too).

This would also mean that tools (like visualHFSM) or drivers of JdeRobot should not assume they or jderobot will reside in /usr/local in their code. This can be solved changing the sources to allow the usage of CMake functions like configure_file to resolve the installation path when running cmake.

lr-morales added a commit to lr-morales/JdeRobot that referenced this issue Jun 18, 2016
- Search python 2 executable as a new dependency.
- Search python module "lib" path and stores it in a variable.
lr-morales added a commit to lr-morales/JdeRobot that referenced this issue Jun 18, 2016
This replaces all hardcoded install prefixes ("/usr/local") with the
CMake variable CMAKE_INSTALL_PREFIX, allowing users to set their
favourite prefix.
@fqez
Copy link
Member

fqez commented Jun 22, 2016

I think this is a better solution for the installation of the project. Hardcoded paths could generate several headaches in the future. So I say yes to your proposal!

lr-morales added a commit to lr-morales/JdeRobot that referenced this issue Jul 9, 2016
lr-morales added a commit to lr-morales/JdeRobot that referenced this issue Jul 30, 2016
lr-morales added a commit to lr-morales/JdeRobot that referenced this issue Jul 30, 2016
lr-morales added a commit to lr-morales/JdeRobot that referenced this issue Jul 30, 2016
lr-morales added a commit to lr-morales/JdeRobot that referenced this issue Jul 30, 2016
lr-morales added a commit to lr-morales/JdeRobot that referenced this issue Jul 30, 2016
lr-morales added a commit to lr-morales/JdeRobot that referenced this issue Aug 30, 2016
lr-morales added a commit to lr-morales/JdeRobot that referenced this issue Aug 30, 2016
lr-morales added a commit to lr-morales/JdeRobot that referenced this issue Aug 30, 2016
chanfr added a commit that referenced this issue Sep 5, 2016
…paths

[Issue #485] Refactor easyiceconfig to use CMAKE_INSTALL_PREFIX
lr-morales added a commit to lr-morales/JdeRobot that referenced this issue Sep 5, 2016
@aitormf
Copy link
Collaborator

aitormf commented Sep 14, 2016

One of the Pull Request merged related of this Issue had a bug and now libJderobotInterfaces.so is installed in /usr/local/jderobot instead of /usr/local/lib/jderobot

@fqez
Copy link
Member

fqez commented Sep 14, 2016

You are right, it was included right here https://github.com/JdeRobot/JdeRobot/pull/531/files#diff-af3b638bc2a3e6c650974192a53c7291R140 It's easy to solve tho.

lr-morales added a commit to lr-morales/JdeRobot that referenced this issue Sep 15, 2016
lr-morales added a commit to lr-morales/JdeRobot that referenced this issue Sep 15, 2016
fqez added a commit that referenced this issue Sep 15, 2016
…e_paths

[Issue #485] Refactor resourcelocator to use CMAKE_INSTALL_PREFIX
@lr-morales
Copy link
Member Author

Note that it should be fixed with f1a7aaf, please check it.

@fqez fqez closed this as completed Sep 16, 2016
@lr-morales
Copy link
Member Author

One question @fqez ... should I - or somebody - request the re-opening of this issue when addressing the problem in remaining components (like visualHFSM) or create a new one?

@fqez
Copy link
Member

fqez commented Sep 16, 2016

Oh sorry, I missed the VisualHFSM part. Don't worry I can reopen it now.

@fqez fqez reopened this Sep 16, 2016
@lr-morales
Copy link
Member Author

Thanks.
I'm not sure at this moment if the only one remaining is VisualHFSM, but I'm sure that it is still using hardcoded "/usr/local"s in its code at least.

Regarding VisualHFSM... I'm not very familiarized with its internal structure and workflow, so some help would be appreciated; I want to be sure that is safe to set all hardcoded paths to the CMake variable versions or if some workaround is needed.

@reysam93
Copy link
Contributor

Sorry for the really late response, but I just saw this.

Yes, I think it is safe to replace the "/usr/local" paths with the CMake variables if the file setup.sh is also modified. As long as the paths inside VisualHFSM code are the same as the paths in that file, there shouldn't be any problem

@Voidminded
Copy link
Collaborator

Voidminded commented Jun 9, 2017

Hi all,

I'm new to JdeRobot and have the exact same issue, I REALY don't like to install it in my /usr/*, all the cpp components respect the CMAKE_INSTALL_PREFIX passed to cmake, however none of the python components seem to !
Based on previous conversations in this thread everything should be fixed except VisualHFSM, but I doubt it, or am I doing sth wrong ?! would you pass any other variable (set it as bash variable) or anything that defines the python path for cmake that I've missed skipping though all the cmake files ?!

@lr-morales
Copy link
Member Author

Hello there,

Tools like basic_component_py seems to honour CMAKE_INSTALL_PREFIX as expected, but libraries seem to not doing so or using anything similar.

Doing this check I found something that doesn't look right - maybe I'm wrong - with the install location of some __init__.py files. I'll open a new issue about this as it isn't the same as discussed here.

It would be a good source of help having a list of all tools, libs and drivers that still aren't honouring the prefix.

lr-morales added a commit to lr-morales/JdeRobot that referenced this issue Jul 26, 2017
aitormf added a commit to aitormf/JdeRobot that referenced this issue Jul 27, 2017
[issue JdeRobot#485] Set CMAKE_INSTALL_PREFIX only if default value is requested
@fqez fqez closed this as completed in 8d8f928 Jul 27, 2017
@lr-morales
Copy link
Member Author

@fqez I think VisualStates isn't completly fixed, it still has hardcoded paths. Should we close this issue?

@fqez
Copy link
Member

fqez commented Jul 27, 2017

Again, didn't notice. Sorry. Please @reysam93 can you check this one, so I can definetly close this issue?

Thanks!

@fqez fqez reopened this Jul 27, 2017
@lr-morales
Copy link
Member Author

lr-morales commented Jul 27, 2017

After reading again last @reysam93 comment:

Yes, I think it is safe to replace the "/usr/local" paths with the CMake variables if the file setup.sh is also modified. As long as the paths inside VisualHFSM code are the same as the paths in that file, there shouldn't be any problem

I have 2 questions:

  • Could VisualStates be modified to accept this path (or these paths) from configuration file, defaulting to some string if not defined? If so, could be this "default value" be located in a .h? It would simplify the task of making a .in that could be modified by CMake at generation time and also would allow to redefine the jderobot paths afterwards.

  • If this message talks about /usr/local at setup.sh, and this file still has this path... could [installation] move all install to /opt/jderobot #879 broke the component or simply visualStates relocates there? Have someone verified this? @fqez @aitormf

@fqez
Copy link
Member

fqez commented Jul 28, 2017

Hi @lr-morales

About the first question, I think @reysam93 should answer it because I am not sure about how visualStates is coded.

About the second one, actually the file setup.sh doesn't make any sense because those files are insatlled throught the CMakeLists.txt. Nevertheless, @aitormf solved the issue that you pointed out in this PR #891
We should delete that file, but for now I will merge it to avoid possible future problems.

@lr-morales
Copy link
Member Author

Ok, then we are waiting now to @reysam93 comment about the first question. I don't mind helping with the .in and cmake modifications on this subject (or on visualStates if is something that could help without problem).

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

No branches or pull requests

6 participants