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

Restore OTP 25 support #41

Closed
wants to merge 3 commits into from
Closed

Restore OTP 25 support #41

wants to merge 3 commits into from

Conversation

belltoy
Copy link
Contributor

@belltoy belltoy commented Aug 6, 2024

I've read about the failure mentioned in #37. It's because the erlang_service uses the new map comprehension syntax introduced in OTP 26.

Only three places use the new syntax. Can we restore OTP 25 support by replacing the map comprehension with the legacy maps:to_list/maps:from_list? I have built the elp based on 2024-07-16 and 2024-08-05, seems to work fine.

@robertoaloi
Copy link
Contributor

@belltoy Thanks for this. I don't have a problem restoring OTP 25 support via this patch. But in CI we can only vscode-publish one entry. I'd go for OTP 25 to maximize compatibility. So, please modify one of the flags to false.

@belltoy
Copy link
Contributor Author

belltoy commented Aug 7, 2024

Updated and rebased.

@belltoy belltoy requested a review from robertoaloi August 8, 2024 08:40
@robertoaloi
Copy link
Contributor

Why is there a change in the Cargo.lock file?

@belltoy
Copy link
Contributor Author

belltoy commented Aug 8, 2024

Every time I run cargo build, cargo will always give that change. It seems there should be the source and checksum field there.

@belltoy
Copy link
Contributor Author

belltoy commented Aug 9, 2024

The reason why there is a change in my compilation is that cargo uses oss tree-sitter-erlang dep from crates.io instead of a local fb-only tree-sitter-erlang folder.

https://github.com/WhatsApp/erlang-language-platform/blob/main/Cargo.toml#L101-L102

# @fb-only: tree-sitter-erlang = { path = "./tree-sitter-erlang" }
tree-sitter-erlang = "0.7.0" # @oss-only

I guess the annotation @fb-only and @oss-only are taken by a Facebook internal tool, which works like the patch in cargo. I can reproduce this via patch in Cargo.toml:

[patch.crates-io]
tree-sitter-erlang = { path = "path/to/tree-sitter-erlang" }

And run the cargo build, the source and checksum fields will be removed by cargo, just like the lock file in the repo.

IMO, adding the two fields back to the lock file should be a fix for the open source version.

@facebook-github-bot
Copy link
Contributor

@robertoaloi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@robertoaloi
Copy link
Contributor

IMO, adding the two fields back to the lock file should be a fix for the open source version.

Unfortunately accepting those two lines breaks in the internal tooling/CI. We are discussing how to make it work for both versions. In the meantime I will patch it and get this one in!

@facebook-github-bot
Copy link
Contributor

@robertoaloi merged this pull request in 8b8eb2e.

@belltoy
Copy link
Contributor Author

belltoy commented Aug 9, 2024

Thanks, Roberto.

@robertoaloi
Copy link
Contributor

Thank you for the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants