Have touch in test fixture fall back to portable format
#1496
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
For each edge-of-range `touch`, if at first it fails, try it rounded to the nearest *more extreme* value in seconds, which some implementations will accept and clip it at or near the most extreme value in the range that it was going for. See GitoxideLabs#1491.
Byron
approved these changes
Aug 8, 2024
There was a problem hiding this comment.
Thanks a lot for the fix!
I think if the test isn't actually asserting for nanoseconds anymore and works even with more portable date specifications, it should be preferable to not use the nanosecond timestamp in the first place.
But now that we have found out about the portability issues coming with the nanosecond specification, I'd think it's fine to leave it as documentation, hopefully preventing such issues in the future (or account for them properly).
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Oct 24, 2024
1. Stop attempting to specify fractions of a second, which often (always?) does not work better and which requires more complex logic to express because a fallback is needed for some systems. The test currently using this fixture does not appear to need or significantly benefit from the most extreme possible dates, even if fractional values facilitate that, and the output of `stat` implementations that show fractional values do not seem to ever show different values when they are used with `touch`. 2. Having made what was the fallback logic (introduced in GitoxideLabs#1496) the primary approach, add a fallback touch command for `future` to handle how some systems with 32-bit `time_t` remain in use and have `touch` commands that treat too-large values as hard errors. The test systems where this was produced are a 32-bit x86 Ubuntu 18.04 LTS (Extended Security Maintenance) and a 64-bit x86 OmniOS system (where even the 64-bit GNU `touch` binary fails with an error if the fallback is not used). See the rewritten comment for details.
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Oct 24, 2024
1. Set `TZ=UTC`. This is for accuracy even more than portability; it is worthwhile on its own. The effect is not implied by any of the environment variables we set when running fixtures, and it has not been included in the date strings, though that could be done. Doing it this way, rather than by adding something to the date strings, makes it slightly easier to verify portability by examination. Without `TZ=UTC` or specifying the time zone in the date string, an unspecified time zone may be used; this may be the local time zone, but even if so, that will be incorrect since Unix timestamps stored in filesystem metadata are implicitly assumed UTC, so the extreme values we hard-code must be interpreted that way to have their intended effects. 2. Stop attempting to specify fractions of a second, which often (always?) does not work better and which requires more complex logic to express because a fallback is needed for some systems. The test currently using this fixture does not appear to need or significantly benefit from the most extreme possible dates, even if fractional values facilitate that, and the output of `stat` implementations that show fractional values do not seem to ever show different values when they are used with `touch`. 3. Having made what was the fallback logic (introduced in GitoxideLabs#1496) the primary approach, add a fallback touch command for `future` to handle how some systems with 32-bit `time_t` remain in use and have `touch` commands that treat too-large values as hard errors. The test systems where this was produced are a 32-bit x86 Ubuntu 18.04 LTS (Extended Security Maintenance) and a 64-bit x86 OmniOS system (where even the 64-bit GNU `touch` binary fails with an error if the fallback is not used). See the rewritten comment for details.
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Oct 24, 2024
1. Set `TZ=UTC`. This is for accuracy even more than portability; it is worthwhile on its own. The effect is not implied by any of the environment variables we set when running fixtures, and it has not been included in the date strings, though that could be done. Doing it this way, rather than by adding something to the date strings, makes it slightly easier to verify portability by examination. Without `TZ=UTC` or specifying the time zone in the date string, an unspecified time zone may be used; this may be the local time zone, but even if so, that will be incorrect since Unix timestamps stored in filesystem metadata are implicitly assumed UTC, so the extreme values we hard-code must be interpreted that way to have their intended effects. 2. Stop attempting to specify fractions of a second, which often (always?) does not work better and which requires more complex logic to express because a fallback is needed for some systems. The test currently using this fixture does not appear to need or significantly benefit from the most extreme possible dates, even if fractional values facilitate that, and the output of `stat` implementations that show fractional values do not seem to ever show different values when they are used with `touch`. 3. Having made what was the fallback logic (introduced in GitoxideLabs#1496) the primary approach, add a fallback touch command for `future` to handle how some systems with 32-bit `time_t` remain in use and have `touch` commands that treat too-large values as hard errors. The test systems where this was produced are a 32-bit x86 Ubuntu 18.04 LTS (Extended Security Maintenance) and a 64-bit x86 OmniOS system (where even the 64-bit GNU `touch` binary fails with an error if the fallback is not used). See the rewritten comment for details.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This mitigates and probably can even be considered to fix #1491, at least with respect to the main variant of that issue where the problem is solely due to the format of the
-doperand and not to the range of supported values.For each edge-of-range
touch, if at first it fails, this will try it rounded to the nearest more extreme value in seconds, which some implementations will accept and clip it at or near the most extreme value in the range that it was going for.This should not lead to any unexpected or harder-to-diagnose errors because the one test that uses this fixture deliberately avoids asserting anything about the actual timestamps, anticipating that they may be close-by values instead:
https://github.com/Byron/gitoxide/blob/29898e3010bd3332418c683f2ac96aff5c8e72fa/gix-index/tests/index/fs.rs#L8-L9
However, if it is intended that
touchfail and bring down the test when it cannot specify nanoseconds, then that might be a reason not go with this change.Note that while, before this change, the test fails (due to the fixture failing) when
touchrejects nanoseconds, it would still pass if the nanoseconds are ignored. As noted in #1491, I think that happens more often than may have been anticipated, and I wonder if really it might be better to use singletouchcommands (no&&) with more portable date strings.This fixture's previous code did not seem to account for or explain the intended behavior with respect to time zone, and I have not attempted to address that here either.
I have verified locally that all tests pass on the Alpine Linux 3.17 system on which I discovered #1491. This includes
from_path_no_follow, which failed on that system prior to the changes here.