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

Merge upstream + script to fix headers #14

Merged
merged 55 commits into from
Sep 10, 2024
Merged

Merge upstream + script to fix headers #14

merged 55 commits into from
Sep 10, 2024

Conversation

yangchi
Copy link

@yangchi yangchi commented Sep 6, 2024

A biggie.. i know.. You can jump directly to this commit: 85a8095

@glkz and I had some discussion about the current divergence between our fork and go-tree-sitter upstream.
The tl;dr is that getting rid of fork will be a bit hard, thanks to Bazel in Forge. But we should try to keep in sync more often.
Our fork is now behind upstream when it comes to grammar version in quite a number of languages.
This PR syncs with upstream.
The first 50 commits are from upstream
The last 2 are my own changes: The first is just merge conflicts due to the first 50.
The last commit is the only interesting one. I added a script header_fix.py to fix up header #include in C files. Upstream start to do the following types of inclusions that cannot work with bazel

#include "../bar.h"
#include "./bar.h"
#include "foo/bar.h"

The script tries to fix all of them by modifying these lines, and move files around.
Our smart AI wrote 90% of this python code, I did the last 10% human touch. :)
If this needs a in-person discussion then good thing I'm going to Paris to meet you all :)

header_fix.py Outdated
'python',
'ruby',
'rust',
#'typescript', Oh you are so special. You make me feel dumb

Choose a reason for hiding this comment

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

did you mean to commit it? i'm not sure i follow what was the intent here

Copy link
Author

Choose a reason for hiding this comment

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

i should have left a better comment.
the typescript grammar subdir is organized slightly different from the rest of them, and there is no need to do the header fixing stuff there, and if we need to do it someday, it's very likely to be very different

Copy link
Author

Choose a reason for hiding this comment

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

changed the comments

# (1) Fix up go-tree-sitter root level headers so they do not have #include "./foo.h"
# (2) Flatten a list of language subdirs that we care about. No more go-tree-sitter/{lang}/foo/bar.h
# (3) Then in these flatten subdirs, fix up any header inclusion that isn't directly a sibling file.
if __name__ == '__main__':

Choose a reason for hiding this comment

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

mind throwing a readme with instructions how and when this script should be used?

Copy link
Author

Choose a reason for hiding this comment

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

added a new README_poolside.md file

Comment on lines +9 to +16
There will be lots of conflicts. Unless we have changes we specifically made for poolside's use case and have not pushed to upstream, you probably will want to prefer upstream's version in any conflict.

Then you need to fix up some C header #include directives, mostly because upstream is OK with having Unix shortcuts in them but Forge cannot build with such. There is a python script in this repo to help you with that:

```
# in the root of go-tree-sitter repo:
./header_fix.py
```
Copy link
Member

Choose a reason for hiding this comment

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

Does it make sense to leverage _autoamtion/main.go for this purpose? It might be parametrised for a header prefix, or json config might be used.

Copy link
Author

Choose a reason for hiding this comment

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

ye that's a good question.
i thought about it, but automatically generate a patch for that file is a bit harder, since it involved changing a few replaceMap into new downloadFiles. At that point we may as well just maintain a separate main.go + grammer.json fork. That's another approach

@yangchi yangchi force-pushed the mergeUpstream branch 2 times, most recently from 354edc5 to b4ac8f8 Compare September 9, 2024 15:26
@yangchi yangchi force-pushed the mergeUpstream branch 2 times, most recently from af471e9 to 8ab49d4 Compare September 10, 2024 08:50
@yangchi yangchi merged commit 50966fc into master Sep 10, 2024
4 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.

10 participants