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

reason-language-server: init at 1.7.8 #85841

Open
wants to merge 3 commits into
base: master
from
Open

reason-language-server: init at 1.7.8 #85841

wants to merge 3 commits into from

Conversation

@NickHu
Copy link
Contributor

NickHu commented Apr 23, 2020

Motivation for this change

Language Server for Reason.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

How to test

Configure your editor by telling it where the binary lives: ${pkgs.reason-language-server}/bin/reason-language-server, then

nix-shell -p bs-platform
bsb -init my-reason-project -theme basic-reason
cd my-reason project
$EDITOR src/Demo.re
NickHu added 3 commits Apr 23, 2020
This makes it much easier to override the version of ocaml reason is
built against (`reason.override { ocamlPackages = ...; }` instead of
having to override `ocaml`, `findlib`, `dune`, etc.
note: reason-language-server is *not* compatible with ocaml 4.09
ocamlPackages = ocaml-ng.ocamlPackages_4_08;
inherit (ocaml-ng.ocamlPackages_4_08) buildDunePackage;
};

This comment has been minimized.

Copy link
@NickHu

NickHu Apr 23, 2020

Author Contributor

Maybe this should live in pkgs/top-level/ocaml-packages.nix? I couldn't find a nice way to force OCaml 4.08 when it was there though

This comment has been minimized.

Copy link
@anmonteiro

anmonteiro Apr 23, 2020

Member

Agree it should.

You can do something like this: https://github.com/NixOS/nixpkgs/blob/master/pkgs/build-support/ocaml/dune.nix#L7-L10

And it would avoid having to override ocamlPackages in the reason derivation.

This comment has been minimized.

Copy link
@NickHu

NickHu Apr 23, 2020

Author Contributor

The problem is that there is no maximumOCamlVersion argument anywhere - reason-language-server works with OCaml versions before 4.09. I don't know the ocaml infrastructure well at all (this is my first look at it), so I would really appreciate it if someone more knowledgeable could maybe add a commit to this pr if this is something that requires changes.

This comment has been minimized.

Copy link
@turboMaCk

turboMaCk Apr 23, 2020

Member

What about changing the expression so that instead of buildDunePackage and ocamlPackages it takes ocaml-ng and then picking these two things within the pkgs/development/tools/reason-language-server/default.nix ?

This comment has been minimized.

Copy link
@anmonteiro

anmonteiro Apr 23, 2020

Member

Something else I remembered: given that Reason is a build time only dependency, does it even need to be on the same OCaml version than RLS? AFAIK there just needs to be a refmt binary in $PATH for Dune to compile a Reason project.

This comment has been minimized.

Copy link
@NickHu

NickHu Apr 23, 2020

Author Contributor

Something else I remembered: given that Reason is a build time only dependency, does it even need to be on the same OCaml version than RLS? AFAIK there just needs to be a refmt binary in $PATH for Dune to compile a Reason project.

While I was packaging it seemed to break unless Reason used the same OCaml version.

What about changing the expression so that instead of buildDunePackage and ocamlPackages it takes ocaml-ng and then picking these two things within the pkgs/development/tools/reason-language-server/default.nix ?

I don't have strong feelings either way, but I feel like someone who uses OCaml a lot more than me should make the decision.

This comment has been minimized.

Copy link
@vbgl

vbgl Apr 23, 2020

Contributor

Does the user of reason-language-server care about the version of OCaml that is used to build it? (i.e., does RLS behave differently depending on the version of OCaml used to build it?) Programs that use native OCaml serialization often do (for instance: unison, Coq, merlin).

If this is the case, it may make sense to put it inside ocamlPackages.

Otherwise just take ocamlPackages as argument (don’t take neither reason nor buildDunePackage as argument: read them from ocamlPackages).

In either case, reason should not take the whole ocamlPackage set as argument (because it is part of it).

This comment has been minimized.

Copy link
@turboMaCk

turboMaCk Apr 23, 2020

Member

yup exactly my thoughts @vbgl 👍

This comment has been minimized.

Copy link
@NickHu

NickHu Apr 24, 2020

Author Contributor

I think there's an argument that reason shouldn't be a part of ocamlPackages to be made - seems like the use case of compiling reason against different sets of ocamlPackages is going to be a lot more common than overriding specific dependencies, and it is supposed to be its own whole ecosystem.

Does the user of reason-language-server care about the version of OCaml that is used to build it? (i.e., does RLS behave differently depending on the version of OCaml used to build it?) Programs that use native OCaml serialization often do (for instance: unison, Coq, merlin).

I'm not sure of the answer to this question. Someone who has used reason more should weigh in.

This comment has been minimized.

Copy link
@vbgl

vbgl Apr 25, 2020

Contributor

Whatever the answer to this question, the commit “reason: use the whole ocamlPackages set as a variable” is wrong.

In particular, its explanation (“This makes it much easier to override the version of ocaml reason is built against (reason.override { ocamlPackages = ...; } instead of having to override ocaml, findlib, dune, etc.”) is nonsense. If you want a reason built with a specific set of OCaml stuff, just use ocaml-ng.ocamlPackages_XXX.reason.

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

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.