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 some issues with newlines in fixes on Windows #315

Merged
merged 9 commits into from
Feb 4, 2025

Conversation

ZedThree
Copy link
Member

When I deduplicated building the binaries, it also stopped us building on different platforms.

And it looks like we were never actually testing on those platforms!

Windows failures seem pretty simple: paths are using \ instead of / as expected in the snapshots. Can probably do something simple to fix those in the tests.

Mac failures more puzzling, looks like per-file-ignores not working quite as expected?

@ZedThree ZedThree added the ci Related to CI label Jan 31, 2025
@LiamPattinson
Copy link
Collaborator

For the Mac problem, perhaps it's this line:

We're using .current_dir(path) and also .arg(path), which might be confusing things. I think removing .arg(path) might fix it, but without a Mac in my hands I can't really check.

@ZedThree
Copy link
Member Author

We're using .current_dir(path) and also .arg(path), which might be confusing things. I think removing .arg(path) might fix it, but without a Mac in my hands I can't really check.

Nice, good catch!

Failures on Windows now are due to line-ending handling -- I think these are actual bugs, not just issues in the tests

@ZedThree ZedThree changed the title CI: Restore Windows and Mac tests on PRs Fix some issues with newlines in fixes on Windows Feb 3, 2025
@ZedThree ZedThree added bug Something isn't working and removed ci Related to CI labels Feb 3, 2025
@ZedThree
Copy link
Member Author

ZedThree commented Feb 3, 2025

Hmm remaining failure is this:

---- rules::style::tests::rules::rule_incorrectspacebeforecomment_path_new_s102_f90_expects stdout ----
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ Snapshot Summary ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
Snapshot file: fortitude\src\rules\style\snapshots\fortitude__rules__style__tests__incorrect-space-before-comment_S102.f90.snap
Snapshot: incorrect-space-before-comment_S102.f90
Source: D:\a\fortitude\fortitude:42
───────────────────────────────────────────────────────────────────────────────
Expression: diagnostics
───────────────────────────────────────────────────────────────────────────────
-old snapshot
+new results
────────────┬──────────────────────────────────────────────────────────────────
   11    11 │ ℹ Safe fix␊
   12    12 │ 1 1 | ! This is fine␊
   13    13 │ 2 2 |  ! This isn't but we'll let it pass␊
   14    14 │ 3   |-module mymod! This should be given two extra spaces␊
   15       │-  3 |+module mymod  ! This should be given two extra spaces␊
   16       │-4 4 |   implicit none ! This should be given one extra space␊
   17       │-5 5 |   private  ! This is fine␊
   18       │-6 6 |   ! This is fine␊
         15 │+  3 |+module mymod  ! This should be given two extra spaces␍
         16 │+  4 |+␊
         17 │+4 5 |   implicit none ! This should be given one extra space␊
         18 │+5 6 |   private  ! This is fine␊
         19 │+6 7 |   ! This is fine␊
   19    20 │ ␊
   20    21 │ ./resources/test/fixtures/style/S102.f90:4:17: S102 [*] need at least 2 spaces before inline comment␊
   21    22 │   |␊
   22    23 │ 2 |  ! This isn't but we'll let it pass␊
┈┈┈┈┈┈┈┈┈┈┈┈┼┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈
   32    33 │ 1 1 | ! This is fine␊
   33    34 │ 2 2 |  ! This isn't but we'll let it pass␊
   34    35 │ 3 3 | module mymod! This should be given two extra spaces␊
   35    36 │ 4   |-  implicit none ! This should be given one extra space␊
   36       │-  4 |+  implicit none  ! This should be given one extra space␊
   37       │-5 5 |   private  ! This is fine␊
   38       │-6 6 |   ! This is fine␊
   39       │-7 7 | ! As is this
         37 │+  4 |+  implicit none  ! This should be given one extra space␍
         38 │+  5 |+␊
         39 │+5 6 |   private  ! This is fine␊
         40 │+6 7 |   ! This is fine␊
         41 │+7 8 | ! As is this
────────────┴──────────────────────────────────────────────────────────────────

and I cannot see anything obvious. I would've guessed that this could happen if we were writing a newline, but I'm reasonably sure we're explicitly not.

It might be quite tricky to debug this without a Windows env!

@LiamPattinson
Copy link
Collaborator

That's really strange. Am I reading it correctly in that it it's adding an extra newline in Windows?

@ZedThree
Copy link
Member Author

ZedThree commented Feb 3, 2025

Sort of -- it's changing the existing line ending from \n (unix) to \r\n (DOS line ending). The characters in the output are really small, but you can see CR/LF.

My understanding is that we're replacing the comment node upto but not including the line ending, so we shouldn't be writing one back out. I don't think we explicitly write new lines anywhere?

@LiamPattinson
Copy link
Collaborator

I think this might be similar to an issue we saw with end module etc, where the node included a newline character. Oddly, I think tree-sitter was adding a trailing \r via utf8_text. I wasn't able to completely figure out what was going wrong and why, but I was able to fix it up by simplifying the edit. I also fixed up a couple other Windows issues, and I'll raise a PR once it's all finalised.

@ZedThree
Copy link
Member Author

ZedThree commented Feb 3, 2025

Nice, thanks for the extra fixes! The insertion is much simpler.

I've investigated further, and the tree-sitter grammar consumes the \r character into comments -- that might be something we have to watch out for in future. Or work out how to fix it in the grammar.

@ZedThree ZedThree merged commit fbfb140 into main Feb 4, 2025
37 checks passed
@ZedThree ZedThree deleted the restore-windows-mac-ci branch February 4, 2025 10:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants