Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Sample client and generators for built-in queries/languages. #528

Merged
merged 10 commits into from
Jun 27, 2023

Conversation

torhovland
Copy link
Member

Fixes #137 and #509.

  • Sample client app.
  • TopiaryQuery generators for each built-in language/query.

@aspiwack
Copy link
Member

I suppose this paves the way or removing the compile-time configuration of the directory which contains the (installed) query files, doesn't it?

@vkleen
Copy link
Contributor

vkleen commented Jun 22, 2023

I think this would work nicely for the nickel format use case, except that the location of the languages folder may produce the same issue as #511.

@torhovland
Copy link
Member Author

Hmm, I wonder if my sample works only because I use a path dependency, and I happen to have the query files where the compiler expects to find them. I need to investigate.

@vkleen
Copy link
Contributor

vkleen commented Jun 22, 2023

#511 would only be an issue with external Nix builds, I think.

@torhovland torhovland marked this pull request as draft June 22, 2023 09:37
@torhovland
Copy link
Member Author

cargo package --list is our friend. Interestingly, it shows that files like languages.toml and all our sample test files are included, but the query files are not.

@torhovland
Copy link
Member Author

OK, I see what's going on. cargo package --list is checking each of our workspace members. But the languages dir is outside of these, so it doesn't get included. I will try adding an explicit include now. Which makes me think we can do the same with that languages.toml from #511, and drop the symlink.

@torhovland
Copy link
Member Author

Although it looks like we cannot include files from parent directories, so the symlink route is the way to go anyway:
https://users.rust-lang.org/t/including-files-from-parent-directory-in-package/88969

@torhovland torhovland marked this pull request as ready for review June 23, 2023 06:54
@torhovland
Copy link
Member Author

So the PR should be fine now.

@torhovland
Copy link
Member Author

I suppose this paves the way or removing the compile-time configuration of the directory which contains the (installed) query files, doesn't it?

Hmm, not directly. Because Topiary doesn't itself use these convenience generator functions for now. It could, but we already have some more complex logic for looking up query files. We need to keep this, because it allows users to override the built-in queries.

/// Convert a Language into the canonical basename of its query file, under the most appropriate
/// search path. We test 3 different locations for query files, in the following priority order,
/// returning the first that exists:
///
/// 1. Under the `TOPIARY_LANGUAGE_DIR` environment variable at runtime;
/// 2. Under the `TOPIARY_LANGUAGE_DIR` environment variable at build time;
/// 3. Under the `./languages` subdirectory.

@torhovland
Copy link
Member Author

Actually, making sure the languages directory is included in the crate, the same way we handle languages.toml, is the real fix of #509. The convenience functions are just for, well, convenience, and will likely get dropped when we support dynamic loading of grammars (#4).

Maybe it's best to drop them right now. I won't object to that.

@vkleen Can you check if this branch helps you, even without using the new convenience functions?

@vkleen
Copy link
Contributor

vkleen commented Jun 23, 2023

Building with crane and Nix fails with the branch as it is. Crane ends up copying dangling symlinks into the cargo vendor directory. Maybe that is an issue that should be addressed in crane upstream. Once topiary makes its way to crates.io, I imagine it will work fine. As you say, cargo package replaces the symlinks with copies of the actual files for publishing.

Just putting the query files into the package sources for cargo wouldn't help with accessing those files in nickel format, I think. There would still need to be either the helper functions like topiary::nickel() or just a global static string with include_str!() to get the right queries.

@torhovland
Copy link
Member Author

Building with crane and Nix fails with the branch as it is. Crane ends up copying dangling symlinks into the cargo vendor directory. Maybe that is an issue that should be addressed in crane upstream. Once topiary makes its way to crates.io, I imagine it will work fine. As you say, cargo package replaces the symlinks with copies of the actual files for publishing.

I was trying to be clever, and switched the direction of the symlinks, so the real files could stay in root. But it sounds like that's what breaking Crane. Please try again now.

Just putting the query files into the package sources for cargo wouldn't help with accessing those files in nickel format, I think. There would still need to be either the helper functions like topiary::nickel() or just a global static string with include_str!() to get the right queries.

I see. Then we'll keep them. But I wonder what the scalable, future solution should be when at some point we consider languages something that is loaded dynamically. @ErinvanderVeen

@vkleen
Copy link
Contributor

vkleen commented Jun 23, 2023

The nix builds succeed with the switch symlinks 👍

@ErinvanderVeen
Copy link
Collaborator

It will sure be interesting to see how we solve this in the future!

Copy link
Collaborator

@ErinvanderVeen ErinvanderVeen left a comment

Choose a reason for hiding this comment

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

Otherwise looks great!

@torhovland torhovland enabled auto-merge (squash) June 26, 2023 10:25
@torhovland torhovland force-pushed the 509-include-default-queries-in-crate branch from 76dacff to e360043 Compare June 27, 2023 06:39
@torhovland torhovland merged commit f317e6b into main Jun 27, 2023
@torhovland torhovland deleted the 509-include-default-queries-in-crate branch June 27, 2023 06:39
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make sure Topiary works when added as a crate in a third-party app
4 participants