Skip to content

Added checks for python, bazel, pipenv for OS X#181

Merged
chinmayshah99 merged 3 commits intoOpenMined:devfrom
codeboy5:prereqs-mac
Jun 16, 2020
Merged

Added checks for python, bazel, pipenv for OS X#181
chinmayshah99 merged 3 commits intoOpenMined:devfrom
codeboy5:prereqs-mac

Conversation

@codeboy5
Copy link
Copy Markdown
Member

@codeboy5 codeboy5 commented Jun 3, 2020

Pull Request

Description

Added checks for python, bazel & pipenv for OS X, install them if they are not already present

Affected Dependencies

List any dependencies that are required for this change.

Type of Change

Please mark options that are relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • Documentation (non-breaking change which adds documentation)
  • Improvement (non-breaking change that improves the performance or reliability of existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How has this been tested?

  • Describe the tests that you ran to verify your changes.
  • Provide instructions so we can reproduce.
  • List any relevant details for your test configuration.

Checklist

Additional Context

Please also include relevant motivation and context.

@madhavajay
Copy link
Copy Markdown
Contributor

Works for me:
$ ./prereqs_mac.sh

Already up-to-date.
Checking for python3 installation
Python 3 already installed
Bazel already installed
Checking for pipenv
pipenv is already installed

Only issue I can see is there isn't any version requirements, and most of these things aren't hugely version dependant, but if there is a version bug later we will need to be careful to document it. Maybe mention some working versions in the README.md or contributing.md just in case?

madhavajay
madhavajay previously approved these changes Jun 8, 2020
Copy link
Copy Markdown
Contributor

@madhavajay madhavajay left a comment

Choose a reason for hiding this comment

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

LGTM

@chinmayshah99
Copy link
Copy Markdown
Member

@madhavajay Is it possible in homebrew to specify which version to install?

@codeboy5
Copy link
Copy Markdown
Member Author

codeboy5 commented Jun 8, 2020

@madhavajay Is it possible in homebrew to specify which version to install?

Yes we can do something like brew install bazel@3.2.2

Copy link
Copy Markdown
Member

@chinmayshah99 chinmayshah99 left a comment

Choose a reason for hiding this comment

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

Specify the version number of each package during brew install

@codeboy5
Copy link
Copy Markdown
Member Author

codeboy5 commented Jun 8, 2020

Specify the version number of each package during brew install

I use bazel 3.2.0 and python 3.8.3 , should I use this ?

@chinmayshah99
Copy link
Copy Markdown
Member

chinmayshah99 commented Jun 8, 2020 via email

@codeboy5
Copy link
Copy Markdown
Member Author

codeboy5 commented Jun 8, 2020

Yeah, bazel 3.2 works. And python version as 3.7?

Doing it for bazel would be difficult, since we are tapping and using this https://github.com/bazelbuild/homebrew-tap/blob/master/Formula/bazel.rb. It has 3.2.0 set right now, for installing a specific version we would have to install it using a binary installer. Should we change, brew makes it really simple.
@madhavajay What do you think ?

@madhavajay
Copy link
Copy Markdown
Contributor

As much as I love brew, I think the binaries for bazel are also really easy to use.
See the Dockerfile:
https://github.com/OpenMined/PyDP/blob/dev/Dockerfile

Maybe take this and switch to the macOS binary release.
https://github.com/bazelbuild/bazel/releases/download/3.2.0/bazel-3.2.0-darwin-x86_64

ARG BAZEL_VERSION=3.2.0
ARG BAZEL_INSTALLER=bazel-${BAZEL_VERSION}-installer-linux-x86_64.sh
ARG BAZEL_DOWNLOAD_URL=https://github.com/bazelbuild/bazel/releases/download
RUN wget ${BAZEL_DOWNLOAD_URL}/${BAZEL_VERSION}/${BAZEL_INSTALLER} && \
    chmod +x ${BAZEL_INSTALLER} && ./${BAZEL_INSTALLER} --user && rm ${BAZEL_INSTALLER}

I actually think it's perfectly sane to pull a specific pinned Bazel binary and call it from within the project dir so that we don't force the users to change their system path Bazel.

With brew, the python versions should co-exist as different /usr/local/bin symlinks so that should also work by pinning to a version. The less invasive and more isolated and reliable the dev setup / code the better.

@madhavajay
Copy link
Copy Markdown
Contributor

Looks like there might be another way.

It seems that the homebrew core is actually very up to date with bazel now:
https://github.com/Homebrew/homebrew-core/commits/master/Formula/bazel.rb

Lets try an older version for an example:
version 3.1.0
Homebrew/homebrew-core@051a657

$ BREW_VERSION_SHA=051a657252392afcfa7b560d36db0a09c0fe1647
BREW_FORMULA_NAME=bazel
brew info $BREW_FORMULA_NAME \
| sed -n \
    -e '/^From: /s///' \
    -e 's/github.com/raw.githubusercontent.com/' \
    -e 's%blob/%%' \
    -e "s/master/$BREW_VERSION_SHA/p" \
| xargs brew install

This might at least help take care of other dependencies like OpenJDK, but in the end you will run into errors on upgrade etc.

Error: bazel 3.0.0 is already installed
To upgrade to 3.2.0, run `brew upgrade bazel`.

Completely and reliably automating the whole thing might not be worth the effort and testing required due to all the edge cases. Pulling a binary or using the .sh installer might be easier.

@codeboy5
Copy link
Copy Markdown
Member Author

codeboy5 commented Jun 9, 2020

Looks like there might be another way.

It seems that the homebrew core is actually very up to date with bazel now:
https://github.com/Homebrew/homebrew-core/commits/master/Formula/bazel.rb

Lets try an older version for an example:
version 3.1.0
Homebrew/homebrew-core@051a657

$ BREW_VERSION_SHA=051a657252392afcfa7b560d36db0a09c0fe1647
BREW_FORMULA_NAME=bazel
brew info $BREW_FORMULA_NAME \
| sed -n \
    -e '/^From: /s///' \
    -e 's/github.com/raw.githubusercontent.com/' \
    -e 's%blob/%%' \
    -e "s/master/$BREW_VERSION_SHA/p" \
| xargs brew install

This might at least help take care of other dependencies like OpenJDK, but in the end you will run into errors on upgrade etc.

Error: bazel 3.0.0 is already installed
To upgrade to 3.2.0, run `brew upgrade bazel`.

Completely and reliably automating the whole thing might not be worth the effort and testing required due to all the edge cases. Pulling a binary or using the .sh installer might be easier.

@chinmayshah99 should we switch to the binary installer ?

@chinmayshah99
Copy link
Copy Markdown
Member

@chinmayshah99 should we switch to the binary installer ?

Yeah, let's shift to binary installer!

chinmayshah99
chinmayshah99 previously approved these changes Jun 10, 2020
Copy link
Copy Markdown
Member

@chinmayshah99 chinmayshah99 left a comment

Choose a reason for hiding this comment

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

LGTM!

@chinmayshah99 chinmayshah99 requested review from madhavajay and removed request for Frank-7 June 10, 2020 21:01
@codeboy5
Copy link
Copy Markdown
Member Author

@chinmayshah99 should we switch to the binary installer ?

Yeah, let's shift to binary installer!

For the binary installer, the Xcode command line tools need to be installed first

LGTM!

Our linux prereqs uses an older version of bazel, should we update just to be consistent ?

@codeboy5 codeboy5 changed the title Added checks for python, bazel & pipenv for OS X Added checks for python, bazel, pipenv for OS X and switch to binary installer for bazel Jun 10, 2020
@chinmayshah99
Copy link
Copy Markdown
Member

For the binary installer, the Xcode command line tools need to be installed first

For any other process, we don't need Xcode correct?

Our linux prereqs uses an older version of bazel, should we update just to be consistent ?

Yeah, we can move to bazel 3.2. But that needs to be tested and also on a separate PR.

@codeboy5
Copy link
Copy Markdown
Member Author

For the binary installer, the Xcode command line tools need to be installed first

For any other process, we don't need Xcode correct?

Our linux prereqs uses an older version of bazel, should we update just to be consistent ?

Yeah, we can move to bazel 3.2. But that needs to be tested and also on a separate PR.

No we don't need Xcode for any other process, infact we don't need Xcode just xcode command line tools which can be done using xcode-select --install . We can add a note to the contributing so that users are aware

@chinmayshah99
Copy link
Copy Markdown
Member

chinmayshah99 commented Jun 10, 2020 via email

@codeboy5
Copy link
Copy Markdown
Member Author

How big is xcode-select --install? Because from what I understand, Xcode takes up 10GB+ disk space.

iirc it is around 700mb.

@chinmayshah99
Copy link
Copy Markdown
Member

chinmayshah99 commented Jun 10, 2020 via email

@chinmayshah99
Copy link
Copy Markdown
Member

What if is check for xcode-select --install and if it's present, use, command line. Else use brew.

@codeboy5
Copy link
Copy Markdown
Member Author

What if is check for xcode-select --install and if it's present, use, command line. Else use brew.

Could add that but seems an overkill, brew is the best option imo.If bazel seems to remove backwards compatibility in some major version later, we could switch.

@chinmayshah99
Copy link
Copy Markdown
Member

chinmayshah99 commented Jun 11, 2020 via email

@codeboy5 codeboy5 changed the title Added checks for python, bazel, pipenv for OS X and switch to binary installer for bazel Added checks for python, bazel, pipenv for OS X Jun 14, 2020
@chinmayshah99 chinmayshah99 merged commit 3013a61 into OpenMined:dev Jun 16, 2020
dvadym added a commit to dvadym/PyDP that referenced this pull request Jul 3, 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.

3 participants