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

Improved new developer experience: documentation & environment setup #34317

Draft
wants to merge 28 commits into
base: master
Choose a base branch
from

Conversation

MikeBusuttil
Copy link
Contributor

@MikeBusuttil MikeBusuttil commented Dec 27, 2024

  • Links to recently updated system requirements and WSL wiki articles
    • WSL-specific instructions and issues moved to wiki
  • Link to thorough Intro to Openpilot wiki
  • removed redundant steps from installation documentation
  • added required packages to installation

Comment on lines +44 to +45
libfreeimage-dev \
libfreeimage3 \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

required for rerun

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's leave these out for now, I'm not sure rerun is moving forward.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok sure. I'll put them in the WSL docs

@@ -57,7 +59,9 @@ function install_ubuntu_common_requirements() {
opencl-headers \
ocl-icd-libopencl1 \
ocl-icd-opencl-dev \
pocl-opencl-icd \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

required for OpenCL

Copy link
Collaborator

Choose a reason for hiding this comment

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

Required for webcam only, which is a relatively rare need. I just checked on my machine, it wants to drag in 22 dependencies and 845MB of disk. Let's leave it as manual for webcam users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI it's also required for sim users. You're right, It is hefty, as are the CUDA drivers - but these are important to mention to devs that'll be attempting to run anything that uses models. I'll mention it in the docs instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please be very mindful before adding packages to this list since it makes everything big and slow.

The pocl icd is also very slow since it is a software implementation of opencl, so it is better to mention it as a last resort for people without a driver installed already.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's good to know Max, I wasn't aware, I'll remove it from here and add it to some documentation elsewhere

portaudio19-dev \
pulseaudio \
Copy link
Contributor Author

@MikeBusuttil MikeBusuttil Dec 27, 2024

Choose a reason for hiding this comment

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

required for audio on WSL2 (ie. in sumulator)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this is a good idea. Native Ubuntu 24.04 is using pipewire instead of pulseaudio; apt wants to remove pipewire when asked to install pulseaudio. This is a big change to user systems that we shouldn't be making. Maybe just add it to the WSL2 wiki.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ya that's fair. I'll try pipewire instead on WSL and make sure that behaves nicely

``` bash
git lfs pull
```

Copy link
Contributor Author

Choose a reason for hiding this comment

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

op.sh takes care of Git LFS (which is now installed with git on Ubuntu 24.04)

Copy link
Collaborator

Choose a reason for hiding this comment

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

We should replace 1-3 with bash <(curl -fsSL openpilot.comma.ai).

@MikeBusuttil MikeBusuttil changed the title Improved experience for new developers: documentation & environment setup Improved new developer experience: documentation & environment setup Dec 27, 2024
Copy link
Contributor

github-actions bot commented Jan 6, 2025

This PR has had no activity for 9 days. It will be automatically closed in 2 days if there is no activity.

@github-actions github-actions bot added the stale label Jan 6, 2025
@MikeBusuttil
Copy link
Contributor Author

yo @adeebshihadeh or @jyoung8607 are you boys able to give me a quick review on this

Comment on lines -51 to -58
## WSL on Windows

[Windows Subsystem for Linux (WSL)](https://docs.microsoft.com/en-us/windows/wsl/about) should provide a similar experience to native Ubuntu. [WSL 2](https://docs.microsoft.com/en-us/windows/wsl/compare-versions) specifically has been reported by several users to be a seamless experience.

Follow [these instructions](https://docs.microsoft.com/en-us/windows/wsl/install) to setup the WSL and install the `Ubuntu-24.04` distribution. Once your Ubuntu WSL environment is setup, follow the Linux setup instructions to finish setting up your environment. See [these instructions](https://learn.microsoft.com/en-us/windows/wsl/tutorials/gui-apps) for running GUI apps.

**NOTE**: If you are running WSL and any GUIs are failing (segfaulting or other strange issues) even after following the steps above, you may need to enable software rendering with `LIBGL_ALWAYS_SOFTWARE=1`, e.g. `LIBGL_ALWAYS_SOFTWARE=1 selfdrive/ui/ui`.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm neutral on moving the WSL stuff to Wiki, but if we're going to move it, we need to preserve the explicit instruction to use Ubuntu 24.04 as the distribution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, but I think it's pretty explicit both here and on the WSL wiki. Do you think it needs to be made more explicit?

Copy link
Collaborator

@jyoung8607 jyoung8607 left a comment

Choose a reason for hiding this comment

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

This should probably be broken into pieces

Comment on lines +44 to +45
libfreeimage-dev \
libfreeimage3 \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's leave these out for now, I'm not sure rerun is moving forward.

@@ -57,7 +59,9 @@ function install_ubuntu_common_requirements() {
opencl-headers \
ocl-icd-libopencl1 \
ocl-icd-opencl-dev \
pocl-opencl-icd \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Required for webcam only, which is a relatively rare need. I just checked on my machine, it wants to drag in 22 dependencies and 845MB of disk. Let's leave it as manual for webcam users.

portaudio19-dev \
pulseaudio \
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this is a good idea. Native Ubuntu 24.04 is using pipewire instead of pulseaudio; apt wants to remove pipewire when asked to install pulseaudio. This is a big change to user systems that we shouldn't be making. Maybe just add it to the WSL2 wiki.

@github-actions github-actions bot removed the stale label Jan 7, 2025
@MikeBusuttil MikeBusuttil marked this pull request as draft January 7, 2025 05:30
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