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

[CImgPlugin] FIX CMakeLists install fail since pluginization #609

Merged
merged 5 commits into from
Apr 9, 2018

Conversation

marques-bruno
Copy link
Member

@marques-bruno marques-bruno commented Mar 14, 2018

Hello,

Since we talked about it during the SofaDev meeting,
Here's our QuickNDirty answer to the make install fail for CImgPlugin.
Of course as-is it is not acceptable or mergeable but it's here


This PR:

  • builds with SUCCESS for all platforms on the CI.
  • does not generate new warnings.
  • does not generate new unit test failures.
  • does not generate new scene test failures.
  • does not break API compatibility.
  • is more than 1 week old (or has fast-merge label).

Reviewers will merge only if all these checks are true.

@marques-bruno marques-bruno added the pr: fix Fix a bug label Mar 14, 2018
@marques-bruno marques-bruno added the pr: status wip Development in the pull-request is still in progress label Mar 14, 2018
@guparan
Copy link
Contributor

guparan commented Mar 14, 2018

Even in the eventuality that more changes are needed to actually clean the installation of SOFA, these changes still are needed.
So LGTM 👍

@damienmarchal
Copy link
Contributor

Hi all,

Thank bruno for the PR.

@guparan I'm not sure we should merge this now.
Do we really want to remove the config.h from the CMakeLists ?
Wouldn't it be better to use static config.h instead of config.h.in ?

Having a .in means that the file is a template that is transformed by CMakeLists. This is not the case here so the .in file is misleading and useless. In case we prefer an autogenerated config.h.in from a template it would be much better to have a single one instead of one per plugin/module.
Eg of what could be the content of such a config.h.in:

/// This file is autogenerated by CMakeLists.txt please do not edit. 
#ifndef @PROJECT_CNAME@_CONFIG_H
#define @PROJECT_CNAME@_CONFIG_H

#include <SofaBase/config.h>

#ifdef SOFA_BUILD_@PROJECT_CNAME@
#  define SOFA_TARGET @PROJECT_NAME@
#  define SOFA_@PROJECT_CNAME@_API SOFA_EXPORT_DYNAMIC_LIBRARY
#else
#  define SOFA_@PROJECT_CNAME@_API SOFA_IMPORT_DYNAMIC_LIBRARY
#endif

#endif /// @PROJECT_CNAME@_CONFIG_H

@guparan guparan added pr: status to review To notify reviewers to review this pull-request and removed pr: status wip Development in the pull-request is still in progress labels Apr 5, 2018
@guparan
Copy link
Contributor

guparan commented Apr 6, 2018

[ci-build][with-scene-tests]

@guparan guparan added pr: status ready Approved a pull-request, ready to be squashed and removed pr: status to review To notify reviewers to review this pull-request labels Apr 9, 2018
@guparan guparan merged commit 1c4772b into sofa-framework:master Apr 9, 2018
@guparan guparan added this to the v18.06 milestone Jun 18, 2018
@marques-bruno marques-bruno deleted the fix_cimgplugin_install branch September 6, 2018 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: fix Fix a bug pr: status ready Approved a pull-request, ready to be squashed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants