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

Bump Packages dependency to point to Sourcegraph fork #9

Closed
varungandhi-src opened this issue Jan 14, 2022 · 5 comments
Closed

Bump Packages dependency to point to Sourcegraph fork #9

varungandhi-src opened this issue Jan 14, 2022 · 5 comments

Comments

@varungandhi-src
Copy link
Collaborator

varungandhi-src commented Jan 14, 2022

I recently created a Sourcegraph-specific fork of Packages and updated it with changes from Stephen's fork.
sourcegraph/Packages#1

Additionally, this fork is largely up-to-date with upstream syntect.

However, when we try to update this fork to use a recent commit of sourcegraph/Packages, we run into issues because the grammars use features such as meta_prepend: etc. which are "new" in Sublime Text 4.

Upstream is tracking adding support for these: trishume#323 ; there are several new features and it looks like every new feature is being used by some grammar in the Packages repo. 😬

There are a couple of options here:

  1. Create a st3 branch in sourcegraph/Packages, and base it off a merge of the commit currently being used by syntect/master and slimsag/master. Point this repo to the st3 branch until upstream adds support for ST4 grammar features; after that we can update this repo and then point to the master branch instead of st3.

    Since I recently fixed the merge conflicts for sourcegraph/Packages, maybe it's not that hard to do that for a st3 branch as well. If we pick this option, we should make sure to set up CI for the st3 branch and mention the branch in the README on the master branch.

  2. Work with upstream to add support for newer ST4 grammar features.

These don't have to be mutually exclusive. Maybe we should do the first one in the short-term and the second one after that?

@varungandhi-src
Copy link
Collaborator Author

varungandhi-src commented May 5, 2022

I tried a bunch of configurations for running tests and here are the results:

id syntect Packages make assets syntect tests
1 slimsag/master slimsag/master Y ❌ 8 failures
2 slimsag/master slimsag/master N ❌ 8 failures
3 sourcegraph/main fa6b8629† no-op ✅ passing
4 sourcegraph/main slimsag/master Y ❌ 9 failures
5 sourcegraph/main sourcegraph/st3 Y ❌ 9 failures
6 sourcegraph/main sublimehq/st3 Y ❌ 8 failures

†fa6b8629 is a commit from 2017 on sublimehq/st3 (there were some changes to that branch, until 2019), which is currently being using by upstream syntect/master.

(Aside: slimsag/master is also available as sourcegraph/slimsag-main in this repo.)

Test failures

Config 1 and 2

    html::tests::strings
    html::tests::test_classed_html_generator_no_empty_span
    html::tests::tokens
    parsing::parser::tests::can_compare_parse_states
    parsing::parser::tests::can_parse_backrefs
    parsing::parser::tests::can_parse_includes
    parsing::parser::tests::can_parse_simple
    parsing::syntax_set::tests::can_load

Config 4 and 5 - same failures as for 1 & 2, plus extra failure

    parsing::parser::sourcegraph_tests::javascript_hang

Config 6 - same failures as for 1 & 2, except for:

-   html::tests::test_classed_html_generator_no_empty_span
+   parsing::parser::sourcegraph_tests::javascript_hang

@varungandhi-src
Copy link
Collaborator Author

varungandhi-src commented May 6, 2022

On bisecting between the current st3 branch on sourcegraph/Packages (ebd791c91) and the "known good" fa6b8629, I get the first failing commits for the different tests as follows:

Test name First failure
html::tests::test_classed_html_generator_no_empty_span 3c3930fcfa85510eeef9ca07ba41040a95425888
html::tests::tokens 296cb3006b6791a7f65d9248f688c240c1f68481
html::tests::strings 294df46dba36afa14c578fcf12b648d1c0af0edc
parsing::parser::sourcegraph_tests::javascript_hang 4fdbe6cf0b6f9ffb65e2685c745be35ff2edda85
parsing::parser::tests::can_compare_parse_states 03baf7403993ac509908a337ed1dc01ffcdad3e5
parsing::parser::tests::can_parse_includes 9deeda8e574d4ee957aa9b7a4edb394a20142935
parsing::parser::tests::can_parse_backrefs c264123ef9436e8f382bad25225d2db8fcd8496c
parsing::parser::tests::can_parse_simple e0f865d8a073a5b36f3c356e3e2700973703095e
parsing::syntax_set::tests::can_load c264123ef9436e8f382bad25225d2db8fcd8496c

@varungandhi-src
Copy link
Collaborator Author

Closing this; we can bump the commit of Sourcegraph/Packages later.

@varungandhi-src
Copy link
Collaborator Author

Reopened because the bump was incorrect -- we are not including any of the changes in Sourcegraph/Packages.

@varungandhi-src
Copy link
Collaborator Author

Fixed in #11

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

No branches or pull requests

1 participant