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.

Sign up for GitHub

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

Jump to bottom

Alias dog script names as dogshell #305

Merged
merged 1 commit into from
Jan 28, 2020
Merged

Alias dog script names as dogshell #305

merged 1 commit into from
Jan 28, 2020

Conversation

dwminer
Copy link
Contributor

@dwminer dwminer commented Oct 26, 2018

/usr/bin/dog conflicts with the existing sheepdog package in common
Linux distributions. dogshell seems like a sensible choice since
it's already called that in the source module.

Conan-Kudo
Conan-Kudo previously approved these changes Oct 26, 2018
View reviewed changes
@yannmh yannmh self-requested a review November 2, 2018 20:22
@dabcoder
Copy link
Contributor

dabcoder commented Oct 25, 2019 •
edited
Loading

@dwminer Thanks for this PR and sorry for the long delay in addressing it. This change would not be backward compatible as it would require users to update their scripts/applications/wherever dog and dogwrap are used basically.
Would aliasing any of the dog commands be an alternative solution for this issue?
EDIT: it also looks like sheepdog is no longer maintained: sheepdog/sheepdog#439 (comment).

@dabcoder
Copy link
Contributor

Closing this for now but let us know if you have additional feedback to provide.

@dabcoder dabcoder closed this Oct 28, 2019
@Conan-Kudo
Copy link

@dabcoder Unfortunately, sheepdog is still actively used by libvirt, so it won't be changing anytime soon. Aliases don't help, because the problem is that datadog is trying to use the /usr/bin/dog file, occupied by sheepdog for years.

@dabcoder dabcoder reopened this Oct 28, 2019
@jd
Copy link
Contributor

jd commented Oct 30, 2019

This looks a bit too drastic. This is mostly a packaging issue and it can be easily be solved by distributions by diverting (e.g. see dpkg-divert, by not shipping our dog command or renaming it to dogshell themselves.

I'd suggest adding dogshell, but not removingdog as a first step. Distros are free to ignore the old dog command and not ship it if they don't want too, but regular users installing via pip won't be lost.

@dwminer
Copy link
Contributor Author

dwminer commented Nov 4, 2019

@jd Sure, what we're already doing in the Fedora packaging is renaming dog to dogshell. That works, and it'd be a slight step-up to also have that binary shipped by default. It's not ideal for users installing via pip to potentially have dog from sheepdog overridden in the search path, though. @dabcoder would it make sense to deprecate the dog command now and remove it in some future release?

@github-actions
Copy link

github-actions bot commented Jan 7, 2020

This issue has been automatically marked as stale because it has not had activity in the last 30 days.
Note that the issue will not be automatically closed, but this notification will remind us to investigate why there's been inactivity.

@github-actions github-actions bot added the stale Stale - Bot reminder label Jan 7, 2020
@jirikuncar jirikuncar requested a review from a team as a code owner January 21, 2020 19:41
jirikuncar
jirikuncar suggested changes Jan 21, 2020
View reviewed changes
Copy link
Contributor

@jirikuncar jirikuncar left a comment

Choose a reason for hiding this comment

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

Please add PendingDeprecationWarning to datadog.dogshell:main and datadog.dogshell.wrap:main when sys.argv[0] ends with dog or dogwrap.

@jirikuncar jirikuncar added changelog/Deprecated Deprecated features results into a major version bump community Community driven changes resource/dogshell severity/minor Minor severity issue and removed stale Stale - Bot reminder labels Jan 21, 2020
@dwminer
Add dogshell/dogshellwrap entry points
564564e 
Currently, the 'dog' entrypoint conflicts with a binary of the same name
from sheepdog, a package available in major linux distributions. In
packaging, we've renamed the dog entrypoint from this package to
dogshell to match the source module's name and to avoid conflicts. I
think it makes sense to also add it as an entrypoint here for
consistency. I've marked it as Pending Deprecation, so that in the
future we can transition users to dogshell and retire the dog name.
@dwminer
Copy link
Contributor Author

dwminer commented Jan 25, 2020 •
edited
Loading

Done, @jirikuncar. Thanks for the feedback.

No strong opinion here - would FutureWarning make more sense as a warning to raise?

@jirikuncar
Copy link
Contributor

@dwminer the important difference is that dog and dogshell are not deprecated at the moment, hence Pending....

@jirikuncar
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@jirikuncar jirikuncar changed the title setup.py: Rename script names from 'dog' to 'dogshell' Alias dog script names as dogshell Jan 27, 2020
@zippolyte
Copy link
Contributor

Unrelated test failures, fixed on master, merging

@zippolyte zippolyte merged commit 1550d32 into DataDog:master Jan 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jirikuncar jirikuncar jirikuncar approved these changes

@Conan-Kudo Conan-Kudo Conan-Kudo approved these changes

@yannmh yannmh Awaiting requested review from yannmh

Assignees
No one assigned
Labels
changelog/Deprecated Deprecated features results into a major version bump community Community driven changes resource/dogshell severity/minor Minor severity issue
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@dwminer @dabcoder @Conan-Kudo @jd @jirikuncar @zippolyte