Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Prevent Parallelization on Windows #266

Closed
wants to merge 3 commits into from
Closed

Prevent Parallelization on Windows #266

wants to merge 3 commits into from

Conversation

ooeygui
Copy link

@ooeygui ooeygui commented Sep 25, 2019

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.

Windows has an environment size limitation, which catkin can overflow during nested operations.
This fixes: ms-iot/ROSOnWindows#148
The previous commit had multiple imports on the same line. This caused a CI break.
@ooeygui
Copy link
Author

ooeygui commented Sep 25, 2019

Fix for ms-iot/ROSOnWindows#148

@dirk-thomas
Copy link
Member

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
Copy link
Author

ooeygui commented Sep 26, 2019

@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
Copy link
Member

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
Copy link
Author

ooeygui commented Sep 26, 2019

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
Copy link
Member

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
Copy link
Member

@ooeygui Any update on this?

@dirk-thomas
Copy link
Member

@ooeygui Another friendly ping.

@ooeygui
Copy link
Author

ooeygui commented Nov 14, 2019

@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
Copy link
Member

@ooeygui Ping.

@ooeygui
Copy link
Author

ooeygui commented Jan 11, 2020

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.

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants