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

Updated building instructions for Linux #45

Merged
merged 3 commits into from
Jun 2, 2024

Conversation

DarkMatter-999
Copy link
Contributor

Added updated instructions for building on Linux and added a script to auto-download the correct bazel version and add it to local venv path.

@j20001970 j20001970 self-requested a review May 31, 2024 03:17
Copy link
Owner

@j20001970 j20001970 left a comment

Choose a reason for hiding this comment

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

It's a very good idea to download bazel automatically to python venv imo, but I think there are several points that can be improved before we get this merged:

  1. macOS and Windows could also experience similar problems from using different bazel version, we could address this altogether by converting the download step to python script, and make it part of setup.py while creating venv.
  2. Since we are using pre-compiled bazel anyways, we could download bazelisk instead to automatically select required version for mediapipe submodule, as they could update required bazel version in the future.

How do you think about the improvements above?

@DarkMatter-999
Copy link
Contributor Author

Yeah sure converting to python script and integrating it into the setup process itself is indeed a very good idea.

Just needed a small clarification how do you plan to handle the downloads?

  1. We can do it with Python itself using the urlrlib
  2. Let the platform handle it like wget/curl for Linux and MacOS and iwr for Windows.

@j20001970
Copy link
Owner

Yeah sure converting to python script and integrating it into the setup process itself is indeed a very good idea.

Just needed a small clarification how do you plan to handle the downloads?

1. We can do it with Python itself using the `urlrlib`

2. Let the platform handle it like `wget`/`curl` for Linux and MacOS and `iwr` for Windows.

Downloading through Python itself would be more suitable, so that we can maintain one code path for this task.

@DarkMatter-999
Copy link
Contributor Author

Added auto-download of the latest bazelisk version to the setup.py itself. ✌️

Is there any need to update the requirements.txt ?

docs/BUILDING.md Outdated
@@ -55,6 +55,8 @@ Run `build.py --help` to see all options.
1. Install OpenCV and FFmpeg, then modify `mediapipe/third_party/opencv_linux.BUILD` to make OpenCV visible to Bazel.
2. Run `build.py desktop` to build desktop library.

NOTE:- Bazel version 6.1.1 is strictly required and is installed automatically by running the `setup.py`, it is installed inside the Python venv.
Copy link
Owner

Choose a reason for hiding this comment

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

Nitpicking: No need to state required Bazel version explicitly if setup script has already handled for us, or we would need to update this section every time mediapipe change required version.

Copy link
Contributor Author

@DarkMatter-999 DarkMatter-999 Jun 1, 2024

Choose a reason for hiding this comment

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

Yeah, I was thinking about the need to handle this whenever mediapipe decides to update their version.
So.... I think I should just remove this whole line altogether

@j20001970
Copy link
Owner

Is there any need to update the requirements.txt ?

I prefer leaving it unchanged unless there are known issues or package deprecation.

@j20001970 j20001970 merged commit 7626080 into j20001970:master Jun 2, 2024
0 of 4 checks passed
@j20001970
Copy link
Owner

Thanks!

j20001970 pushed a commit that referenced this pull request Jun 2, 2024
* Updated building instructions for Linux and script for downloading proper bazel version

* Added automatic download Bazelisk during setup process

* Removed redundant information in BUILDING docs
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