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

remove need for hard-coded boost path with unit tests #3660

Merged
merged 3 commits into from
Apr 17, 2020

Conversation

mMerlin
Copy link
Contributor

@mMerlin mMerlin commented Apr 16, 2020

I found that the existing unit test .pro files had boost_1_70_0 hard-coded for the boost environment. That broke for me, since boost_1_72_0 is being used locally. Trying to get generic detection to work, so that the same boost environment (detection logic) would be used for both phoenix and the unit tests, I discovered that the relative path detected and used by pri/boostdetect.pri did not work for the unit tests. The folder nest level paths do not match the detected path.

I created a new boostdetect version (near rewrite) that takes an optional flag to generate an absolute path. An older version of pri/boostdetect.pri was unconditionally setting an absolute path. That was changed to relative at the same time that the detection search was expanded to look for the boost environment folder in src/lib. Although I see that .gitignore was NOT adjusted to ignore a boost folder in src/lib.

This needs testing with a windows build. I do not have an environment available to do that. Some of the information seen says project and pri files are handled a bit differently on windows, using resource files for some things instead. With this version, phoenix.pro works with or without absolute_boost = 1. Without uses the same relative path as previously.

@KjellMorgenstern
Copy link
Member

KjellMorgenstern commented Apr 16, 2020

I think copying boost to src/lib (or any other library into the fritzing code) should be avoided.
What I actually did (or intended to do) when I repaired the build was to deprecate this behavior, and instead look at
a) BOOST_ROOT env variable,
b) same level as friting root, so next to fritzing
c) system installation of boost.

Only method b) is currently used and tested.
So, not having boost in the gitignore is how it should be.

@KjellMorgenstern
Copy link
Member

Please also see the refactor_projectstructure branch. I change the build system there in a way recommended by Qt5. This should also make it much more straightforward to include tests (Qt Creator will automatically manage them) . Also having cmake and Qt6 in mind with that.

pri/boostdetect.pri Outdated Show resolved Hide resolved
pri/boostdetect.pri Show resolved Hide resolved
@mMerlin
Copy link
Contributor Author

mMerlin commented Apr 16, 2020

I think copying boost to src/lib (or any other library into the fritzing code) should be avoided.

Agreed

Please also see the refactor_projectstructure branch. I change the build system there in a way recommended by Qt5. This should also make it much more straightforward to include tests (Qt Creator will automatically manage them) . Also having cmake and Qt6 in mind with that.

I noticed that the existing qtcreator build was scanning for and not finding tests. Is it practical to migrate the refactored build environment by it self, or are there too many other dependencies? It would be nice to get the tests better integrated.

I did a quick scan of the refactored project file, but am not familiar enough with them to assess what the impacts are.

@KjellMorgenstern KjellMorgenstern dismissed their stale review April 17, 2020 05:59

MacOS build is broken, can't find boost

@KjellMorgenstern
Copy link
Member

Windows build is still broken due to Qt license/installer change, not related to this PR.

@KjellMorgenstern KjellMorgenstern merged commit a799673 into fritzing:develop Apr 17, 2020
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.

2 participants