search

ros-infrastructure / catkin_pkg

Standalone Python library for the catkin build system.
https://github.com/ros/catkin
Other
47 stars 91 forks source link

Prevent Parallelization on Windows #266

Closed ooeygui closed 4 years ago

ooeygui commented 4 years ago

Windows has a total environment size limitation, which catkin can overflow during nested operations. This change prevents creating a nested catkin operation for exceptionally large work spaces.

ooeygui commented 4 years ago

Fix for https://github.com/ms-iot/ROSOnWindows/issues/148

dirk-thomas commented 4 years ago

For the record: this is a re-do of #265. (Please add references like this in the future otherwise all the previous conversation on this topic is not accessible.)

This change prevents creating a nested catkin operation for exceptionally large work spaces.

It doesn't seem that the patch is doing what you are describing. It unconditionally disables parallelization on Windows?

Also I don't see how parsing of package.xml files is related to any environment limitations?

ooeygui commented 4 years ago

@dirk-thomas Thank you for the feedback.

For ROS1/catkin, this change will universally disable paralleization on Windows. I believe this is a rare scenario (although correct me if this is an invalid assumption). The previous proposed fix affected both Linux and Windows, whereas this change only affects Windows.

dirk-thomas commented 4 years ago

this change will universally disable paralleization on Windows.

The question is why?

I believe this is a rare scenario (although correct me if this is an invalid assumption).

What scenario are you referring to?

The previous proposed fix affected both Linux and Windows, whereas this change only affects Windows.

Please reference the fix you are referring to. All linked older tickets (#265, #251, #250) were specific to Windows?

ooeygui commented 4 years ago

this change will universally disable paralleization on Windows.

The question is why? I believe you are asking two different questions with that statement, so I'll answer both:

  1. We're disabling it on Windows to unblock customers who cannot use this today with a large workspace.
  2. Why does it fail? Python multiprocessing creates a new process which doesn't inherit the environment correctly; part of this is due to limitations in environment variables.

I believe this is a rare scenario (although correct me if this is an invalid assumption).

What scenario are you referring to?

I believe it is a rare scenario where a development workspace contains more than 100 projects. (With the customer we are engaged with, dependencies are install in the host workspace, so the development workspace is quite small.)

The previous proposed fix affected both Linux and Windows, whereas this change only affects Windows.

Please reference the fix you are referring to. All linked older tickets (#265, #251, #250) were specific to Windows?

Please ignore this comment. I did not see the history.

dirk-thomas commented 4 years ago

Python multiprocessing creates a new process which doesn't inherit the environment correctly; part of this is due to limitations in environment variables.

Can you provide more context on this? Why can an existing environment not be replicated in a new process? (Since we are not talking about extending any environment variables.) Can you reference any resources which document that specific limitation / bug.

I believe it is a rare scenario where a development workspace contains more than 100 projects.

Ok, I wouldn't call this case rare though. Building a ROS distribution from source commonly involves more packages that this threshold and that is a very common process in the ROS community.

dirk-thomas commented 4 years ago

@ooeygui Any update on this?

dirk-thomas commented 4 years ago

@ooeygui Another friendly ping.

ooeygui commented 4 years ago

@dirk-thomas Thanks for the ping. I ended up getting quick sick after ROSCon; I'm on the mend and am digging out.

dirk-thomas commented 4 years ago

@ooeygui Ping.

ooeygui commented 4 years ago

I'm going to close this issue. We have a different solution which would keep parallization. Python multiprocessing doesn't handle the .exe install - it is specifically looking for .py. Since catkin_make doesn't do anything on windows, I think we can install it as catkin_make.py and have it work correctly.