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

[SofaKernel] FIX FileRepository on Windows #99

Merged
merged 2 commits into from
Dec 15, 2016

Conversation

guparan
Copy link
Contributor

@guparan guparan commented Dec 12, 2016

ADD dependencies to boost::filesystem and boost::locale

Fixes #17

ADD dependencies to boost::filesystem and boost::locale

Fixes #17
@guparan guparan added this to the 16.12 milestone Dec 12, 2016
@guparan guparan added issue: bug (major) Critical bug affecting everyone: not working, performances or accuracy degraded test labels Dec 13, 2016
@damienmarchal
Copy link
Contributor

I cannot test myself on window but:
- the tests is not failing anymore.
- no new tests are failing.
- I don't think the new boost dependency is a problem.

I let @matthieu-nesme (who made the issue #17) to make the final decision.

@matthieu-nesme
Copy link
Member

Great! and the fix is impressively concise.
The only concern could be the dynamic link to boost libraries, I guess it is mandatory?

@guparan
Copy link
Contributor Author

guparan commented Dec 13, 2016

Not all of them, we could replace ${Boost_LIBRARIES} with ${Boost_SYSTEM_LIBRARY} ${Boost_FILESYSTEM_LIBRARY} ${Boost_LOCALE_LIBRARY}

@bcarrez
Copy link
Contributor

bcarrez commented Dec 14, 2016

(edited)
No problem for me since boost is now mandatory.
As seen in the main CMakeLists.txt, line 158:
find_package(Boost REQUIRED)

Does the job, no test broken, fixes one failing test: green light for me.

@damienmarchal
Copy link
Contributor

Can I merge it ?

@guparan
Copy link
Contributor Author

guparan commented Dec 14, 2016

Nope, I have to search first for any header only improvement.

@guparan
Copy link
Contributor Author

guparan commented Dec 14, 2016

So there is no header-only way to use boost::filesystem and boost::locale.
The only improvement I see would be to move the find_package(Boost COMPONENTS filesystem locale QUIET) to SofaHelper.
Don't know if this would be a real improvement though 😕

@damienmarchal
Copy link
Contributor

So we merge it this way ?

@guparan
Copy link
Contributor Author

guparan commented Dec 15, 2016

No objections so I vote yes :)

@damienmarchal damienmarchal merged commit f59db8e into sofa-framework:master Dec 15, 2016
@guparan guparan deleted the fix_filerepository branch February 14, 2017 11:09
@guparan guparan added pr: fix Fix a bug and removed issue: bug (major) Critical bug affecting everyone: not working, performances or accuracy degraded labels May 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: fix Fix a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants