search

bazelbuild / bazel-skylib

Common useful functions and rules for Bazel
https://bazel.build/
Apache License 2.0
376 stars 179 forks source link

feat: case insensitive diff_test and fix manifest bug #527

Open peakschris opened 1 month ago

peakschris commented 1 month ago

In my work on windows quality in rulesets, a frequent issue I am finding is that diff_tests are failing due to differences in line endings -- either due to differences in the 'expected' values, or cross-platform differences in generators (e.g. buildifier always writes LF, whilst skydoc writes LF or CRLF depending on platform).

We can't expect rule authors to work around these differences, because in practice they do not have good access to windows dev hardware.

The goal of this PR is to make diff_test slightly less brittle and ignore line endings by default. An ignore_line_endings flag is provided to switch this behavior.

This flag is supported by the diff command on unix. On windows, it requires the tr.exe command that is part of most bash installs including msys2. If tr.exe cannot be found the ignore_line_endings flag is silently ignored and set to False.

This PR allows us to enable the 1 disabled test 'stardoc_with_diff_test' on windows CI, and also fixes many test failures in dependent rulesets such as rules_starlib and rules_bazel_integration_test

google-cla[bot] commented 1 month ago

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

peakschris commented 1 month ago

This fixes:

peakschris commented 3 weeks ago

@brandjon @tetromino @comius @hvadehra @c-mita gentle ping, would it be possible for someone to review this? I have a chain of PRs to rulesets dependent on this one to improve windows support.

cgrindel commented 3 weeks ago

@peakschris Did you see this message? This may not be reviewed and merged. I will bring this up at the next Community for Bazel technical steering committee meeting which is scheduled for Tuesday, July 9.