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

Warning Dialog for ROS 1 EOL. #1843

Open
wants to merge 3 commits into
base: noetic-devel
Choose a base branch
from

Conversation

kscottz
Copy link

@kscottz kscottz commented Dec 4, 2024

image

I've been discussing the ROS 1 EOL with @sloretz. 20% of the ROS community has not upgraded to ROS 2, and some of them seem to be unaware of the impending ROS 1 EOL. We would like to add a series of warnings / notifications to ROS 1 and RViz to encourage users to upgrade to ROS 2. We have made similar changes to Gazebo Classic. This is the first change in a series of changes we would like to add to Noetic in the next few weeks.

Description

This pull request adds a critical dialog on startup to notify users of the ROS 1 EOL date and where they can find migration information. Users can disable the warning by setting the DISABLE_ROS1_EOL_WARNINGS environment variable.

We are still deciding on a final URL location. I will update the pull request when I have that information.

@wjwwood
Copy link
Member

wjwwood commented Dec 4, 2024

👍 from me generally.

I'm not sure how much work it would be, but a "notification toast" which doesn't take focus from the rest of the window might be better. On the other hand it's easy to disable the warnings, so maybe it's fine as proposed.

Could use something like https://github.com/niklashenning/qt-toast or implement something like it manually.

Copy link
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

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

I think the dialog should (also) mention the ROS-O initiative providing ROS1 support for newer Ubuntu releases.

// Get the environment variables
QProcessEnvironment env = QProcessEnvironment::systemEnvironment();
// If environment variable is not set
if(env.contains(QString::fromStdString("DISABLE_ROS1_EOL_WARNINGS")) == false)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if(env.contains(QString::fromStdString("DISABLE_ROS1_EOL_WARNINGS")) == false)
if(env.contains("DISABLE_ROS1_EOL_WARNINGS") == false)

Comment on lines +41 to +43

private Q_SLOTS:
//void onTimer();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
private Q_SLOTS:
//void onTimer();

src/rviz/noetic_eol_dialog.cpp Outdated Show resolved Hide resolved
src/rviz/noetic_eol_dialog.cpp Outdated Show resolved Hide resolved
setIcon(QMessageBox::Critical);
std::stringstream ss;
ss << "ROS 1 goes end-of-life 2025-05-31\n\n";
ss << "Users are encouraged to migrate to ROS 2 as soon as possible!\n\n";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ss << "Users are encouraged to migrate to ROS 2 as soon as possible!\n\n";
ss << "Users are encouraged to migrate to ROS 2 as soon as possible!\n";
ss << "Alternatively, switch to the ROS One distribution\n\n";

Copy link
Author

Choose a reason for hiding this comment

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

This second new line is intentional.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I just shifted the extra newline to the next comment line.

kscottz and others added 2 commits December 4, 2024 09:32
Co-authored-by: Robert Haschke <rhaschke@users.noreply.github.com>
Co-authored-by: Robert Haschke <rhaschke@users.noreply.github.com>
@kscottz
Copy link
Author

kscottz commented Dec 4, 2024

I think the dialog should (also) mention the ROS-O initiative providing ROS1 support for newer Ubuntu releases.

@rhaschke The plan is to point to documentation laying out available migration options for current Noetic users. Our plan is to include ROS-O on that list. I want to keep the notification itself short and sweet as we simply can't provide enough context in warning message.

@kscottz
Copy link
Author

kscottz commented Dec 4, 2024

@wjwwood A toast would be nice but I am leary about pulling in another dependency. Let me see if I can adjust the warning to be non-blocking.

@rhaschke
Copy link
Contributor

rhaschke commented Dec 5, 2024

The plan is to point to documentation laying out available migration options for current Noetic users. Our plan is to include ROS-O on that list.

I think it is important to mention both ROS2 and ROS-O in the warning. Otherwise, if only ROS2 is mentioned, users wanting to stick to ROS1 might not even click the link.

@sloretz
Copy link

sloretz commented Dec 6, 2024

I think it is important to mention both ROS2 and ROS-O in the warning. Otherwise, if only ROS2 is mentioned, users wanting to stick to ROS1 might not even click the link.

Mentioning ROS-O with ROS 2 might give the impression that ROS-O is officially supported by the OSRA, or is continuing development of ROS 1, but ROS-O's goal is only to build on newer distros. IIUC someone interested in ROS-O might also be interested in Canonical's ROS ESM.

How about we limit the dialog to just a link to a page describing the different options (ROS 2, ROS-O, ROS ESM, forking ROS 1, etc)?

ROS Noetic goes end-of-life 2025-05-31

Learn about your options here <some URL TBD>

To disable this dialog, set the DISABLE_ROS1_EOL_WARNINGS environment variable.

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