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

Conversation

@MetaDark
Copy link
Contributor

MetaDark 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

@MetaDark

This comment has been minimized.

Copy link
Contributor Author

MetaDark 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?

@ofborg ofborg bot requested a review from doronbehar Nov 9, 2019
@MetaDark MetaDark changed the title textlab: init at 1.6.0 texlab: init at 1.6.0 Nov 9, 2019
@MetaDark MetaDark force-pushed the MetaDark:texlab branch from fb0ea7e to b82cc0c Nov 9, 2019
@doronbehar

This comment has been minimized.

Copy link
Contributor

doronbehar commented Nov 9, 2019

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!

@MetaDark

This comment has been minimized.

Copy link
Contributor Author

MetaDark 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

This comment has been minimized.

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
@MetaDark

This comment has been minimized.

Copy link
Contributor Author

MetaDark commented Nov 10, 2019

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

Copy link
Contributor

doronbehar left a comment

Looks great now!

@veprbl

This comment has been minimized.

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.

@veprbl veprbl requested a review from marsam Nov 11, 2019
@MetaDark

This comment has been minimized.

Copy link
Contributor Author

MetaDark 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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor

doronbehar commented Nov 12, 2019

@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)..

@MetaDark MetaDark force-pushed the MetaDark:texlab branch from a0c7b33 to e4f0c27 Nov 13, 2019
@MetaDark

This comment has been minimized.

Copy link
Contributor Author

MetaDark 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

This comment has been minimized.

Copy link
Contributor

doronbehar commented Nov 13, 2019

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 Nov 13, 2019
@MetaDark MetaDark force-pushed the MetaDark:texlab branch from e4f0c27 to a4e600e Nov 13, 2019
@marsam
marsam approved these changes Nov 13, 2019
@marsam

This comment has been minimized.

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
14 of 15 checks passed
14 of 15 checks passed
texlab on aarch64-linux Failure
Details
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
texlab on x86_64-linux Success
Details
@MetaDark

This comment has been minimized.

Copy link
Contributor Author

MetaDark 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 doronbehar referenced this pull request Nov 13, 2019
@doronbehar

This comment has been minimized.

Copy link
Contributor

doronbehar commented Nov 13, 2019

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..

@MetaDark

This comment has been minimized.

Copy link
Contributor Author

MetaDark commented Nov 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.