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

Switch to using Pixi/Conda for Windows. #802

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

clalancette
Copy link
Contributor

This allows us to much more easily manage dependencies, and to update them in the future.

We accomplish this by first, removing the chef cookbooks. Next, we rewrite the Windows dockerfile to install MSVC, install Connext, install pixi, fetch the dependency list from https://github.com/ros2/ros2 , install those dependencies, and then run the build.

This depends on ros2/ros2#1642 being merged first, as it will provide the file necessary to make this run.

This allows us to much more easily manage dependencies,
and to update them in the future.

We accomplish this by first, removing the chef cookbooks.
Next, we rewrite the Windows dockerfile to install MSVC,
install Connext, install pixi, fetch the dependency list
from https://github.com/ros2/ros2 , install those dependencies,
and then run the build.

Signed-off-by: Chris Lalancette <[email protected]>
.gitmodules Show resolved Hide resolved
job.run(['"%s"' % job.python, '-m', 'pip', '--version'], shell=True)

# Show what pip has
job.run(['"%s"' % job.python, '-m', 'pip', 'list'], shell=True)
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we wouldn't prefer pip freeze --all here in order to get information about all installed packages. I can't say exactly what's excluded without the --all but it is bothersome to me that some things are.

This suggestion is non-blocking.

Suggested change
job.run(['"%s"' % job.python, '-m', 'pip', 'list'], shell=True)
job.run(['"%s"' % job.python, '-m', 'pip', 'freeze', '--all'], shell=True)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, this is a good question. Early on in development here, I found that pip freeze --all just would not work properly in a Conda environment. I seem to remember it giving me either empty or weird results. Because of that, I ended up changing to pip list everywhere.

But it is long enough ago that I now don't remember exactly what the problem was. Let me go back and do some testing here and see if that was either a misconfiguration, or something more serious here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, right. The issue popped up immediately when I tried it locally.

For reasons I don't understand, when installing python packages via pixi/conda, the output from pip freeze --all looks like this:

(pixi_ros2_rolling) C:\dev\rolling>pip freeze --all
argcomplete @ file:///home/conda/feedstock_root/build_artifacts/argcomplete_1698912039670/work
catkin-pkg @ file:///home/conda/feedstock_root/build_artifacts/catkin_pkg_1694651879177/work
cffi @ file:///D:/bld/cffi_1725560655177/work
...

(truncated for brevity). As you can see, the output there doesn't give much useful information about which version of the package is installed, and thus in the CI job output it isn't very helpful. That's why I switched to pip list everywhere.

ros2_batch_job/__main__.py Show resolved Hide resolved
@@ -53,7 +53,7 @@ def show_env(self):
# Show the env
self.run(['export'], shell=True)
# Show what pip has
self.run(['"%s"' % self.python, '-m', 'pip', 'freeze', '--all'], shell=True)
self.run(['"%s"' % self.python, '-m', 'pip', 'list'], shell=True)
Copy link
Member

Choose a reason for hiding this comment

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

Again, I'd actually advocate for freeze --all over just list. What prompted the change?

@@ -36,7 +36,7 @@ def show_env(self):
# Show the env
self.run(['set'], shell=True)
# Show what pip has
self.run([self.python, '-m', 'pip', 'freeze', '--all'])
self.run([self.python, '-m', 'pip', 'list'])
Copy link
Member

Choose a reason for hiding this comment

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

Again, I'd actually advocate for freeze --all over just list. What prompted the change?

windows_docker_resources/Dockerfile Show resolved Hide resolved
Signed-off-by: Chris Lalancette <[email protected]>
@@ -14,10 +14,3 @@
path = windows_docker_resources/rticonnextdds-license
url = [email protected]:osrf/rticonnextdds-license.git
branch = license
[submodule "windows_docker_resources/qtaccount"]
Copy link
Contributor

Choose a reason for hiding this comment

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

These repositories can not be archived/removed are still in use in other places?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think they can probably be archived, but I'd say let's wait to do this for a while until this is all settled.

You can set `CI_ROS_DISTRO` on ci.ros2.org, which will choose the corresponding chef cookbook configuration to test your updates.
If testing on different releases, don't forget to specify the correct ros2.repos file.

#### Testing locally
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that would be great if we can save the testing locally instructions with the new layout. They can be probably simplified a lot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I'll reinstate these with some updates.

windows_docker_resources/Dockerfile Show resolved Hide resolved
@clalancette
Copy link
Contributor Author

Just for completeness, here are CI runs with the latest version here (before making any changes to pin pixi):

  • Rolling Windows Build Status
  • Jazzy Windows Build Status
  • Humble Windows Build Status

@ros-discourse
Copy link

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/upcoming-switch-of-windows-installation-to-pixi-conda/41916/1

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.

4 participants