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

Drop warning about using the inn symlink #266

Merged
merged 1 commit into from
Jun 2, 2023
Merged

Conversation

strohel
Copy link
Member

@strohel strohel commented Jun 2, 2023

Follow-up to just-merged #175. We no longer install the inn symlink, so users doing that manually should know what they are doing.

Follow-up to just-merged #175. We no longer install the `inn` symlink, so users doing that manually should know what they are doing.
@strohel strohel requested review from mcginty and bschwind June 2, 2023 06:49
@alerque
Copy link
Contributor

alerque commented Jun 2, 2023

I would actually have held off on this because while end users that make their own links probably know what they are up to, but distro packagers (I'm deep into that world!) are sometimes a little less attentive and I think there are still distro packages that will add this link out of the box and might go on doing so for a while until somebody brings it to their attention.

In any case this PR is obsolete since it was tacked onto #175, and I would treat it like water under the bridge. If any action is taken the best thing to do is probably run through the packaging that is out there (Repology is good for finding them) and directly submit bug reports to any that haven't removed the link.

@strohel
Copy link
Member Author

strohel commented Jun 2, 2023

@alerque no problem with keeping this warning for a bit more time (releases). The intention was to keep the codebase clean from historic cruft (distro users shuold have been seeing fir warning for over a year).

In any case this PR is obsolete since it was tacked onto #175, and I would treat it like water under the bridge.

Sorry not sure what you mean by this?

@alerque
Copy link
Contributor

alerque commented Jun 2, 2023

I read the other PR wrong, it was a mention not a commit pushed to the branch. Disregard.

As for timing you might be right, I forgot this has been in several releases already along with the documentation update, it was only the packaging generated here that finally got fixed. Sure go for it...

Copy link
Member

@bschwind bschwind left a comment

Choose a reason for hiding this comment

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

LGTM!

@mcginty
Copy link
Collaborator

mcginty commented Jun 2, 2023

I definitely see the usefulness in keeping a warning around, but since we're just checking argv here any user-defined symlinks or aliases would trigger it, which would get really annoying.

I'd imagine our options are to remove it like this, or at least add a new --suppress-inn-warning type argument. Personally, agree I think the first is fine at this point.

@strohel
Copy link
Member Author

strohel commented Jun 2, 2023

Thanks everyone for the quick discussion iterations. I'll go ahead and merge this as-is.

Let the inn symlink rest in peace. 🪦

@strohel strohel merged commit b3a9718 into main Jun 2, 2023
9 checks passed
@strohel strohel deleted the drop-inn-symlink-warning branch June 2, 2023 07:56
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.

None yet

4 participants