AVSLab / basilisk

Astrodynamics simulation framework
https://hanspeterschaub.info/basilisk
ISC License
147 stars 61 forks source link

Fix "invalid command `bdist_wheel`" errors during some Conanfile-based installs #778

Closed dpad closed 2 months ago

dpad commented 2 months ago

Description

Fixes an error that could occur during some python conanfile.py installations.

In short, if the user had setuptools>=64.0,<70.1.0 installed, but did not have wheel installed (as it is not required), the python conanfile.py installation could fail with an "invalid command bdist_wheel" type of error. This is fixed by upgrading the minimum build requirement to setuptools>=70.1.0 in both conanfile.py and pyproject.toml.

As a sidenote, this only affected the Conanfile installs because it now installs Basilisk using the pip --no-build-isolation flag. Without the --no-build-isolation flag, the latest version of setuptools is automatically installed temporarily, thus avoiding this issue. However, this is technically a different environment to the one that conanfile.py installs, so I prefer to leave the --no-build-isolation flag in place.   Three unrelated but useful changes are also included, and could be split into a separate merge request if you want. I left them here because they're small and shouldn't affect anything:

Verification

No changes to tests, should continue to pass CI.

Only expected change for users is that they might have their setuptools automatically upgraded when running python conanfile.py. In general, this is a good thing, but it could potentially cause some compatibility issues when installing some older packages.

Documentation

Not required.

Future work

Not required.

schaubh commented 2 months ago

Howdy @dpad , thanks for this contribution. I pulled your branch and built on my macOS system. It builds fine if I have the newer setuptools package installed. However, I originally had setuptools version 69.1.1 installed. Running python conanfile.py auto-updated the package, but then resulted in this error:

Checking Required Python packages:
Found: pandas
Found: matplotlib
Found: numpy<=2.0.1
Found: colorama
Found: tqdm
Found: pillow
Found: pytest
Found: pytest-html
Found: pytest-xdist
Found: 
Found: conan>=1.40.1, <2.00.0
Found: parse>=1.18.0
Collecting setuptools>=70.1.0
  Using cached setuptools-74.0.0-py3-none-any.whl.metadata (6.7 kB)
Using cached setuptools-74.0.0-py3-none-any.whl (1.3 MB)
Installing collected packages: setuptools
  Attempting uninstall: setuptools
    Found existing installation: setuptools 69.1.1
    Uninstalling setuptools-69.1.1:
      Successfully uninstalled setuptools-69.1.1
Successfully installed setuptools-74.0.0
conanfile.py (Basilisk/2.4.4): ERROR: while executing system_requirements(): [Errno 2] No such file or directory: '/Users/hp/Repos/basilisk/.venv/lib/python3.11/site-packages/setuptools-69.1.1.dist-info/METADATA'
ERROR: Error in system requirements
Traceback (most recent call last):
  File "/Users/hp/Repos/basilisk/conanfile.py", line 387, in <module>
    completedProcess = subprocess.run(conanCmdString, shell=True, check=True)
                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Library/Frameworks/Python.framework/Versions/3.11/lib/python3.11/subprocess.py", line 571, in run
    raise CalledProcessError(retcode, process.args,
subprocess.CalledProcessError: Command '/Users/hp/Repos/basilisk/.venv/bin/python -m conans.conan install . --build=missing -s build_type=Release -if dist3/conan -o opNav=False -o vizInterface=True -o buildProject=True -o managePipEnvironment=True' returned non-zero exit status 1.

Next time I run python conanfile.py it runs just fine. There seems to be something off with the version checking here that wile the newer setuptools is installed, it still fails that requirement?

dpad commented 2 months ago

@schaubh

Okay yeah, it seems that any upgrade to setuptools was always going to cause this issue, because pkg_resources comes from setuptools itself. The proper fix is to upgrade to importlib.metadata, as pkg_resources is actually deprecated. This is only supported in Python 3.8+ though, but I think that's now the minimum for Basilisk anyway, so I'll go ahead and make the change.

schaubh commented 2 months ago

That is correct, we currently support python 3.8 to 3.11. We will likely drop 3.8 support at some point when there is a strong reason. Otherwise nice PR with good improvements.

dpad commented 2 months ago

It looks like importlib.metadata doesn't provide the ability to check using the version specifiers (e.g. it'll fail for "numpy<2"). I'll keep the pkg_resources but just reload it after any installation instead. It's still deprecated but I guess we can keep relying on it until setuptools decides to kill it completely :)

dpad commented 2 months ago

Pushed the change above, it should no longer error when you upgrade (e.g. from setuptools==69.1.1 like you had).

schaubh commented 2 months ago

I pulled your latest branch and I still get the same error if I have the older setuptools installed? The CI tests are mostly now failing as well?

dpad commented 2 months ago

That is bizarre. I was able to replicate your issue (installing the same version of setuptools), and with the reload change it all runs perfectly. I'll have a further look.

schaubh commented 2 months ago

🤠 Bizarre indeed, the joins of developing such software. Thanks for the continued support with the Build system and the incremental modernizations.

dpad commented 2 months ago

Ok, I think it might happen if multiple packages are upgraded. I think I just need to re-assign the reloaded module back to pkg_resources variable. Let me give that a go.

dpad commented 2 months ago

Okay, this version seemingly works for multiple upgrades at once. Just had to re-assign the variable properly. (Let's see if it passes CI and on your environment though. All works great on my end, and I'm only using Python 3.4+ features...)

dpad commented 2 months ago

Still no good on the CI... Sigh. Will keep looking into it.

dpad commented 2 months ago

Solution number 2 pushed, and seemingly passes CI. Fixed just by installing the missing packages after checking which are missing, so we don't need to re-use pkg_resources.

For future reference, we shouldn't need to read and install the packages in requirements.txt and requirements_optional.txt anymore, since these are installed automatically by the call to pip install -e . in add_basilisk_to_sys_path(). But I left the current behaviour as is for now.

schaubh commented 2 months ago

This latest branch is now configuring and building well on macOS when the older setuptools is installed. Nice work.

dpad commented 2 months ago

@schaubh Thanks for checking! I've also just taken the opportunity to remove the "parse" requirement, I thought we had removed it in #752 but apparently I forgot to do so. Let me know if you want me to drop any of the commits from this merge request and put everything in a separate one though.

schaubh commented 2 months ago

I'm ok with the series of build improvements in this one PR. Each is easy to review. I'll wait for the CI tests to full pass and then do final review in the morning.