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

User-friendly skip component warning #165

Merged
merged 5 commits into from
Apr 28, 2021
Merged

User-friendly skip component warning #165

merged 5 commits into from
Apr 28, 2021

Conversation

mxgrey
Copy link
Contributor

@mxgrey mxgrey commented Apr 26, 2021

Summary

This is an alternative proposal for #164.

The idea here is that ign-cmake assumes users might want to build all optional components unless the user explicitly specifies SKIP_<component> to cmake. We allow the configuration to succeed even if optional components need to be skipped due to missing dependencies, but we issue a clear (but friendly) warning to the user about which optional components will be skipped and why they are being skipped. We also provide an instruction for how to suppress the warning in case they find it bothersome.

I recommend we issue warnings rather than status messages because status messages are easily overlooked, especially when building with colcon whose default behavior is to send all status messages into the build log instead of displaying them to users. We provide a straightforward way to suppress these warnings, so users who don't like them can easily turn them off. Also this PR modifies the tone of the warning so no one should be left with the impression that their configuration is broken due to optional components being skipped.

As a side-effect this PR also cleans up the way warnings are displayed to users. It turns out cmake's rules for formatting messages are rather complex, and our approach to generating the warning messages was creating a lot more noise than we intended.

Here's an example of someone configuring ign-rendering4 off of this PR when they are missing the dependencies of ignition-rendering-optix and ignition-rendering-ogre2:

   CONFIGURATION WARNINGS:
   -- Skipping component [ogre2]: Missing dependency [IgnOGRE2] (Components: HlmsPbs, HlmsUnlit, Overlay).
      ^~~~~ Set SKIP_ogre2=true in cmake to suppress this warning.

   -- Skipping component [optix]: Missing dependency [OptiX].
      ^~~~~ Set SKIP_optix=true in cmake to suppress this warning

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

@github-actions github-actions bot added 🏢 edifice Ignition Edifice 🏯 fortress Ignition Fortress 🏰 citadel Ignition Citadel 🔮 dome Ignition Dome labels Apr 26, 2021
Copy link
Contributor

@adlarkin adlarkin 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 this a good way to solve the issue of suppressing warnings of optional dependencies. Thanks @mxgrey!

@mxgrey
Copy link
Contributor Author

mxgrey commented Apr 26, 2021

I'll just note the catch here: To get the build farm to have zero warnings when it doesn't have the OptiX dependency, we'll need to add -DSKIP_optix to the cmake command in the build farm script. Since the build farm is explicitly intended to not build optix, I think it's sensible to set up its cmake invocation to explicitly skip the optix component.

@adlarkin
Copy link
Contributor

I'll just note the catch here: To get the build farm to have zero warnings when it doesn't have the OptiX dependency, we'll need to add -DSKIP_optix to the cmake command in the build farm script. Since the build farm is explicitly intended to not build optix, I think it's sensible to set up its cmake invocation to explicitly skip the optix component.

Agreed 👍 I mentioned this in gazebosim/gz-rendering#309 (comment), but it's good to mention that here as well so that we can update the buildfarm script if/when this PR gets merged.

Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

I like the skip mechanism 👍

I still don't agree that we need to print warnings despite the user having explicitly requested QUIET though.

I don't think of REQUIRED_BY as similar to REQUIRED, so I don't see any conflicts in using QUIET and REQUIRED_BY together. My understanding was:

  • REQUIRED: if not present, the entire application fails to build.
  • REQUIRED_BY: if not present, that component fails to build. Whether this is ok to ignore should be configured at the component level.

Does this sound reasonable?

cmake/IgnConfigureBuild.cmake Show resolved Hide resolved
cmake/IgnConfigureBuild.cmake Show resolved Hide resolved
cmake/IgnUtils.cmake Show resolved Hide resolved
cmake/IgnConfigureBuild.cmake Show resolved Hide resolved
@chapulina
Copy link
Contributor

chapulina commented Apr 26, 2021

Thinking a bit more about this, I think we should consider an opt-in mechanism rather than opt-out (i.e. REQUIRE_optix instead of SKIP_optix). Due to the nature of our software and how it provides various options to the user, I suspect that in the long run, warnings can be ignored more often than they need to be addressed. i.e. you only need to compile one rendering engine, one physics engine, etc.

I believe this could be handled a bit more cleanly if we kept engine implementations in their own repositories. So an ign-rendering user would only add to their workspace the core ign-rendering, and the specific implementations they're interested in (i.e. ign-rendering-ogre2).

@mxgrey
Copy link
Contributor Author

mxgrey commented Apr 27, 2021

(Also responding to this comment)

I agree that it's a tough balancing act between these conflicting concerns:

  • Minimizing the noise that the user has to deal with
  • Keeping the user informed about important configuration conditions
  • Inferring what the user's intention is

This is especially hard since we aim to serve a wide range of users from very early beginners to very advanced experts. I think it may be valuable to imagine some potential user stories and how each user is affected by opt-in versus opt-out versus separate repos.


Expert build manager

Description

I am managing a CI/CD pipeline for my company's simulation tools, which includes various ignition packages. My company doesn't need every optional component of every ignition package, and it would be a waste of resources for us to build all of them with every CI test.

Additionally I'm stickler for making sure my builds always come out totally clean. I will interpret any warning in my CI pipeline as an error and demand that it be fixed.

Flag opt-in

I set up my build scripts to include -DREQUIRE_...=true for each optional component that we do want to have and ignore the optional components that we don't care about.

Separate repo

I add whatever optional component repos that I need to my build script and ignore the repos that I don't need.

Flag opt-out

I set up my build scripts to include -DSKIP_...=true for each optional component that we don't care about.

Summary

The user experience is roughly the same no matter which option we go with. Every option would require the user to configure their build script to be explicit about what they do or do not want. We could try to take guesses at whether this type of user is statistically more likely to require more optional components than they want to skip, and then figure out which option will require fewer keystrokes for the greatest number of this type of user, but I think that's a very minor quantitative quality of life concern and would not make a meaningful qualitative difference. No matter what, this user is committed to explicitly configuring the packages that they need, and they have the knowledge necessary to do it in any of these ways.


Curious beginner

Description

I'm someone who works in the robotics industry, and I'm curious about trying out a robot simulator to help guide our robot platform development practices. I heard that ignition is free to use, so I'm giving it a try. I don't have much experience with building modular software projects from source code, but I've used cmake and make enough that I think I can get through it.

I'm trying to use ignition out of my own personal curiosity to see if it can help out my business; I'm not being forced to use it by my employer. But if it goes well, I might advocate for its use at my company to help us design and test our design concepts.

Flag opt-in

I've retrieved and built the set of repositories that supposedly allow me to run ignition-gazebo. After spending some time wrestling with some missing dependencies, I've gotten all of the packages to configure and build successfully.

I try to run an example with ign gazebo, but I get a runtime error saying that ignition-rendering4-ogre2 is missing. I don't know what that is. I look back at my ignition-rendering4 build output and don't see anything wrong. I do a web search for ignition-rendering4-ogre2 and it just leads me back to ignition-rendering. At this point I've spent a considerable amount of time to get everything built successfully, and it still can't run. I see that there is a lot of documentation on ignitionrobotics.org, but I don't know where to look to solve this problem, and I no longer consider this to be worth the effort I'm spending.

Separate repo

I've retrieved and built the set of minimal repositories that supposedly allow me to run ignition-gazebo. After spending some time wrestling with some missing dependencies, I've gotten all of the packages to configure and build successfully.

I try to run an example with ign gazebo, but I get a runtime error saying that ignition-rendering4-ogre2 is missing. I don't know what that is. I look back at my ignition-rendering4 build output and don't see anything wrong. I do a web search for ignition-rendering4-ogre2 and find a new repo called ignition-rendering-ogre2. I add that to my workspace and try to rebuild. I sort out the missing dependencies for this new package, and then the configuration and build steps are successful.

I try to run an example with ign gazebo again, but now I get a runtime error saying that ignition-physics3-dartsim is missing. I don't know what that is. At this point I'm wondering just how many pieces are secretly missing before I'll be able to successfully run ign gazebo. Is it just this one more package or are there a dozen more that I'll need to find and build before I can even run a simulation? I no longer consider this to be worth the effort I'm spending.

Flag opt-out

I've retrieved and built the set of repositories that supposedly allow me to run ignition-gazebo. After spending some time wrestling with some required dependencies, I've gotten all of the packages to configure and build successfully. I noticed there are some "component" packages that are being skipped, but the configuration and build steps were successful, so I'll just ignore those.

I try to run an example with ign gazebo, but I get a runtime error saying that ignition-rendering4-ogre2 is missing. I remember the package ignition-rendering4 said it was skipping a component called ogre2. I look back at the log and see that the ogre2 component had some missing dependencies. I install those dependencies and rebuild the package.

I try to run an example with ign gazebo again, but now I get a runtime error saying that ignition-physics3-dartsim is missing. I remember that the ignition-physics3 package had a dartsim component that got skipped. At this point I'm mildly annoyed at all the extra steps I need to take, but I know there were only a handful of components that were skipped by the configuration because there was a clear warning about each one, so there can't be very many components remaining.

I keep installing component dependencies and rebuilding until the ign gazebo example runs successfully.

Summary

Depriving a non-expert user of information makes it impossible for them to forecast the remaining steps to get everything working. This can be especially bad when it's unclear to them why things aren't working in the first place. Runtime warnings alone are not necessarily sufficient to help a user understand that they need to change the way they were building our packages. The frustration that users feel from being kept in the dark may cause them to abandon their attempt to use our software.


Experienced ignition user

Description

I use ignition regularly to simulate robots. Ignition has many packages whose versions change frequently, so I'm often using different ignition distributions at the same time in different colcon workspaces to keep them separate. Some of my old work requires old distributions while my more recent work requires newer ignition features, and I need to keep it all running at once.

This means I'll periodically be creating new colcon workspaces to run newer versions of ignition.

Flag opt-in

I clone all the repos for my new workspace using the new distro's .repos file. I run colcon build and let it finish successfully. I already have all the ignition dependencies from my previous builds of ignition, so this is no problem.

I go to run some of my projects, but ign gazebo crashes complaining about missing plugins. Oh yeah, I need to explicitly opt in to each plugin component. Luckily I already have a colcon.meta file in my other colcon workspace that specifies which components I want to opt into. I copy that file into my new colcon workspace and rebuild. Now my projects are working again.

Separate repo

I clone all the repos for my new workspace using the new distro's minimal required .repos file. I run colcon build and let it finish successfully. I already have all the ignition dependencies from my previous builds of ignition, so this is no problem.

I go to run some of my projects, but ign gazebo crashes complaining about missing plugins. Oh yeah, there are additional repos I need which aren't considered required parts of the distribution. I run ls on my new workspace and my old workspace to try to eyeball which packages are missing. One by one I manually git clone the missing repos.

With all the optional repos cloned, I rerun colcon build and wait for it to succeed. Now my projects are working again.

Flag opt-out

I clone all the repos for my new workspace using the new distro's .repos file. I run colcon build and let it finish successfully. I already have all the ignition dependencies from my previous builds of ignition, so this is no problem. colcon shows me a few configuration warnings, but I already know that I don't need those optional components, so I ignore them. The warnings won't show up again unless I force cmake to run again, so I'm not bothered by them.

I go to run some of my projects, and they work immediately because all the plugin components that I needed are built.

Summary

Requiring the user to explicitly opt in to optional components will create unavoidable extra steps for them to get ignition-gazebo running correctly. If we instead assume that the user wants to build everything and issue warnings when something can't be built, then no extra steps are needed for ignition-gazebo to run correctly. Extra steps would only be needed if the user wants to suppress the configuration warnings, and those optional extra steps for Flag opt-out are no worse than the mandatory extra steps for Flag opt-in or Separate repo.


Overall summary

The opt-in designs (either "Flag opt-in" or "Separate repos") create mandatory burdens on users, and that burden may be invisible (and therefore unsolvable) to users who are unfamiliar with the overall design of ignition.

The opt-out design has objectively less burden in terms of getting ign gazebo to work as intended, and only introduces any burden if users want to suppress some warnings that will ordinarily only print out once when the package first gets configured. This optional burden of the opt-out design is never more significant than the mandatory burden imposed by the opt-in designs.

In my mind the choice is between an "optional burden" and a "greater or equal mandatory burden". For me the balance between those choices seems very clear.

@mxgrey
Copy link
Contributor Author

mxgrey commented Apr 27, 2021

I still don't agree that we need to print warnings despite the user having explicitly requested QUIET though.

One thing I'll point out here is that "user" is an overloaded word when it comes to configuration scripts. A configuration script that uses ign_find_package is actually being written by an ignition developer, not by an end user. The end user of the script will interact with the script by calling cmake and passing in cmake arguments from the command line, or setting variables in the cmake cache. End users generally don't (and shouldn't) be modifying the content of a configuration script.

Therefore when REQUIRED_BY and QUIET are used together, that's not an end user asking for ign_find_package to be silent; that's an ignition developer imposing their own preference about information output onto the end user.

This is why I believe that the verbosity of an ign_find_package call should really be set through cmake arguments/variables, not hard-coded into the script. I would generally discourage the practice of us ever passing QUIET into ign_find_package for that reason, but I'm not opposed to it enough to remove support for it entirely.

Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

Thanks for iterating, @mxgrey . Let's go with this approach and revisit if it becomes burdensome for users.

@mxgrey mxgrey merged commit 17b7058 into ign-cmake2 Apr 28, 2021
@mxgrey mxgrey deleted the skip_component-2 branch April 28, 2021 06:16
@mxgrey
Copy link
Contributor Author

mxgrey commented Apr 28, 2021

To forward port this to main, I have a branch called skip_component that branches off of main and cherry picks these changes in. Should we open a PR for that, or would you rather merge all of ign-cmake2 forward into main?

@adlarkin
Copy link
Contributor

I think it'd probably be best to merge all of ign-cmake2 forward to main, since we try to do that periodically anyways. It looks like merging forward ign-cmake2 to main shouldn't be too difficult: main...ign-cmake2

@mxgrey, if you'd like me to open a PR to complete this merge forward, just let me know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏰 citadel Ignition Citadel 🔮 dome Ignition Dome 🏢 edifice Ignition Edifice 🏯 fortress Ignition Fortress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants