search

PX4 / PX4-Autopilot

PX4 Autopilot Software
https://px4.io
BSD 3-Clause "New" or "Revised" License
8.3k stars 13.43k forks source link

[Bug] Recent changes to homebrew cmake and python defaults break dev environment #23073

Open saosebastiao opened 5 months ago

saosebastiao commented 5 months ago

Describe the bug

Homebrew now defaults to installing Python 3.12, which breaks the PX4 MacOS dev environment.

Homebrew now follows PEP-668 regarding virtual environments. This means that it is expected to use virtual environments like venv or pipx in order to manage dependencies. PX4 should adopt these standards regardless, as there are many reasons why using global or user level dependencies instead of project level dependencies is a bad idea.

And to clarify, this means that the steps to manually install dependencies in the official documentation are broken, as well as the Tools/setup/macos.sh script.

To Reproduce

using homebrew:

brew tap PX4/px4
brew install px4-dev
python3 -m pip install --user pyserial empty toml numpy pandas jinja2 pyyaml pyros-genmsg packaging kconfiglib future jsonschema
make px4_sitl

Expected behavior

Build should succeed

Screenshot / Media

On python3 -m pip install --user pyserial empty toml numpy pandas jinja2 pyyaml pyros-genmsg packaging kconfiglib future the following error message is give:

error: externally-managed-environment

× This environment is externally managed
╰─> To install Python packages system-wide, try brew install
    xyz, where xyz is the package you are trying to
    install.

    If you wish to install a Python library that isn't in Homebrew,
    use a virtual environment:

    python3 -m venv path/to/venv
    source path/to/venv/bin/activate
    python3 -m pip install xyz

    If you wish to install a Python application that isn't in Homebrew,
    it may be easiest to use 'pipx install xyz', which will manage a
    virtual environment for you. You can install pipx with

    brew install pipx

    You may restore the old behavior of pip by passing
    the '--break-system-packages' flag to pip, or by adding
    'break-system-packages = true' to your pip.conf file. The latter
    will permanently disable this error.

    If you disable this error, we STRONGLY recommend that you additionally
    pass the '--user' flag to pip, or set 'user = true' in your pip.conf
    file. Failure to do this can result in a broken Homebrew installation.

    Read more about this behavior here: <https://peps.python.org/pep-0668/>

note: If you believe this is a mistake, please contact your Python installation or OS distribution provider. You can override this, at the risk of breaking your Python installation or OS, by passing --break-system-packages.
hint: See PEP 668 for the detailed specification.
WARNING: Skipping /opt/homebrew/lib/python3.12/site-packages/numpy-1.26.4.dist-info due to invalid metadata entry 'name'

Flight Log

N/A

Software Version

main

Flight controller

sitl

Vehicle type

None

How are the different components wired up (including port information)

No response

Additional context

No response

dagar commented 5 months ago

Thanks @saosebastiao, would you be able to open a pull request fixing this in Tools/setup/macos.sh? Once that settles I can look at updating it for Linux.

saosebastiao commented 5 months ago

@dagar I'd love to take a stab at it. I'll let you know up front that I don't know anything about the codebase, dev practices, MR guidelines, etc. Is there a guide for submitting a first MR to the project? Any example MRs from a project fork (assuming that's how it is done here)?

Also, I don't know a lot about CMake and my own tinkering that led to this bug report leads me to believe we won't be able to do anything with python virtual environments unless we change the CMake build to get rid of the deprecated FindPythonInterp module and start using the recommended FindPython3 module, which allows CMake to use virtual environments via the Python_FIND_VIRTUALENV hint.

I can of course try to do that, but I might be in over my head with CMake, especially on a project that I'm unfamiliar with. 

# Find Python
find_package(PythonInterp 3)
# We have a custom error message to tell users how to install python3.
if(NOT PYTHONINTERP_FOUND)
    message(FATAL_ERROR "Python 3 not found. Please install Python 3:\n"
        "    Ubuntu: sudo apt install python3 python3-dev python3-pip\n"
        "    macOS: brew install python")
endif()

option(PYTHON_COVERAGE "Python code coverage" OFF)
if(PYTHON_COVERAGE)
    message(STATUS "python coverage enabled")
    set(PYTHON_EXECUTABLE coverage run -p)
endif()

include(px4_config)
include(kconfig)
message(STATUS "PX4 config: ${PX4_CONFIG}")
message(STATUS "PX4 platform: ${PX4_PLATFORM}")
DronecodeBot commented 4 months ago

This issue has been mentioned on Discussion Forum for PX4, Pixhawk, QGroundControl, MAVSDK, MAVLink. There might be relevant details there:

https://discuss.px4.io/t/px4-sync-q-a-may-15-2024/38773/1

saosebastiao commented 4 months ago

In order to transition to a virtualenv environment, we do need to transition to the newer cmake FindPython3 system. I have a partially working branch in progress here: https://github.com/saosebastiao/PX4-Autopilot/tree/fix_macos_homebrew_python_env

This seems to work for make px4_sitl, but it fails for anything on a nuttx platform. I can't seem to find what is going wrong. 

[2/1211] Generating nuttx/mm/libmm.a
FAILED: NuttX/nuttx/mm/libmm.a /Users/danieltoone/ws/PX4-Autopilot/build/px4_fmu-v6xrt_default/NuttX/nuttx/mm/libmm.a 
cd /Users/danieltoone/ws/PX4-Autopilot/platforms/nuttx/NuttX/nuttx && /opt/homebrew/Cellar/cmake/3.29.3/bin/cmake -E remove -f /Users/danieltoone/ws/PX4-Autopilot/platforms/nuttx/NuttX/nuttx/mm/libmm.a && find mm -type f -name *.o -delete && make -C mm --no-print-directory --silent libmm.a TOPDIR="/Users/danieltoone/ws/PX4-Autopilot/platforms/nuttx/NuttX/nuttx" KERNEL=n EXTRAFLAGS= && /opt/homebrew/Cellar/cmake/3.29.3/bin/cmake -E copy_if_different /Users/danieltoone/ws/PX4-Autopilot/platforms/nuttx/NuttX/nuttx/mm/libmm.a /Users/danieltoone/ws/PX4-Autopilot/build/px4_fmu-v6xrt_default/NuttX/nuttx/mm/libmm.a
clang: warning: optimization flag '-fno-strength-reduce' is not supported [-Wignored-optimization-argument]
warning: unknown warning option '-Wlogical-op'; did you mean '-Wlong-long'? [-Wunknown-warning-option]
warning: unknown warning option '-Wno-format-truncation' [-Wunknown-warning-option]
warning: unknown warning option '-Wno-maybe-uninitialized'; did you mean '-Wno-uninitialized'? [-Wunknown-warning-option]
warning: unknown warning option '-Wno-stringop-truncation'; did you mean '-Wno-string-concatenation'? [-Wunknown-warning-option]
warning: unknown warning option '-Wno-logical-op'; did you mean '-Wno-long-long'? [-Wunknown-warning-option]
warning: unknown warning option '-Wno-nonnull-compare' [-Wunknown-warning-option]
warning: unknown warning option '-Wno-old-style-declaration'; did you mean '-Wno-out-of-line-declaration'? [-Wunknown-warning-option]
In file included from mm_heap/mm_initialize.c:33:
/Users/danieltoone/ws/PX4-Autopilot/platforms/nuttx/NuttX/nuttx/mm/mm_heap/mm.h:207:1: error: static assertion failed due to requirement 'sizeof(struct mm_freenode_s) <= (1 << (4))': Error size for struct mm_freenode_s

static_assert(SIZEOF_MM_FREENODE <= MM_MIN_CHUNK,
^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/danieltoone/ws/PX4-Autopilot/platforms/nuttx/NuttX/nuttx/include/assert.h:81:27: note: expanded from macro 'static_assert'
#    define static_assert _Static_assert
                          ^
/Users/danieltoone/ws/PX4-Autopilot/platforms/nuttx/NuttX/nuttx/mm/mm_heap/mm.h:207:34: note: expression evaluates to '32 <= 16'
static_assert(SIZEOF_MM_FREENODE <= MM_MIN_CHUNK,
              ~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~
7 warnings and 1 error generated.
make[1]: *** [bin/mm_initialize.o] Error 1
[8/1211] Generating nuttx/sched/libsched.a
FAILED: NuttX/nuttx/sched/libsched.a /Users/danieltoone/ws/PX4-Autopilot/build/px4_fmu-v6xrt_default/NuttX/nuttx/sched/libsched.a 
cd /Users/danieltoone/ws/PX4-Autopilot/platforms/nuttx/NuttX/nuttx && /opt/homebrew/Cellar/cmake/3.29.3/bin/cmake -E remove -f /Users/danieltoone/ws/PX4-Autopilot/platforms/nuttx/NuttX/nuttx/sched/libsched.a && find sched -type f -name *.o -delete && make -C sched --no-print-directory --silent all TOPDIR="/Users/danieltoone/ws/PX4-Autopilot/platforms/nuttx/NuttX/nuttx" KERNEL=y EXTRAFLAGS=-D__KERNEL__ && /opt/homebrew/Cellar/cmake/3.29.3/bin/cmake -E copy_if_different /Users/danieltoone/ws/PX4-Autopilot/platforms/nuttx/NuttX/nuttx/sched/libsched.a /Users/danieltoone/ws/PX4-Autopilot/build/px4_fmu-v6xrt_default/NuttX/nuttx/sched/libsched.a

There is a lot of noise that happens after this error, then it reports the same thing for several other nuttx files. 

make px4_fmu-v6xrt_default | grep FAILED

FAILED: NuttX/nuttx/mm/libmm.a /Users/danieltoone/ws/PX4-Autopilot/build/px4_fmu-v6xrt_default/NuttX/nuttx/mm/libmm.a 
FAILED: NuttX/nuttx/sched/libsched.a /Users/danieltoone/ws/PX4-Autopilot/build/px4_fmu-v6xrt_default/NuttX/nuttx/sched/libsched.a 
FAILED: NuttX/nuttx/libs/libc/libc.a /Users/danieltoone/ws/PX4-Autopilot/build/px4_fmu-v6xrt_default/NuttX/nuttx/libs/libc/libc.a 
FAILED: NuttX/nuttx/arch/arm/src/libarch.a /Users/danieltoone/ws/PX4-Autopilot/build/px4_fmu-v6xrt_default/NuttX/nuttx/arch/arm/src/libarch.a 
FAILED: NuttX/nuttx/net/libnet.a /Users/danieltoone/ws/PX4-Autopilot/build/px4_fmu-v6xrt_default/NuttX/nuttx/net/libnet.a 
FAILED: NuttX/nuttx/fs/libfs.a /Users/danieltoone/ws/PX4-Autopilot/build/px4_fmu-v6xrt_default/NuttX/nuttx/fs/libfs.a 
FAILED: NuttX/nuttx/drivers/libdrivers.a /Users/danieltoone/ws/PX4-Autopilot/build/px4_fmu-v6xrt_default/NuttX/nuttx/drivers/libdrivers.a
saosebastiao commented 4 months ago

I noticed via this issue that nuttx is an older version and is using a custom px4 build system fork to use cmake which nuttx didn't have at the time. Could an upgrade here possibly help?

MaEtUgR commented 4 months ago

Interesting. I just freshly installed the toolchain from homebrew on my Macbook Air M1 and successfully built SITL and NuttX targets. For the python modules I used the --break-syste-apackages workaround which is not ideal but we need to find a general virtual environment solution since all distributions are updating python. First one was Arch Linux, see https://github.com/PX4/PX4-Autopilot/pull/21340/files#diff-4ce953c96a002a55eb9773fd29372540c47d736556a63852df9f6950991e6149R70

Let's follow up on your https://github.com/PX4/PX4-Autopilot/pull/23138 to get the virtual environment rolling. I didn't look into details yet and wasn't aware that it needs a different CMake plugin. But I expected there to be some work since the build environment needs to somehow load the virtual environment.

saosebastiao commented 4 months ago

Hi @MaEtUgR thanks for the followup. Unless I'm understanding incorrectly, the arch script doesn't use virtual environments. The --break-system-packages flag bypasses the virtual environment and installs packages in the system environment, which is essentially doing the the same thing as the previous user-level environment installation of packages. If it is using virtual environments I would expect to see the creation of a virtual environment with one of the following:

python3 -m venv {venv_name}
//or 
virtualenv {venv_name}

and then a subsequent call of source {venv_name}/bin/activate to activate the virtual environment before running the pip install .... I think by using the --break-system-packages flag, we're essentially getting the build to work by using an unrecommended escape hatch to preserve the old behavior.

In my merge request, I modified the scripts to create the virtual environment and activate it, install the packages, and then I deactivate it within the script. I chose to deactivate within the script because there are weird issues with activating a virtual environment within a script that make it hard to deactivate outside of that script.

The new FindPython3 CMake module has the capability of detecting a python virtual environment, but it must already be activated for it to be identified. From my understanding, the activation of the virtual environment can be emulated by setting the VIRTUAL_ENV environment variable, so I put in some code to do that so there isn't a need for manual activation step before running. I'm also gonna look into whether there is more logic behind the activation script which might need to be replicated.

saosebastiao commented 3 months ago

@MaEtUgR Would you happen to have merge access levels for the PX4-containers repo? I can't seem to get anybody's attention for this PR which would be required for any work on virtual environments.