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

Catch another possible exception when determining the timezone. #153

Closed
wants to merge 1 commit into from

Conversation

albu-diku
Copy link

This change makes the test suite run to completion under FreeBSD.

@jonasbardino jonasbardino added enhancement New feature or request unit test labels Dec 10, 2024
@jonasbardino
Copy link
Contributor

jonasbardino commented Dec 10, 2024

AFAICT the exception comes from the /etc/localtime link pointing to something without the expected 'zoneinfo' in it on that platform so that index('zoneinfo') fails with a value not in list ValueError.
I think we should either actually parse whatever file it points to for the zone info if possible, or modify the code to clarify those boundary conditions. E.g. a conditional or a comment about why/when we might hit the ValueError.

In other words, can you please provide a quick analysis of that actual localtime link there and whether it's feasible to extract the actual zoneinfo we need or push and update / let me know if I should just add the comment?

@albu-diku
Copy link
Author

Happy to try, but I don't think that analysis is exactly what's happening - from what I saw, it's a difference in the string returned when calling localtime so when assume thing about it to break it up leading to the ValueError instead of an IndexError. Catching this extra possible exception doesn't make reading the timezone work under BSD, but it means the fallback path gets taken and things start to work. At the time I applied this change I figured a small change so the suite works, albeit not reading this value entirely correctly, was a decent trade-off.

@jonasbardino
Copy link
Contributor

Thanks, your patch adds the ValueError exception handler in the part of the code simply trying to extract the timezone name from the /etc/localtime symlink prior to calling any commands.
On Linux that symlink typically points to a path that includes the exact timezone name as in:

0|~ > ls -l /etc/localtime
lrwxrwxrwx 1 root root 37 Jul 10 07:30 /etc/localtime -> /usr/share/zoneinfo/Europe/Copenhagen

we extract the Europe/Copenhagen timezone name as the part after zoneinfo/ by splitting on / and looking up the index of timezone in the resulting list. That will of course fail if the symlink does not actually point to something with zoneinfo in it. That particular lookup with zoneinfo_path_parts.index('zoneinfo') must be where the ValueError comes from (https://github.com/ucphhpc/migrid-sync/pull/153/files#diff-aef9158f13f295cb08c3fe607c954b19860974733bc9f73b989ee583abec91e8R107).

My suggestion would therefore be to look if it's a link to something similar with a name in we can grab on BSD. If not I'm fine with any solution that bails out gracefully but just briefly explains how/why.

Copy link
Contributor

@jonasbardino jonasbardino left a comment

Choose a reason for hiding this comment

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

Basically looks safe and alright to me, but a line or two explaining where and why we might hit that ValueError would be good if we can't easily similarly extract the value on BSD.
Please refer to my previous PR comments for details.

This change makes the test suite run to completion under FreeBSD.
@albu-diku albu-diku force-pushed the fix/determine-timezone-caught-exceptions branch from e53d343 to 361c015 Compare January 21, 2025 17:24
@jonasbardino
Copy link
Contributor

Merged through svn with minor comment polish. Thanks @albu-diku :)

@albu-diku albu-diku deleted the fix/determine-timezone-caught-exceptions branch February 24, 2025 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants