search

keith / buildifier-prebuilt

A bazel toolchain for using prebuilt binaries for buildifier and buildozer
MIT License
35 stars 13 forks source link

Windows fixes for buildifier check, fix and test #89

Open peakschris opened 2 months ago

peakschris commented 2 months ago

This PR includes fixes to support check, fix and test on Windows. Fixes https://github.com/keith/buildifier-prebuilt/issues/88

Tested with bazel 7.2.1 and both --noenable_runfiles and --enable_runfiles All validations performed with --windows_enable_symlinks enabled

for buildifier.test to work, I used the following config. I could not get test to work without specifying workspace and no_sandbox.

buildifier_test(
    name = "buildifier.test",
    srcs = ["BUILD"],
    exclude_patterns = [
        "./.git/*",
    ],
    lint_mode = "warn",
    no_sandbox = True,
    workspace = "WORKSPACE",
)
keith commented 2 months ago

any way we can verify the previous failure on CI?

peakschris commented 2 months ago

@keith @cgrindel I've merged https://github.com/keith/buildifier-prebuilt/pull/90 here. Please would you run this CI?

Expected results: Windows: all tests pass Linux: 1 test fails. 4 of 6 sub-tests pass. 2 fail as the linux path does not do manifest lookups.

How should we handle the linux failure? change the test to ignore that failure and raise an extra defect?

Thanks, Chris

keith commented 2 months ago

Started CI

peakschris commented 2 months ago

@keith thank you. Results as expected. Would to like me to disable the two failing cases for non-windows?

keith commented 2 months ago

Looks like it failed on Linux but not windows? Maybe misunderstanding what you expected?

peakschris commented 2 months ago

I added new cases within my new test for:

buildifier invoke (norunfiles) - passes windows & linux buildifier check (norunfiles) - passes windows, fails linux buildifier fix (norunfiles) - passes windows, fails linux buildifier invoke (w/runfiles) - passes windows & linux buildifier check (w/runfiles) - passes windows & linux buildifier fix (w/runfiles) - passes windows & linux

The linux failures are demonstrating existing problems (or limitations), and are not addressed by this PR.

If you agree, I can I open a new defect and disable these two cases on linux for now. Most people won't hit these as no runfiles on linux is uncommon. I won't have time to work on these.

Cheers, Chris

peakschris commented 2 months ago

I have opened #91 for the linux issue, updated the test cases.

@keith @cgrindel Please rerun CI, I'd expect all to pass now.

cgrindel commented 2 months ago

I just approved CI.

peakschris commented 2 months ago

@cgrindel could you rerun this one?

peakschris commented 2 months ago

Awesome! Windows & Linux are clean... Mac seems to be waiting on a runner... What are next steps with this PR?