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.

Sign up for GitHub

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

Jump to bottom

Drop the input before writing to the output #813

Merged
merged 11 commits into from
Jan 21, 2025
Merged

Drop the input before writing to the output #813

merged 11 commits into from
Jan 21, 2025

Conversation

Xophmeister
Copy link
Member

@Xophmeister Xophmeister commented Dec 23, 2024 •
edited
Loading

Drop the input before writing to the output

Resolves #812

Description

When Topiary tries to write its output in place, it does so by writing to a temporary file that is then moved to the same location as the input file when it's complete (known as "persisting"). The hypothesis is that Windows opens an exclusive lock on the input file and therefore this process fails.

This PR causes all references to the open input file to be dropped -- and hence the file handler closed -- before persistence.

Note

This PR also introduces a minimal Windows CI workflow.

Checklist

Checklist before merging:

@Xophmeister Xophmeister force-pushed the chris/drop-in-before-out branch from 1088e24 to f8eb9e9 Compare January 14, 2025 16:32
@Xophmeister
Copy link
Member Author

Experimentation suggests that building the libraries with Topiary doesn't work on Windows, even if you force it to use the GNU toolchain. tree-sitter build demonstrably works, so there are two options at this point:

  1. Put together a quick-and-dirty CI workflow for Windows which calls tree-sitter build first, to build all the grammars?
  2. Implement it properly within Topiary (per Should we shell out to tree-sitter build to build grammar libraries? #829) and then rebase, allowing for a very simple Windows pipeline?

I think (2) is the obvious choice...but it will take a bit of time.

@Xophmeister Xophmeister force-pushed the chris/drop-in-before-out branch from d993474 to 455e8a7 Compare January 15, 2025 10:55
@Xophmeister
Copy link
Member Author

Going with (2) in #830...

@Xophmeister Xophmeister force-pushed the chris/drop-in-before-out branch from 455e8a7 to 0634ed5 Compare January 20, 2025 14:53
@Xophmeister Xophmeister mentioned this pull request Jan 20, 2025
Merged
2 tasks
@Xophmeister
Copy link
Member Author

The good news is that the grammar building changes introduced (and rebased into this branch) in #830 are working on Windows:

2025-01-20T15:25:33.3388582Z [2025-01-20T15:22:07Z INFO  topiary_config::language] bash: Language Grammar not found, attempting to fetch and compile it
2025-01-20T15:25:33.3389932Z [2025-01-20T15:22:07Z INFO  topiary_config::language] bash: Cloning from https://github.com/tree-sitter/tree-sitter-bash.git
2025-01-20T15:25:33.3390910Z Cloning into 'C:\Users\RUNNER~1\AppData\Local\Temp\.tmpB8O8TH\bash'...
2025-01-20T15:25:33.3391795Z [2025-01-20T15:22:08Z INFO  topiary_config::language] bash: Checking out d1a1a3fe7189fdab5bd29a54d1df4a5873db5cb1
2025-01-20T15:25:33.3392593Z Note: switching to 'd1a1a3fe7189fdab5bd29a54d1df4a5873db5cb1'.
2025-01-20T15:25:33.3393003Z 
2025-01-20T15:25:33.3393287Z You are in 'detached HEAD' state. You can look around, make experimental
2025-01-20T15:25:33.3393996Z changes and commit them, and you can discard any commits you make in this
2025-01-20T15:25:33.3394665Z state without impacting any branches by switching back to a branch.
2025-01-20T15:25:33.3395105Z 
2025-01-20T15:25:33.3395366Z If you want to create a new branch to retain commits you create, you may
2025-01-20T15:25:33.3396016Z do so (now or later) by using -c with the switch command. Example:
2025-01-20T15:25:33.3396370Z 
2025-01-20T15:25:33.3396535Z   git switch -c <new-branch-name>
2025-01-20T15:25:33.3396779Z 
2025-01-20T15:25:33.3396910Z Or undo this operation with:
2025-01-20T15:25:33.3397180Z 
2025-01-20T15:25:33.3397294Z   git switch -
2025-01-20T15:25:33.3397462Z 
2025-01-20T15:25:33.3397802Z Turn off this advice by setting config variable advice.detachedHead to false
2025-01-20T15:25:33.3398229Z 
2025-01-20T15:25:33.3398376Z HEAD is now at d1a1a3f 0.20.5
2025-01-20T15:25:33.3399202Z [2025-01-20T15:22:08Z INFO  topiary_config::language] bash: Building grammar
2025-01-20T15:25:33.3399988Z [2025-01-20T15:22:10Z INFO  topiary_config::language] bash: Grammar successfully compiled
2025-01-20T15:25:33.3401256Z [2025-01-20T15:22:10Z DEBUG topiary_config::language] Loading grammar from C:\Users\runneradmin\AppData\Local\topiary\cache\bash\d1a1a3fe7189fdab5bd29a54d1df4a5873db5cb1.dll

The bad news is that input_output_tester and formatted_query_tester are panicking, causing the CI to fail. They're panicking because the output and expected output differ (see topiary-core/src/test_utils.rs), but the diff output is only showing:

...

i.e., The context skipping marker, with no actual diffs 🤷

Best guess: Windows line endings, or no EOF \n...but I think that's a long shot.

@Xophmeister
Copy link
Member Author

Best guess: Windows line endings, or no EOF \n...but I think that's a long shot.

Bingo!...but it's not in Topiary's output, it's from the Git checkout:

$ md5sum expected.sh formatted.sh
f862ea1582a18bf93dfb1109859a9801  expected.sh
a5259ba2d34f94d028405fe730154ad8  formatted.sh

$ dos2unix -n expected.sh expected-unix.sh
$ md5sum expected-unix.sh formatted.sh
a5259ba2d34f94d028405fe730154ad8  expected-unix.sh
a5259ba2d34f94d028405fe730154ad8  formatted.sh

@Xophmeister Xophmeister changed the base branch from main to chris/grammar-build January 21, 2025 10:38
@Xophmeister Xophmeister marked this pull request as ready for review January 21, 2025 11:02
yannham
yannham approved these changes Jan 21, 2025
View reviewed changes
topiary-cli/src/main.rs Show resolved Hide resolved
Base automatically changed from chris/grammar-build to main January 21, 2025 12:57
@Xophmeister Xophmeister force-pushed the chris/drop-in-before-out branch from a421397 to 3fd8740 Compare January 21, 2025 12:59
@Xophmeister Xophmeister merged commit 373b58d into main Jan 21, 2025
9 checks passed
@Xophmeister Xophmeister deleted the chris/drop-in-before-out branch January 21, 2025 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yannham yannham yannham approved these changes

@nbacquey nbacquey Awaiting requested review from nbacquey

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Could not persist output to disk on windows
2 participants
@Xophmeister @yannham