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

vala-language-server: init at 0.48.1 #102664

Merged

Conversation

felix-andreas
Copy link
Member

Fixes #85024

Motivation for this change
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 (elementary OS)
  • 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.

@felix-andreas
Copy link
Member Author

Hi, I recently started using Nix and adding the vala-language-server seemed like a useful first exercise. The default.nix file is loosely based on #83386. I did my best to match the CONTRIBUTING.md. For now I added myself as only maintainer, but as I am a new user, I am unsure if I can guarantee to maintain it. So maybe someone else wants to add themselves to the maintainers list?

@worldofpeace
Copy link
Contributor

worldofpeace commented Nov 4, 2020

Oh hi @andreasfelix, I recall some of your contributions in the elementary community. Cool to see u here, I maintain pantheon and vala here.

@worldofpeace
Copy link
Contributor

So maybe someone else wants to add themselves to the maintainers list?

Feel free to add me, I wasn't really sure if gvls or vala-language-server was going to be defacto or something (like will Code use this one day).
You can also add the following to the expression to get semi-automatic updates

passthru = {
  updateScript = nix-update-script {
    attrPath = pname;
  };
}

and you can run

nix-shell maintainers/scripts/update.nix --argstr maintainer andreasfelix  --argstr commit true

and it will make a commit for you if there's a change upstream. See also https://nixos.org/manual/nixpkgs/stable/#ssec-stdenv-attributes passthru.updateScript

Copy link
Contributor

@worldofpeace worldofpeace left a comment

Choose a reason for hiding this comment

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

Hi, overall copying the other expression and the dependency declaration is all good.

Could we make additional changes as improvements?

It adds the following native dependencies https://github.com/benwaffle/vala-language-server/blob/master/plugins/gnome-builder/meson.build#L7

@SuperSandro2000
Copy link
Member

SuperSandro2000 commented Nov 4, 2020

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

1 package failed to build:
  • vala-language-server
Found ninja-1.10.1 at /nix/store/ba77flp1iya22w2v7yih1vwz8z4zwdnb-ninja-1.10.1/bin/ninja
Using 'STRIP' from environment with value: 'strip'
Using 'STRIP' from environment with value: 'strip'
meson: enabled parallel building
@nix { "action": "setPhase", "phase": "buildPhase" }
building
build flags: -j8 -l8
^M[0/34] Generating vala-language-server.1 with a custom commandninja: fatal: posix_spawn: No such file or directory

@worldofpeace
Copy link
Contributor

Result of nixpkgs-review pr 102664 run on x86_64-linux 1
1 package failed to build:

Found ninja-1.10.1 at /nix/store/ba77flp1iya22w2v7yih1vwz8z4zwdnb-ninja-1.10.1/bin/ninja
Using 'STRIP' from environment with value: 'strip'
Using 'STRIP' from environment with value: 'strip'
meson: enabled parallel building
@nix { "action": "setPhase", "phase": "buildPhase" }
building
build flags: -j8 -l8
^M[0/34] Generating vala-language-server.1 with a custom commandninja: fatal: posix_spawn: No such file or directory

Interesting, it passed on ofborg. Will try to reproduce

@worldofpeace
Copy link
Contributor

gnome-builder support would be cool

I looked at the installed output and there's a plugin. But it assumes ABI is 3.38, so we'd need to add those dependencies.

As for the nixpkgs-review result, I can't reproduce

@felix-andreas
Copy link
Member Author

Oh hi @andreasfelix, I recall some of your contributions in the elementary community. Cool to see u here, I maintain pantheon and vala here.

Yeees, hi there 👋 Thanks for the fast and detailed reply!

I incorporate your suggestion with the update script and also added you as additional maintainer.

gnome-builder support would be cool

done

It adds the following native dependencies https://github.com/benwaffle/vala-language-server/blob/master/plugins/gnome-builder/meson.build#L7

Is it save to assume that sh is available? I added the dash shell dependency for now.

@ofborg ofborg bot requested a review from worldofpeace November 5, 2020 16:25
@SuperSandro2000
Copy link
Member

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

1 package built:
  • vala-language-server

pkg-config
scdoc
# GNOME Builder Plugin
dash
Copy link
Contributor

Choose a reason for hiding this comment

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

the nix sandbox has /bin/sh, so I wonder if find_program https://github.com/benwaffle/vala-language-server/blob/master/plugins/gnome-builder/meson.build#L7 considers /bin, or maybe we'd have to do

preBuild = ''
  # sh for gnome-builder-plugin
  export PATH=$PATH:/bin
'';

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems to work without it: Program sh found: YES

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure if I used sand-boxing though

@ofborg ofborg bot requested a review from worldofpeace November 8, 2020 17:14
Copy link
Contributor

@worldofpeace worldofpeace left a comment

Choose a reason for hiding this comment

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

This LGTM. I checked the ofborg build logs and it said it could find sh with sandboxing.
Could you please fixup and rebase your history so that we have two commits?

maintainers: add andreasfelix
vala-language-server: init at 0.48.1

@felix-andreas felix-andreas force-pushed the andreasfelix/vala-language-server branch from 66f6545 to 49280e8 Compare November 9, 2020 02:44
@felix-andreas
Copy link
Member Author

done

@ofborg ofborg bot requested a review from worldofpeace November 9, 2020 02:54
@worldofpeace worldofpeace merged commit 00ecee0 into NixOS:master Nov 9, 2020
@worldofpeace
Copy link
Contributor

Thx for contributing @andreasfelix

@felix-andreas felix-andreas deleted the andreasfelix/vala-language-server branch November 11, 2020 14:43
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.

Package request: vala-language-server
4 participants