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

Support AIX operating system #57

Merged
merged 1 commit into from Oct 17, 2023
Merged

Support AIX operating system #57

merged 1 commit into from Oct 17, 2023

Conversation

ecnelises
Copy link
Contributor

This change makes this crate support detecting timezone on AIX. Since AIX support hasn't been merged yet, this is in draft status.

@Kijewski
Copy link
Collaborator

Kijewski commented Sep 5, 2022

Thank you for working on this!

According to https://www.ibm.com/docs/en/aix/7.1?topic=files-environment-file and https://unixhealthcheck.com/blog?id=291 AIX has its own time zone naming scheme, incompatible with the names in the Time Zone Database. So I guess there needs to be some translation step, that translates (at least) the standard time zones to one of the cities, e.g. "CET-1CEST" → "Europe/Berlin".

The other implementations don't evaluate the environment variables by design. I would rather scan /etc/environment for TZ=… like in the Solaris/Illumos implementation.

@Kijewski Kijewski added the Tier-3 Rust Tier-3 platform label Oct 1, 2022
@ecnelises
Copy link
Contributor Author

Hi @Kijewski , do you mean that the (timezone, offset, daylight timezone, offset) form should be transformed into Region/City form?

If so, mapping from (timezone, offset) to city is a one-to-many relation. For example, CST-8 means either Asia/Chongqing Asia/Shanghai or Asia/Harbin. So arbitrary will be chosen?

@ecnelises
Copy link
Contributor Author

Gentle ping.. any further comments?

@Kijewski
Copy link
Collaborator

Kijewski commented May 17, 2023

I would scan /etc/environment for TZ=…, same as in illumos. This way the logic is the same as in our other platform implementations. Maybe you don't even need to add a new file, and you can re-use tz_illumos.rs.

On AIX you can use Olson and POSIX time zones (https://developer.ibm.com/articles/au-aix-posix/). For no obvious reason the American POSIX time zones like PST8PDT are also valid IANA time zone identifiers, but e.g. "CET-1CEST" is not.

I guess we can only hope that the system time zone is an IANA identifier. I would not like to add a white list. We could filter out results like CET-1CEST-2,M3.5.0/02:00:00,M10.5.0/03:00:00 (anything that contains a comma), strip off everything after the comma, or let the caller decide how to use the information.

@astraw, what do you think?

@astraw
Copy link
Member

astraw commented May 18, 2023

Why is this still a draft?

I'm far from an AIX expert nor IANA database expert, but this all seems OK to me.

Although new to me, as far as I understand https://web.archive.org/web/20220525113023/https://www.ietf.org/timezones/tzdb-2018g/theory.html, POSIX time zones are valid Olson tz names, so I don't think we need to do any filtering or parsing.

@ecnelises ecnelises marked this pull request as ready for review June 13, 2023 09:41
@astraw
Copy link
Member

astraw commented Oct 17, 2023

@Kijewski do think the logic in my comment above is acceptable and we can merge this?

@Kijewski
Copy link
Collaborator

Kijewski commented Oct 17, 2023

As far as I understand it, every name in the Olson database is a valid value for a POSIX tz string, but the reverse is not true.

I am only mildly hesitant if we should allow POSIX tz strings, because a downstream consumer might assume that we are only returning valid names, and try to put it in a path name. E.g. in windows a colon can be used to for file stream selection, but with AIX being a Unix implementation, I guess that a colon does not pose a security risk. In Unix colons are sometimes used to separate paths, e.g. in the $PATH variable, but I don't think anyone would/should paste their time zone in such a variable anyway.

And we aren't checking if the result is sane for any other OS, so I guess this PR is at least good enough. If the implementation can be made better in the future, then we can still update it.

@astraw
Copy link
Member

astraw commented Oct 17, 2023

These are valid concerns and perhaps we should have a sanity check on the output shared across all implementations. I don't think that's a specific issue with this AIX implementation. Therefore I'll go ahead and merge this now and we can consider sanity checks in a platform independent way.

@astraw astraw merged commit 14687ad into strawlab:main Oct 17, 2023
@ecnelises
Copy link
Contributor Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Tier-3 Rust Tier-3 platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants