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.

Sign up for GitHub

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

Jump to bottom

sumneko-lua-language-server: init at 1.11.2 #108093

Merged
merged 1 commit into from Jan 11, 2021
Merged

sumneko-lua-language-server: init at 1.11.2 #108093

merged 1 commit into from Jan 11, 2021

Conversation

mjlbach
Copy link
Contributor

@mjlbach mjlbach commented Jan 1, 2021 •
edited

Motivation for this change

sumneko-lua-language-server: init at 1.11.2

Things I'm not happy with.

  • Currently sumneko requires that lua-language-server, bee.so, lpeglabel.so are all in the same directory, so with this derivation they are all in bin which pollutes path
  • Currently sumneko does not expand "~" to home directory, so logs write in "~" (prior to some upstreamed patches, it was always writing to "root" which would default into nix store. I'm not sure if this can be addressed with wrap program or has to be fixed within sumneko I wrote a pretty large in-tree patch to work around this, so not happy about that either.

Both fixed, not sure if the former could have been done more cleanly/if extras is the appropriate directory to redirect to, and the latter required a fairly lengthy patch which is mostly copying functions from penlight.

It won't work on mac, since it requires MacOS 10.13 SDK to build.

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.

@mjlbach mjlbach force-pushed the init_sumneko_lua branch 3 times, most recently from d006efb to 5bb50c6 Compare January 1, 2021 04:37
@mjlbach mjlbach marked this pull request as ready for review January 1, 2021 04:38
SuperSandro2000
SuperSandro2000 reviewed Jan 1, 2021
View reviewed changes
pkgs/development/tools/sumneko-lua-language-server/default.nix Outdated Show resolved Hide resolved
pkgs/development/tools/sumneko-lua-language-server/default.nix Outdated Show resolved Hide resolved
pkgs/development/tools/sumneko-lua-language-server/default.nix Outdated
Comment on lines 24 to 31
buildPhase = ''
cd 3rd/luamake
ninja -f ninja/linux.ninja
cd ../..
./3rd/luamake/luamake rebuild
'';
Copy link
Member

Choose a reason for hiding this comment

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

I would like to use the ninja setup-hook. Can we do something like to do that:

```suggestion
  preBuildPhase = ''
    cd 3rd/luamake
  '';
  
  ninjaFlags = "-f ninja/linux.ninja";

  postBuildPhase = ''
    cd ../..
    ./3rd/luamake/luamake rebuild
  '';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried that before and I'm not sure why it isn't working

build flags: -j8 -l8 -f ninja/linux.ninja
ninja: error: loading 'ninja/linux.ninja': No such file or directory
ninja: error: loading 'ninja/linux.ninja': No such file or directory
builder for '/nix/store/i8xzv17v3n4z28yr6786k5a2gjvl78xs-sumneko-lua-language-server-1.9.0.drv' failed with exit code 1

Copy link
Member

Choose a reason for hiding this comment

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

Did you add the cd in preBuild?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is exactly what I did: https://github.com/mjlbach/nixpkgs/blob/sumneko_build_failure/pkgs/development/tools/sumneko-lua-language-server/default.nix

https://github.com/mjlbach/nixpkgs/blob/78628a94ca35b49b06d938dbe010c399ef765548/pkgs/development/tools/sumneko-lua-language-server/default.nix#L24-L34
  patching sources
  applying patch /nix/store/3fsc6p3wsv5ainwhjm22agzx13aqx99z-sumneko_home_expansion.patch
  patching file main.lua
  configuring
  no configure script, doing nothing
  building
  build flags: -j8 -l8 -f ninja/linux.ninja
  ninja: error: loading 'ninja/linux.ninja': No such file or directory
error: --- Error ---------------------------------------------------------------------------------------------------------------------------------------------------------------- nix
build of '/nix/store/i8xzv17v3n4z28yr6786k5a2gjvl78xs-sumneko-lua-language-server-1.9.0.drv' failed

Copy link
Member

Choose a reason for hiding this comment

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

Because it is preBuild and postBuild without phase. Also if you execute ninja you need to add -j$NIX_BUILD_CORES -l$NIX_BUILD_CORES

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I add those to ninja flags I get an error, am I doing it wrong?

  ninjaFlags = [
    "-f ninja/linux.ninja"
    "-j$NIX_BUILD_CORES"
    "-l$NIX_BUILD_CORES"
    ];

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this not already done? https://github.com/mjlbach/nixpkgs/blob/a57ba9ea13c2f09a467e9237c516ba22a09ae93f/pkgs/development/tools/build-managers/ninja/setup-hook.sh#L6-L13

pkgs/development/tools/sumneko-lua-language-server/default.nix Outdated Show resolved Hide resolved
pkgs/development/tools/sumneko-lua-language-server/default.nix Outdated Show resolved Hide resolved
@SuperSandro2000
Copy link
Member

This is a semi-automatic executed nixpkgs-review. If you find some bugs or got suggestions for further things to search or run please reach out to SuperSandro2000 on IRC.

Result of nixpkgs-review pr 108093 run on x86_64-linux 1

1 package built:
  • sumneko-lua-language-server

@mjlbach
Copy link
Contributor Author

mjlbach commented Jan 4, 2021

Fix was upstreamed

Just waiting on a release.

@lblasc
Copy link
Contributor

lblasc commented Jan 4, 2021

@mjlbach thx for push this into upstream, I've packaged it in my private overlay together with vscode extension it was frustrated experience.. anyway your version is much more cleaner then mine.

@mjlbach
Copy link
Contributor Author

mjlbach commented Jan 4, 2021

My version is only cleaner because of upstream patches 😆 You're welcome! Hopefully 1.9.1 gets cut soon and we can get this in.

@mjlbach mjlbach force-pushed the init_sumneko_lua branch 2 times, most recently from c858875 to 2b60e35 Compare January 5, 2021 07:33
@mjlbach
Copy link
Contributor Author

mjlbach commented Jan 5, 2021

@SuperSandro2000 This should be good to go, I removed the in-tree patch.

@mjlbach mjlbach changed the title sumneko-lua-language-server: init at 1.9.0 sumneko-lua-language-server: init at 1.10.0 Jan 5, 2021
@mjlbach mjlbach force-pushed the init_sumneko_lua branch 2 times, most recently from e09c391 to 8e36141 Compare January 10, 2021 19:43
@mjlbach mjlbach changed the title sumneko-lua-language-server: init at 1.10.0 sumneko-lua-language-server: init at 1.11.2 Jan 10, 2021
@SuperSandro2000
Copy link
Member

This is a semi-automatic executed nixpkgs-review which does not build all packages (e.g. lumo, tensorflow or pytorch)
If you find some bugs or got suggestions for further things to search or run please reach out to SuperSandro2000 on IRC.

Result of nixpkgs-review pr 108093 run on x86_64-linux 1

1 package built:
  • sumneko-lua-language-server

@SuperSandro2000 SuperSandro2000 merged commit e9bebb3 into NixOS:master Jan 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SuperSandro2000 SuperSandro2000 SuperSandro2000 left review comments

@lblasc lblasc lblasc approved these changes

Assignees
No one assigned
Labels
8.has: package (new) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@mjlbach @SuperSandro2000 @lblasc