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

fix: moving the preserve mtime test logic to Go for portability #908

Merged

Conversation

jpinkul
Copy link
Contributor

@jpinkul jpinkul commented Aug 15, 2024

The original bash version of the preserve mtime test doesn't work for Mac / Windows. For better portability I changed this to a go_test.

Changes are visible to end-users: yes

Documentation added as well.

  • Searched for relevant documentation and updated as needed: yes
  • Breaking change (forces users to change their own code or config): no
  • Suggested release notes appear below: no

Test plan

I've run this locally on a Linux machine to verify the basic functionality and ensured the test still fails if the preserve_mtime attributes are commented out. CI will be needed to test this on Mac / Windows.

@jpinkul
Copy link
Contributor Author

jpinkul commented Aug 24, 2024

So there are a few things that were going wrong with this mtime test.

First, the previous shell script version was not portable with MacOS and Windows. This has been solved by moving the test logic to Golang.

Second, the Windows version was unable to locate the test data due to differences in how runfiles are managed on the different platforms. This has been solved by using the proper runfiles library.

Third, this feature is incompatible with remote caching and execution as those processes do not preserve the modify time across them. This caveat has been added to the documentation for the preserve_mtime attribute.

Last, with all of these changes the test is still flaky on CI. I've been able to reproduce these failures locally and I think this is what is going wrong:

bazel clean --expunge

# This test should pass
bazel test :test_preserve_mtime

# Update the mtime, in the CI environment this is done with the SCM integration.
touch d/1

# This test now fails
bazel test :test_preserve_mtime 

To get around this I've kept the manual tag on the new version of the test.

With this all said I do think keeping the test around and matching feature is still valuable with all these caveats. When the actions execute the mtime is preserved as the feature suggests, it's just that by design the modify time is not part of Bazel's cache keys.

I think we could make a more reliable version of this test if the the source being copied was from an external repository instead of checked-in files. This matches more closely to the use case that initially inspired this feature and would be more stable since Bazel preserves modify times when extracting external archives and the SCM would not touch that data. I started going down this path a bit by creating a minimal repository tar with just one filegroup but hit a roadblock when I found bzlmod archive_override required absolute file URIs and honestly this is probably more work than the extra test coverage is worth.

@alexeagle alexeagle force-pushed the jpinkul/mtime-go-test-macos-windows branch from 6b8a1bc to 8bc877e Compare September 18, 2024 00:17
@alexeagle alexeagle force-pushed the jpinkul/mtime-go-test-macos-windows branch from 8bc877e to f1688cf Compare September 18, 2024 00:19
@alexeagle alexeagle changed the title fix: moving the preserve mtiem test logic to Go for portability fix: moving the preserve mtime test logic to Go for portability Sep 18, 2024
Copy link
Collaborator

@alexeagle alexeagle left a comment

Choose a reason for hiding this comment

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

Thanks! I shuffled the docs around a bit and rebased.

@alexeagle alexeagle merged commit ad48c0d into bazel-contrib:main Sep 18, 2024
44 of 46 checks passed
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.

2 participants