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

texlab: init at 1.6.0 #73087

Merged
merged 1 commit into from Nov 13, 2019
Merged

texlab: init at 1.6.0 #73087

merged 1 commit into from Nov 13, 2019

Conversation

kira-bruneau
Copy link
Contributor

@kira-bruneau kira-bruneau commented Nov 9, 2019

Motivation for this change

Fixes #67934

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 nix-review --run "nix-review wip"
[118 built, 1782 copied (4690.2 MiB), 1542.7 MiB DL]
19 package were build:
antora create-cycle-app emojione hueadm image_optim iosevka joplin lumo parity-ui rambox-pro sage sageWithDoc slack slack-dark texlab thelounge triton twemoji-color-font wasm-text-gen
  • 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.
Notify maintainers

cc @doronbehar

@kira-bruneau
Copy link
Contributor Author

kira-bruneau commented Nov 9, 2019

All tests pass and I manually verified the binary using Emacs as a client.

@doronbehar I added you as a maintainer, is that ok with you?

@doronbehar
Copy link
Contributor

Looks good! I tried to figure this out last night with no luck. I have 1 comment:

What I was trying to do, based on other packages I saw that use node2nix, was writing a script such as this:

#!/usr/bin/env nix-shell
#! nix-shell -i bash -p nodePackages.node2nix jq

set -eu -o pipefail

if [ "$#" -ne 1 ] || [[ "$1" == -* ]]; then
	echo "Usage: $0 <git release tag>"
	exit 1
fi

TEXLAB_WEB_SRC="https://raw.githubusercontent.com/latex-lsp/texlab/$1"
# We use curl and `jq` inorder to make the `devDependencies` normal `dependencies`
curl --silent "$TEXLAB_WEB_SRC/src/citeproc/js/package.json" | \
	jq '. + {"dependencies": .devDependencies} | del(.devDependencies)' > package.json

wget "$TEXLAB_WEB_SRC/src/citeproc/js/package-lock.json" -O package-lock.json

node2nix \
	--nodejs-10
	-i package.json -l package-lock.json \
	--composition citeproc-packages.nix \
	-e ../../../development/node-packages/node-env.nix

However, from the diff of this PR it seems that you added a package.json from the repo and ran the node2nix script of all node packages. I'm talking about this script:

https://github.com/NixOS/nixpkgs/blob/5e70be026ee4eb05ed1c25d659848ab2f7102100/pkgs/development/node-packages/generate.sh

Right?

Maybe we can at least take the idea of this script to automate the package.json and package-lock.json download procedure?

Oh and also, the PR doesn't use package-lock.json now, right? I think we should..

@doronbehar I added you as a maintainer, is that ok with you?

Yes!

@kira-bruneau
Copy link
Contributor Author

kira-bruneau commented Nov 9, 2019

@doronbehar

However, from the diff of this PR it seems that you added a package.json from the repo and ran the node2nix script of all node packages. I'm talking about this script:

https://github.com/NixOS/nixpkgs/blob/5e70be026ee4eb05ed1c25d659848ab2f7102100/pkgs/development/node-packages/generate.sh

Right?

Maybe we can at least take the idea of this script to automate the package.json and package-lock.json download procedure?

I followed the same style used in luma and iosveka for grabbing nodejs build dependencies. The package.json I added for citeproc is only a projection of the one stored in the texlab repo. It doesn't contain prettier, tslint, or tslint-config-prettier since they aren't required for running npm run dist. This will add some extra maintenance overhead for each update and probably shouldn't be automated with a script, but I think it's worth it to reduce the closure size.

Oh and also, the PR doesn't use package-lock.json now, right? I think we should..

I didn't include a lock file for a few reasons:

  • generate.sh isn't setup to use lock files. It would have to be refactored.
  • From what I understand, package-lock.json is only used for creating a reproducible environment for development / CI. If citeproc were published to npm, the lock file would be ignored.
  • The files generated by node2nix still allow for a reproducible environment (however it may not be the exact same reproducible environment)
  • The nix expressions for dependencies can be shared with other node packages that match the same semver pattern.

@doronbehar
Copy link
Contributor

doronbehar commented Nov 9, 2019

Oh and also, the PR doesn't use package-lock.json now, right? I think we should..
I didn't include a lock file for a few reasons:

  • The files generated by node2nix still allow for a reproducible environment (however it may not be the exact same reproducible environment).
  • The nix expressions for dependencies can be shared with other node packages that match the same semver pattern.

I see 👍.

I followed the same style used in luma and iosveka for grabbing nodejs build dependencies. The package.json I added for citeproc is only a projection of the one stored in the texlab repo. It doesn't contain prettier, tslint, or tslint-config-prettier since they aren't required for running npm run dist. This will add some extra maintenance overhead for each update and probably shouldn't be automated with a script, but I think it's worth it to reduce the closure size.

That's a good reason for not including those linting dependencies but according to git grep, the node packages prettier, tslint etc. are already present in node-packages-v10.nix for example. I think it wouldn't be that significant if we'll download (only) the package.json as is from upstream with a script such as:

diff --git c/pkgs/development/tools/misc/texlab/citeproc/update-package.json.sh i/pkgs/development/tools/misc/texlab/citeproc/update-package.json.sh
new file mode 100755
index 00000000000..54fbfaccaff
--- /dev/null
+++ i/pkgs/development/tools/misc/texlab/citeproc/update-package.json.sh
@@ -0,0 +1,14 @@
+#!/usr/bin/env nix-shell
+#! nix-shell -i bash -p jq
+
+set -eu -o pipefail
+
+if [ "$#" -ne 1 ] || [[ "$1" == -* ]]; then
+	echo "Usage: $0 <git release tag>"
+	exit 1
+fi
+
+TEXLAB_WEB_SRC="https://raw.githubusercontent.com/latex-lsp/texlab/$1"
+
+curl --silent "$TEXLAB_WEB_SRC/src/citeproc/js/package.json" | \
+	jq '. + {"dependencies": .devDependencies} | del(.devDependencies)' > package.json

@kira-bruneau
Copy link
Contributor Author

kira-bruneau commented Nov 10, 2019

@doronbehar That's a good point 👍. I will try using your script.

Copy link
Contributor

@doronbehar doronbehar left a comment

Choose a reason for hiding this comment

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

Looks great now!

@veprbl
Copy link
Member

veprbl commented Nov 11, 2019

Need to resolve the merge conflict. Preferably by not applying irrelevant changes to node-packages-vXX.nix.

@kira-bruneau
Copy link
Contributor Author

kira-bruneau commented Nov 11, 2019

@veprbl The node-packages-vXX.nix files are generated from a script. Should I just selectively exclude dependencies not used by texlab?

@veprbl
Copy link
Member

veprbl commented Nov 12, 2019

@MetaDark Yes, that's my advice. This should help to limit the PR bitrot until it's reviewed/merged.

@doronbehar
Copy link
Contributor

@veprbl Do you conceive a way to make sure the conflicts will always be resolved without making manual changes to the auto generated "Do not edit!" node-packages-v##.nix files? I tend to think there isn't a safe way to do it. Hence the only way this PR could be merged is if someone with merge rights will merge it as soon as it will be updated (by @MetaDark running once more node2nix)..

@kira-bruneau
Copy link
Contributor Author

kira-bruneau commented Nov 13, 2019

I ran into problems trying to manually remove unneeded changes, so I just decided to rebuild the whole expression again.

I also started working on a tool to try to support partial builds. It's not complete yet, but it could be helpful in the future.

@doronbehar
Copy link
Contributor

I hope there won't be a need for that, perhaps @marsam would be willing to review and merge this quickly? As this is a bit urgent, it will be harder to merge this once there will be merge conflicts..

@ofborg ofborg bot requested a review from doronbehar November 13, 2019 12:37
@marsam
Copy link
Contributor

marsam commented Nov 13, 2019

Thanks for working in this, and sorry for the delay

I also started working on a tool to try to support partial builds. It's not complete yet, but it could be helpful in the future.

I would also like to work on this in the future, right now node-packages/node-packages-vXX.nix only lists the packages without version, I was thinking that we could keep a package.json and package-lock.json in nixpkgs to track only the related updates

@marsam marsam merged commit 854f51b into NixOS:master Nov 13, 2019
@kira-bruneau
Copy link
Contributor Author

kira-bruneau commented Nov 13, 2019

@marsam That definitely sounds simpler than what I was originally planning. I was thinking of parsing node-packages.nix. Although, the array style used in node-packages-vXX.json isn't part of the package.json specification: https://docs.npmjs.com/files/package.json. So it would probably have to be generated manually instead of through npm install.

@doronbehar
Copy link
Contributor

That was my original approach towards packaging this but @MetaDark has superseded me. I don't have a strong opinion as for the matter but you do point out an inconsistency..

@kira-bruneau
Copy link
Contributor Author

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.

packaging request: texlab
4 participants