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

adjust scripts, Cmake for OSX. Updates README #96

Merged
merged 1 commit into from
May 31, 2022

Conversation

davebryson
Copy link
Contributor

@davebryson davebryson commented Apr 29, 2022

Signed-off-by: Dave Bryson

Changes:

  • Removed brew installs from configure.sh and added to build information in README. Why? calling sudo ./configure.sh fails as brew can't be called with sudo
  • Moved OSX specific CMAKE_FLAGS to CmakeFile.txt
  • Added build information to README

References: #94

@HalosGhost
Copy link
Collaborator

HalosGhost commented Apr 29, 2022

calling sudo ./configure.sh fails as brew can't be called with sudo

Does it make sense, then, to instead inline the use of sudo for the linux commands (resulting in the correct invocation for both platforms being ./scripts.configure.sh (no sudo))? This PR as-it-stands makes it so that there's an additional step for macOS (the brew install commands) beyond what's necessary for linux (which, in an ideal world, I think we should try to avoid).

Additionally, it looks like this PR includes some trailing whitespace in a few files (namely, CMakeLists.txt, and README.md) which is why the lint CI check failed.

@davebryson
Copy link
Contributor Author

davebryson commented Apr 29, 2022

@HalosGhost Thanks for the feedback. Resolving the linting issues now.

As for sudo, this is something I went back and forth on a couple of times. I even attempted having cmake install leveldb and nuraft to eliminate the need for sudo. But in the end, I settled on the path of least resistance (for now) so as not to cause a ripple effect with ci.yml etc... I'm open to suggestions... There's definitely room for improvement. For example, OSX is using leveldb from brew that's a different version than the one used by the linux build. And is llvm@11 a brew dependency solely for clang-tidy and clang-format?

Copy link
Contributor

@jonwiggins jonwiggins left a comment

Choose a reason for hiding this comment

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

This worked for me on an M1 Mac. Although at first after following the steps when I ran ./scripts/build.sh, I got:

CMake Error at CMakeLists.txt:4 (project):
  The CMAKE_C_COMPILER:

    /Applications/Xcode.app/Contents/Developer/usr/bin/clang

  is not a full path to an existing compiler tool.

I had to run

sudo xcode-select -switch /Library/Developer/CommandLineTools

And then everything worked.

README.md Outdated Show resolved Hide resolved
@HalosGhost
Copy link
Collaborator

sudo xcode-select -switch /Library/Developer/CommandLineTools

This might make sense to include in the README in the macOS instructions. @davebryson what do you think?

@davebryson
Copy link
Contributor Author

@HalosGhost Maybe xcode-select --install would be better to make sure it's installed. I believe installing it will automatically set the path.

@jonwiggins
Copy link
Contributor

@HalosGhost Maybe xcode-select --install would be better to make sure it's installed. I believe installing it will automatically set the path.

I had already installed it, it looks like the issue can be caused by:

  1. Installing other programs which mess with the path: https://stackoverflow.com/questions/17980759/xcode-select-active-developer-directory-error
  2. Updating macOS: https://developer.apple.com/forums/thread/666584

@HalosGhost HalosGhost linked an issue May 4, 2022 that may be closed by this pull request
3 tasks
README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@AlexRamRam AlexRamRam left a comment

Choose a reason for hiding this comment

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

Add cmake to brew install list

Copy link
Contributor

@AlexRamRam AlexRamRam left a comment

Choose a reason for hiding this comment

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

My apologies for the multiple reviews (open-source/GH newbie ...)

README.md Show resolved Hide resolved
@davebryson davebryson mentioned this pull request May 26, 2022
@HalosGhost
Copy link
Collaborator

@davebryson I took another look, and Alex has had occasion to do another clean build using your changes, and they all look set. Can you squash a single commit, and then I shall merge it?

@HalosGhost
Copy link
Collaborator

@davebryson it looks like you merged back in from trunk, but didn't squash (resulting in 10 commits to pull rather than 1).

@davebryson
Copy link
Contributor Author

@HalosGhost Yea I pulled to merge the latest by accident. But haven't done the squash yet. Sorry for the mis-leading message

Signed-off-by: Dave Bryson <daveb@miceda.org>

Updated scripts, cmake, and README for building on OSX
@davebryson
Copy link
Contributor Author

davebryson commented May 26, 2022

@HalosGhost Squash is done. I had to back out the latest merges from dci/trunk.

@HalosGhost
Copy link
Collaborator

HalosGhost commented May 31, 2022

@davebryson these changes look solid and I'm ready to merge but I just wanted to double-check one thing with you. You've included a DCO sign-off, so as far as we're concerned, this is set from an IP perspective. However, because your sign-off isn't at the end of the commit message, git isn't aware of it:

$ git interpret-trailers --parse <this-commit-as-a-patch-file>
$

This might mean that you're not credited appropriately for the commit. If you're worried about that, you can amend the commit message so that the sign-off is in its own paragraph at the end of the commit message (contiguously with any other trailers):

Support for OSX Build

Updated scripts, cmake, and README for building on OSX

Signed-off-by: Dave Bryson <daveb@miceda.org>

Which git understands correctly:

$ git interpret-trailers --parse <the-updated-commit-patch>
Signed-off-by: Dave Bryson <daveb@miceda.org>

Feel free to push the amended commit if you want to fix it; otherwise, ping me and I'll merge!

@HalosGhost
Copy link
Collaborator

Conferred offline; @davebryson asked for it to be merged.

@HalosGhost HalosGhost merged commit ebc542f into mit-dci:trunk May 31, 2022
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.

Unable to build on OSX with bash scripts
4 participants