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.

Sign up for GitHub

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

Jump to bottom

Correct INSTALL_PREFIX for MacOS #4916

Closed
wants to merge 1 commit into from
Closed

Correct INSTALL_PREFIX for MacOS #4916

wants to merge 1 commit into from

Conversation

nmahadevuni
Copy link
Collaborator

@nmahadevuni nmahadevuni commented May 12, 2023 •
edited
Loading

On MacOS, all brew installed dependencies go in brew prefix path(/opt/homebrew), and other components that need these dependencies will need to use this path as INSTALL_PREFIX.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 12, 2023
@netlify
Copy link

netlify bot commented May 12, 2023 •
edited
Loading

✅ Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 4581757
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/651f9fc252cd680008b06a18

@nmahadevuni
Copy link
Collaborator Author

@majetideepak Please review this.

@majetideepak
Copy link
Collaborator

@nmahadevuni the builds are failing. You need to rebase your branch with the latest main.

@nmahadevuni nmahadevuni force-pushed the fix_install_prefix_m1 branch from 3b9678d to 88466b6 Compare May 12, 2023 09:39
yingsu00
yingsu00 reviewed May 12, 2023
View reviewed changes
scripts/setup-helper-functions.sh Outdated
@@ -130,6 +130,10 @@ function cmake_install {
CPU_TARGET="${CPU_TARGET:-avx}"
COMPILER_FLAGS=$(get_cxx_flags $CPU_TARGET)

if [ "${CPU_TARGET}" == "arm64" ]; then
Copy link
Collaborator

Choose a reason for hiding this comment

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

Naveen, have you verified if the CMAKE_PREFIX_PATH and CMAKE_INSTALL_PREFIX are also "/opt/homebrew" on other OS? It would be safer to check both CPU and OS here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did not check. In the same file, it is clear that arm64 target type is only for Apple M1.

Copy link
Collaborator

@majetideepak majetideepak May 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

You can have arm64 with Linux like AWS Graviton. I see that we do not handle this under the Linux OS above.
But let's do the right thing here by adding "$OS" = "Darwin".

@nmahadevuni nmahadevuni force-pushed the fix_install_prefix_m1 branch from 88466b6 to 7240bd7 Compare May 18, 2023 06:09
majetideepak
majetideepak approved these changes May 18, 2023
View reviewed changes
Copy link
Collaborator

@majetideepak majetideepak left a comment

Choose a reason for hiding this comment

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

@kgpai Can you please review and merge? Thanks.

@majetideepak
Copy link
Collaborator

Thanks, @nmahadevuni

kgpai
kgpai reviewed May 24, 2023
View reviewed changes
scripts/setup-helper-functions.sh Outdated
@@ -130,6 +131,11 @@ function cmake_install {
CPU_TARGET="${CPU_TARGET:-avx}"
COMPILER_FLAGS=$(get_cxx_flags $CPU_TARGET)

if [[ "$OS" == "Darwin" && "${CPU_TARGET}" == "arm64" ]]; then
INSTALL_PREFIX="/opt/homebrew"
Copy link
Contributor

Choose a reason for hiding this comment

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

Convention internally is to have homebrew installed on personal home dirs, Can we instead change this to `INSTALL_PREFIX="${INSTALL_PREFIX:-/opt/homebrew}"

@nmahadevuni nmahadevuni force-pushed the fix_install_prefix_m1 branch from 7240bd7 to f204244 Compare May 30, 2023 11:59
@nmahadevuni
Copy link
Collaborator Author

Thanks @majetideepak @kgpai . Addressed the comments. Please review again.

@ethanyzhang
Copy link

Hi @majetideepak @kgpai, can we revisit this? It seems to be very close.

@kgpai
Copy link
Contributor

kgpai commented Aug 23, 2023

@nmahadevuni Can you rebase against latest main , I will start the merge process after that.

@majetideepak
Copy link
Collaborator

@nmahadevuni is this still relevant?
This also seems to be specific to MacOS. Can we add this there?

majetideepak
majetideepak requested changes Aug 23, 2023
View reviewed changes
Copy link
Collaborator

@majetideepak majetideepak left a comment

Choose a reason for hiding this comment

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

Move this to setup-macos.sh?

@nmahadevuni
Copy link
Collaborator Author

Move this to setup-macos.sh?

@majetideepak This change needs to be in setup-helper-functions.sh, since the build commands here use the prefix.

@nmahadevuni nmahadevuni force-pushed the fix_install_prefix_m1 branch from f204244 to 67de8bf Compare October 4, 2023 15:19
@majetideepak
Copy link
Collaborator

This change needs to be in setup-helper-functions.sh, since the build commands here use the prefix.

INSTALL_PREFIX variable can be defined inside setup-macos.sh as well since that includes the helper script.
Some users can have brew installed in a custom location.
We fixed a similar issue with finding the brew path here #5184 and #5152
In a similar manner, we should get the HOMEBREW_PREFIX using brew and set the INSTALL_PREFIX to that value.

@majetideepak
Copy link
Collaborator

majetideepak commented Oct 4, 2023 •
edited
Loading

I am also curious why we need to install dependencies in the brew prefix and not the default system location.
Can you add a description to the PR?

@nmahadevuni
Copy link
Collaborator Author

I am also curious why we need to install dependencies in the brew prefix and not the default system location. Can you add a description to the PR?

That's the default in M1, all brew installed dependencies go in this path, and other components that need these dependencies will need to use the brew prefix. I will automate getting HOMEBREW_PREFIX as you mentioned.

@majetideepak
Copy link
Collaborator

all brew installed dependencies go in this path, and other components that need these dependencies will need to use the brew prefix.

You just have to add the CMAKE_PREFIX_PATH to include the brew install location. You don't need to set INSTALL_PREFIX to brew prefix for other dependencies.

@nmahadevuni nmahadevuni force-pushed the fix_install_prefix_m1 branch from 67de8bf to 159a4b1 Compare October 6, 2023 05:47
@nmahadevuni nmahadevuni changed the title Correct INSTALL_PREFIX for arm64 target Correct INSTALL_PREFIX for MacOS arm64 target Oct 6, 2023
@nmahadevuni nmahadevuni changed the title Correct INSTALL_PREFIX for MacOS arm64 target Correct INSTALL_PREFIX for MacOS Oct 6, 2023
@nmahadevuni nmahadevuni force-pushed the fix_install_prefix_m1 branch from 159a4b1 to 4581757 Compare October 6, 2023 05:48
@ethanyzhang
Copy link

@majetideepak can you check?

@nmahadevuni
Copy link
Collaborator Author

We can set CMAKE_PREFIX_PATH to the install directory to work around this. Closing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yingsu00 yingsu00 yingsu00 left review comments

@kgpai kgpai kgpai left review comments

@majetideepak majetideepak majetideepak requested changes

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@nmahadevuni @majetideepak @ethanyzhang @kgpai @yingsu00 @facebook-github-bot