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

Update cmakelists, and documentation for building on OSX. #27

Closed
wants to merge 2 commits into from

Conversation

Senjai
Copy link

@Senjai Senjai commented Dec 3, 2017

Some of this was sourced from #23.

I ran into some issues with homebrew and tex, so I thought I'd make the changes I made to get past them available for others.

Users often install qt or other libraries on mac via homebrew. Because
other installs of qt may exist, these may not be under the default
prefix for CMake.

This ensures that Qt is found properly in those cases.
This was a bit incomplete when I went through this process for High
Sierra.
Copy link
Contributor

@cvuchener cvuchener left a comment

Choose a reason for hiding this comment

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

With the growing build instructions it may be time to separate them in a BUILDING.rst file. But that can be done later (I can do that myself when I complete the linux part). Just make the changes requested in the line comments.

@@ -28,7 +28,9 @@ The manual for Dwarf Therapist can be downloaded separately.
Building
========

Dwarf-Therapist requires a C++ compiler (with C++14 support), cmake (3.1.0 or newer), and Qt5 (with Widgets and QML modules).
Dwarf-Therapist requires a C++ compiler (with C++14 support), cmake (3.1.0 or newer), and Qt5.5 (with Widgets and QML modules).
Copy link
Contributor

Choose a reason for hiding this comment

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

Qt 5.5 is not required. Any Qt 5 version should work. If DT use some features that was added during the Qt 5 cycle, it should be "Qt 5.x or newer". I have built DT on Ubuntu Trusty with Qt 5.2 so it is at least that.

Copy link
Author

Choose a reason for hiding this comment

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

News to me, I'll update

# On Mac, Homebrew installs this version in /usr/local/opt/qt@5.5
# without this it looks in the usual /usr/local prefix.
# this must occur before the find_package call below
list(APPEND CMAKE_PREFIX_PATH "/usr/local/opt/qt@5.5")
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer adding in the building instructions: "use -DCMAKE_PREFIX_PATH=/usr/local/opt/qt@5.5" rather than hard-coding paths in cmakelists. If Qt is installed any other way, that will break.

Travis uses "CMAKE_PREFIX_PATH=/usr/local/opt/qt" without the version number.

Copy link
Author

@Senjai Senjai Dec 4, 2017

Choose a reason for hiding this comment

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

So, I'm not familiar with cmake, I don't use it on the reg. Would you be fine if this path didn't include the version number + the instructions indicated that it can be overridden? Or are you suggesting something else? I did first try this as an environment variable to no avail.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was suggesting to not change CMakeLists.txt and write something in the building instructions like:

brew install qt@5.5
export CMAKE_PREFIX_PATH=/usr/local/opt/qt@5.5
...

Would you be fine if this path didn't include the version number?

It would be fine if it was a standard location. If you install qt@5.x (specifying a version number), will /usr/local/opt/qt work? If it is too hard to predict where qt-sdk is installed. I think it would be better for the user to set it explicitly, this way it is easier for the user to adapt the value if cmake fails to find Qt.

Dwarf-Therapist requires a C++ compiler (with C++14 support), cmake (3.1.0 or newer), and Qt5 (with Widgets and QML modules).
Dwarf-Therapist requires a C++ compiler (with C++14 support), cmake (3.1.0 or newer), and Qt5.5 (with Widgets and QML modules).

For All Users:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better with some kind of formatting, to make the distinction between sections (all/linux/mac) clearer: bold or low level title.

Copy link
Author

Choose a reason for hiding this comment

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

Yep, I can poke.


brew install qt@5.5
brew install cmake
brew cask install basictex
Copy link
Contributor

Choose a reason for hiding this comment

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

If you want to build the manual, you need to add -DBUILD_MANUAL=ON in cmake parameters. Is basictex enough? On fedora I need: texlive-bibtex, texlive-cm, texlive-latex-bin, texlive-makeindex, texlive-mfware, texlive-preprint, texlive-sidecap, texlive-wrapfig.


After running make, do the following::

mv build/Dwarf-Therapist.app ~/Applications
Copy link
Contributor

Choose a reason for hiding this comment

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

This installs only the executable, right? The memory layouts should be copied too, and the manual if you built it.

@cvuchener
Copy link
Contributor

It is not finished yet but I created PR #31 so you can see how BUILDING.rst content could look like. The plan is to move your instructions in the "macOS" section.

@cvuchener
Copy link
Contributor

This pull request has been inactive for too long. I merged #50. If you want to improve the OSX instructions, create a new pull request based on the new changes (with BUILDING.md).

@cvuchener cvuchener closed this Jan 17, 2018
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.

None yet

2 participants