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

Add --version support to NLS and fix feature unification issues #1936

Merged
merged 1 commit into from
Jun 3, 2024

Conversation

yannham
Copy link
Member

@yannham yannham commented Jun 3, 2024

This PR adds --version support for the nls binary, using the same scheme as for the main nickel binary. Its absence made it hard to know which version of NLS users would currently run, which isn't great for chasing bugs.

Doing so, I discovered some underlying feature unification issues that were here but just invisible, namely that the little stunt we pull off for generating versions in different environments (in the git repo, building for crates.io and the nixified version) requires the string feature of clap, but we didn't enable this feature explicitly anywhere. It just happened that it was enabled previously for nickel-lang-cli by chance, thanks to feature unification (through comrak which is used for the doc feature). In fact building nickel-lang-cli without the default features is failing on current master, because the absence of doc doesn't pull comrak in, which doesn't enable the string feature.

This PR fixes the compilation issue by adding the missing string feature to the clap dependency for both the cli and the lsp. We also make nickel-lang-lsp also depend on nickel-lang-core without the default featurs (as most of them are useless for the LSP), and we fix another unrelated compilation error of nickel-lang-cli without default features by making nickel-lang-core always export eval_record_spine, which would only be included when the doc feature was enabled before, but is actually used for other purposes now (namely the CLI customize mode).

Add `--version` support for the nls binary, using the same scheme as for
the main `nickel` binary.

Doing so, we discovered some underlying feature unification issues that
weren't visible before - but were already there -, namely that the
little stunt we pull off for generating versions in different
environment (in the git repo, building for crates.io and the nixified
version) requires the `string` feature of clap. It just happened that it
was enabled previously for `nickel-lang-cli` by chance by feature
unification (through `comrak`), but in fact building `nickel-lang-cli`
without the default features was failing.

This commit fixes the compilation issue by adding the missing `string`
feature to the `clap` dependency for both the cli and the lsp.
`nickel-lang-lsp` also depends on `nickel-lang-core` without the default
features now (as most of them are useless for the LSP), and we fix the
another compilation error of `nickel-lang-cli` without default features
by making `nickel-lang-core` always export `eval_record_spine`, which
would only be included when the `doc` feature was enabled before, but is
actually used for other purposes now (namely the CLI customize mode).
@yannham yannham requested review from jneem and vkleen June 3, 2024 11:14
@github-actions github-actions bot temporarily deployed to pull request June 3, 2024 11:17 Inactive
@yannham yannham added this pull request to the merge queue Jun 3, 2024
Merged via the queue into master with commit d4d864d Jun 3, 2024
5 checks passed
@yannham yannham deleted the lsp/cli-show-version branch June 3, 2024 15:06
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.

2 participants