Skip to content

Conversation

@JohannPally
Copy link

@JohannPally JohannPally commented Nov 2, 2025

Within the spun up Vagrant VM, in a Python REPL, the current implementation evals to False

vagrant@ubuntu-jammy:~$ python
Python 3.10.12 (main, Aug 15 2025, 14:32:43) [GCC 11.4.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import os
>>> print(os.path.isfile("/ardupilot.vagrant"))
False

... with the suggested change

>>> os.path.isdir("/vagrant")
True

@JohannPally JohannPally changed the title fix: sim_vehicle.py's under_vagrant util doesn't appropriately check whether in vagrant VM fix: sim_vehicle.py's under_vagrant util doesn't appropriately check whether the script is run in a Vagrant VM Nov 2, 2025
Copy link
Contributor

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

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

So the Vagrant install scripts are supposed to create the missing file.

Something is obviously not working there!

https://github.com/ardupilot/ardupilot/blob/master/Tools/vagrant/initvagrant.sh#L102
https://github.com/ardupilot/ardupilot/blob/master/Tools/vagrant/initvagrant-trusty64.sh#L42

... that being said, I think the reasons for needing that separate file are history now (IIRC it wasn't always /vagrant as a mountpoint!)

... so I'm inclined to merge this once it passes CI.

You need to fix the commit message, something like "Tools: sim_vehicle.py: look for /vagrant when determining if we are running under Vagrant".

A couple of extra suitably-named commits for removing the lines I referenced above would be a bonus :-)

Copy link
Contributor

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

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

I've fixed the commit message and marked for merge.

Note I was refering to the commit message, not the title of the PR in github

Thanks!

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants