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

Add wildcard OS_NAME to REP 111 #329

Merged
merged 1 commit into from
Dec 3, 2021
Merged

Conversation

cottsay
Copy link
Member

@cottsay cottsay commented Nov 13, 2021

The rosdep/python.yaml file contains many rules which resolve only to pip packages. In general, pip is supported on nearly all modern platforms and the rules in that file generally apply to all of them. To ensure that these rules are available to new platforms as they are added and also to those which aren't listed specifically each time a new -pip rule is added, I'm proposing support for OS_NAME wildcards in rosdep.

For example, I propose this syntax:

python-backoff-pip:
  '*':
    pip:
      packages: [backoff]

...rather than the current structure:

python-backoff-pip:
  arch:
    pip:
      packages: [backoff]
  debian:
    pip:
      packages: [backoff]
  fedora:
    pip:
      packages: [backoff]
  opensuse:
    pip:
      packages: [backoff]
  osx:
    pip:
      packages: [backoff]
  ubuntu:
    pip:
      packages: [backoff]

From the proposal:

Some package managers are supported and function on more than one platform, and the names of packages in those package managers are typically the same between platforms. To avoid duplicating the rules under several OS_NAME
stanzas the OS_NAME can be specified as * as long as the rule under it specifies the PACKAGE_MANAGER explicitly.

Similar to rule lookups regarding a wildcard OS_VERSION, an explicit OS_NAME entry will take precedence over * rules entirely and an explicitly null PACKAGE_ARGUMENT for an OS_NAME will omit it from the wildcard.

Implementation is in ros-infrastructure/rosdep#838

Copy link
Member

@tfoote tfoote left a comment

Choose a reason for hiding this comment

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

Adding this support will help us significantly reduce the complexity of the rules. Thus will make maintenance easier. And the cost of extra complexity of the parsing is a one time cost. This makes sense to me.

@nuclearsandwich
Copy link
Contributor

I don't have any concerns about the proposed enhancement itself. But I think there are some challenges with its use and I'm not sure what the best way to handle them will be.

According to the contribution guidelines for rosdep rules the pip installer isn't supported on Gentoo. The wildcard match will change the failure mode on Gentoo from the rosdep key not having a definition for the platform to, hopefully, an error trying to use the pip installer as opposed to trying unsuccessfully to complete installation via pip.

With that in mind the standard form for pip keys might have to become something like

python-backoff-pip:
  '*':
    pip:
      packages: [backoff]
  gentoo: null

Since the rosdep definition files aren't versioned. What sort of rollout plan do you have once this REP change is agreed upon and a rosdep version that supports this change is out? Put another way, what happens when rosdep without ros-infrastructure/rosdep#838 tries to cache a file containing an OS name wildcard?

Copy link
Contributor

@nuclearsandwich nuclearsandwich left a comment

Choose a reason for hiding this comment

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

Some questions about rollout and eventual usage but neither of them block integrating the actual syntax change.

@cottsay
Copy link
Member Author

cottsay commented Nov 17, 2021

Thanks for putting some thought into how we move forward with this.

According to the contribution guidelines for rosdep rules the pip installer isn't supported on Gentoo. The wildcard match will change the failure mode on Gentoo from the rosdep key not having a definition for the platform to, hopefully, an error trying to use the pip installer as opposed to trying unsuccessfully to complete installation via pip.

Actually, this isn't the case. The message displayed for a platform which doesn't support the package manager listed under the wildcard rule would be the same as if it were nulled-out like you proposed. As is expected, pip is not supported for Gentoo in rosdep, so a typical key resolution error would be presented - no change in behavior.

Effectively, the wildcard rules only match an OS where the package manager defined for the rule is supported by that OS. Should I update the verbiage here to reflect that, or do you view it as an implementation detail?

What sort of rollout plan do you have...?

Get it stable on the buildfarm, pip, and Fedora/EPEL, which covers all of the sources recommended by the wikis (as far as I know), and then start using it for new rule contributions shortly thereafter. Cleaning up the existing rules should probably wait a while. If I had to choose a date to do the clean-up, I would think 6 months from now would be sufficient time for the release to have reasonably propagated. I'm open to suggestions here.

...what happens when rosdep without ros-infrastructure/rosdep#838 tries to cache a file containing an OS name wildcard?

The update will succeed and the rules without the OS wildcard will function as before. Attempting to resolve an OS wildcard rule will result in a generic key resolution error. Put another way, rosdep will just think that there's a rule for an OS named * and ignore it.

@cottsay cottsay merged commit ec7531d into master Dec 3, 2021
@cottsay cottsay deleted the cottsay/rep111-os-wildcards branch December 3, 2021 23:56
@cottsay cottsay mentioned this pull request Dec 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants