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

No direct migration path for whoami::hostname #117

Closed
jsgf opened this issue Sep 26, 2024 · 5 comments
Closed

No direct migration path for whoami::hostname #117

jsgf opened this issue Sep 26, 2024 · 5 comments

Comments

@jsgf
Copy link

jsgf commented Sep 26, 2024

Is your feature request related to a problem? Please describe.
whoami::hostname is deprecated. The suggested replacement is whoami::fallible::hostname, but this is not functionally identical. In principle the case of hostname shouldn't matter but in practice it means that we can't just directly replace calls to whoami::hostname() to whoami::fallible::hostname().unwrap_or_else(|| "localhost".to_string()) (or just expect/unwrap) without worrying about introducing a regression.

Describe the solution you'd like
Maybe whoami::fallible::hostname_lower() which is functionally identical to the infallible version (aside from returning a Result).

Describe alternatives you've considered
Open-coding a replacement in a local library, which is awkward. Disable warnings/errors for deprecated functions.

Additional context
I'm trying to migrate a large amount of code from 1.4 to 1.5 to work around the security issue.

@AldaronLau
Copy link
Member

Thanks for the issue, and PR!

Yeah, that function isn't functionally identical (on purpose).

I'm not sure what the purpose of adding that function to whoami would be. Converting to lowercase should probably only be done for certain display purposes (and even there I'm not sure), and hostname casing should otherwise be kept.

If possible, can you tell me what you're trying to do with a lowercase-normalized hostname, and why the casing shouldn't be preserved? Is a bug caused by not making it lowercase?

The reasoning for this change is discussed here: #82

@jsgf
Copy link
Author

jsgf commented Sep 26, 2024

I'm doing a mass change of other people's code, I'm not exactly sure what the precise requirements are in each case. It's quite possible that it makes no difference at all, but I'm not in a position to be able to evaluate every callsite. We have had problems in the past from strange failures relating the case-mismatches in hostnames (eg case-sensitive string cmp & mismatch in case between hostname() and a hostname in a config file), so I'd be hesitant about just assuming the case issue can be ignored.

BTW This is to resolve the possible security issue arising from #91. So I really want a very direct migration path without having to worry about any semantic differences.

@AldaronLau
Copy link
Member

AldaronLau commented Sep 26, 2024

I'm doing a mass change of other people's code, I'm not exactly sure what the precise requirements are in each case.

Without a requirement, I'm hesitant to add this as a new function to whoami. It seems like an okay function for consumers of the library to have to write, if they really need it.

We have had problems in the past from strange failures relating the case-mismatches in hostnames (eg case-sensitive string cmp & mismatch in case between hostname() and a hostname in a config file), so I'd be hesitant about just assuming the case issue can be ignored.

To me, this sounds like switching to the new API would have fixed some of those old issues, so I'm questioning the value of the additional API in whoami besides as an intermediate step in addressing tech-debt.

BTW This is to resolve the possible security issue arising from #91. So I really want a very direct migration path without having to worry about any semantic differences.

While that function is deprecated, it's not going away any time soon (whoami 2.0 will likely be released in mid-2025, and 1.0 will still be maintained). I think it would be okay to #[allow(deprecated)] until that upgrade if you want the quickest migration path (unless you want to avoid the #[allow] and handle the Result case).

@jsgf
Copy link
Author

jsgf commented Sep 27, 2024

I've fallen back to scattering a pile of #[allow(deprecated)] around for now.

@AldaronLau
Copy link
Member

Closing as not planned

@AldaronLau AldaronLau closed this as not planned Won't fix, can't repro, duplicate, stale Nov 13, 2024
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 a pull request may close this issue.

2 participants