Google clasp #43376

michojel · 2018-07-11T18:13:37Z

Motivation for this change

Upstream's github page says it all:

🗺️ Develop Locally: clasp allows you to develop your Apps Script projects locally. That means you can check-in your code into source control, collaborate with other developers, and use your favorite tools to develop Apps Script.

🔢 Manage Deployment Versions: Create, update, and view your multiple deployments of your project.

📁 Structure Code: clasp automatically converts your flat project on script.google.com into folders.

Things done

Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
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 nox --run "nox-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)
Fits CONTRIBUTING.md.

michojel · 2018-07-26T08:40:49Z

Rebased.

Not sure whom to ping. @svanderburg would you be so kind to take a look or perhaps suggest another reviewer?
Note: this is my first contribution to NixOs.
Thanks!

mmahut · 2019-08-07T21:14:38Z

Any updates on this pull request, please?

michojel · 2019-08-08T08:13:43Z

Resurrected, but do not merge yet. Need to resolve a TypeError first:

clasp --version
/nix/store/q90cs1sqmjhg2sdksqal5qpyryag9hhs-node-_at_google_slash_clasp-2.2.1/lib/node_modules/@google/clasp/src/dotfile.js:97
    RC: dotf(exports.DOT.RC.DIR, exports.DOT.RC.NAME),
        ^

TypeError: dotf is not a function
    at Object.<anonymous> (/nix/store/q90cs1sqmjhg2sdksqal5qpyryag9hhs-node-_at_google_slash_clasp-2.2.1/lib/node_modules/@google/clasp/src/dotfile.js:97:9)
    at Module._compile (internal/modules/cjs/loader.js:701:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:712:10)
    at Module.load (internal/modules/cjs/loader.js:600:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:539:12)
    at Function.Module._load (internal/modules/cjs/loader.js:531:3)
    at Module.require (internal/modules/cjs/loader.js:637:17)
    at require (internal/modules/cjs/helpers.js:22:18)
    at Object.<anonymous> (/nix/store/q90cs1sqmjhg2sdksqal5qpyryag9hhs-node-_at_google_slash_clasp-2.2.1/lib/node_modules/@google/clasp/src/utils.js:52:17)
    at Module._compile (internal/modules/cjs/loader.js:701:30)

Useful for developing Google's Apps Scripts. Signed-off-by: Michal Minář <mic.liamg@gmail.com>

michojel · 2019-08-18T08:49:02Z

Resurrected, but do not merge yet. Need to resolve a TypeError first:

Fixed by a patch.

@turboMaCk, @domenkozar PTAL (adding people having some experience with node packages, feel free to tag others)

Signed-off-by: Michal Minář <mic.liamg@gmail.com>

turboMaCk

I'm not a commiter btw so just a review.

builds and runs on NixOS
builds and runs on MacOS

I've found 2 minor things in the diff I'm unsure about but that's for someone more experienced to decide. I don't think either is a blocker for merge.

I would recommend squishing commits before merge.

turboMaCk · 2019-08-18T09:43:27Z

pkgs/development/misc/google-clasp/default.nix

+})."@google/clasp-${version}".override rec {
+  preRebuild = ''
+    patch -p1 <<<"${builtins.readFile ./dotf.patch}"
+  '';


I'm not sure but there might be hook patches to which you can just simply list files. But I don't think I ever used it in case of node2nix.

Thanks @turboMaCk. The unpacking of node packages takes place in the installPhase:

checking outputs of '/nix/store/bd9fs5z96ypszcn0zqmh9lp2l29m51sp-node-_at_google_slash_clasp-2.2.1.drv'... unpacking sources patching sources configuring no configure script, doing nothing building installing unpacking source archive /nix/store/xzbw3n2j4zq5nh1j4dkx0aqn1mr629lp-clasp-2.2.1.tgz unpacking source archive /nix/store/ibwdid42ly8x7xidcvp0jr7jnnm281lf-fs.scandir-2.1.1.tgz unpacking source archive /nix/store/zfpb0hszl9827qmhbkhy3sksrlyfnja0-fs.stat-2.0.1.tgz unpacking source archive /nix/store/33qz58lq0f86hcbdi13jp04ndwp4c824-fs.walk-1.2.2.tgz ...

The patchPhase does not have the sources available yet.

The nodePackages seem to patch the sources during in the preBuild phase:

https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/node-packages/default-v10.nix#L47

https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/node-packages/default-v10.nix#L71

So this can be inconsistent either with a regular nix package build or node-package build. An alternative approach to this would be:

list the package in node-packages-v10.json

add another override like (pnpm = nodePackages.pnpm.override) in the default-v10.nix

I don't have a strong opinion on this one.

@adisbladis toughts?

I was playing with this and I think the solution you come up with is fine and most straight forward at the moment.

Seems fine for now. However I'd like to refresh my knowledge about the node2nix codebase first to check if there's a proper workaround (otherwise I'd file an issue there later).

turboMaCk · 2019-08-18T09:46:30Z

pkgs/development/misc/google-clasp/generate.sh

+exec node2nix --nodejs-10 \
+    -i node-packages.json -o node-packages.nix \
+    -c google-clasp.nix \
+     --no-copy-node-env -e ../../../development/node-packages/node-env.nix


not sure if it's preferable to share node-env file this way or not. Can anyone more experienced clarify this?

Two other occurrences:

nixpkgs/pkgs/tools/package-management/nixui/generate.sh

Line 4 in 0e296d5

exec node2nix -8 -i pkg.json -c nixui.nix -e ../../../development/node-packages/node-env.nix --no-copy-node-env

nixpkgs/pkgs/tools/networking/airfield/deps.sh

Line 6 in 0e296d5

-e ../../../development/node-packages/node-env.nix -c node.nix

From my point of view, the less redundancy, the better.

thanks for references. My thinking was that this creates non obvious coupling between what appears as unrelated modules. Anyway this is not really up to me to decide so unless someone more experienced comes in and gives definite answer I would keep it as is.

IIRC we use a single node2nix in nixpkgs (hence the same node-env.nix) for everything, so this should be fine IMHO.
In the end we already have a lot of large diffs because of that, so I'd prefer to use the node-env.nix we already have unless we encounter issues that can be resolved by committing another one : )

turboMaCk · 2019-08-18T10:07:49Z

@GrahamcOfBorg build google-clasp

michojel · 2019-08-20T07:31:26Z

@Ma27 could you please take a look?

Ma27 · 2019-08-20T08:18:41Z

After skimming through the diff, this appears to be fine. Before merging, I'd love to test this first. I'll probably be able to do this tonight, as soon as I have a better uplink to download all needed dependencies :)

Ma27

LGTM. Unless there are further objections, I'd merge tomorrow :)

michojel · 2019-08-22T13:23:13Z

Thanks @Ma27 and @turboMaCk!

michojel force-pushed the google-clasp branch from e45a787 to c6ad896 Compare July 11, 2018 18:15

GrahamcOfBorg added 8.has: package (new) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 labels Jul 11, 2018

michojel force-pushed the google-clasp branch from c6ad896 to bc7ab53 Compare July 26, 2018 08:37

GrahamcOfBorg added 10.rebuild-darwin: 1-10 10.rebuild-linux: 11-100 and removed 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 labels Jul 26, 2018

xeji added the 2.status: merge conflict label Aug 25, 2018

michojel force-pushed the google-clasp branch from bc7ab53 to 9c8fe6b Compare August 8, 2019 07:59

ofborg bot removed the 2.status: merge conflict label Aug 8, 2019

michojel force-pushed the google-clasp branch 2 times, most recently from a2d7e1b to 9485df1 Compare August 18, 2019 08:38

google-clasp: init at version 2.2.1

fc72aae

Useful for developing Google's Apps Scripts. Signed-off-by: Michal Minář <mic.liamg@gmail.com>

michojel force-pushed the google-clasp branch from 9485df1 to fc72aae Compare August 18, 2019 08:47

fixed license

69917c6

Signed-off-by: Michal Minář <mic.liamg@gmail.com>

ofborg bot added 11.by: package-maintainer 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 and removed 10.rebuild-darwin: 1-10 10.rebuild-linux: 11-100 labels Aug 18, 2019

turboMaCk approved these changes Aug 18, 2019

View reviewed changes

Ma27 approved these changes Aug 20, 2019

View reviewed changes

Ma27 merged commit f0c12c4 into NixOS:master Aug 22, 2019

turboMaCk mentioned this pull request Aug 25, 2019

elmPackages: share node-env.nix, add elm-live & elm-xref #67439

Merged

10 tasks

Janik-Haag added the 12. first-time contribution label Jun 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Google clasp #43376

Google clasp #43376

michojel commented Jul 11, 2018

michojel commented Jul 26, 2018

mmahut commented Aug 7, 2019

michojel commented Aug 8, 2019

michojel commented Aug 18, 2019

turboMaCk left a comment •

edited

turboMaCk Aug 18, 2019

michojel Aug 19, 2019

turboMaCk Aug 20, 2019

Ma27 Aug 20, 2019

turboMaCk Aug 18, 2019

michojel Aug 19, 2019

turboMaCk Aug 20, 2019

Ma27 Aug 20, 2019

turboMaCk commented Aug 18, 2019

michojel commented Aug 20, 2019

Ma27 commented Aug 20, 2019

Ma27 left a comment

michojel commented Aug 22, 2019

Google clasp #43376

Google clasp #43376

Conversation

michojel commented Jul 11, 2018

Motivation for this change

Things done

michojel commented Jul 26, 2018

mmahut commented Aug 7, 2019

michojel commented Aug 8, 2019

michojel commented Aug 18, 2019

turboMaCk left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turboMaCk commented Aug 18, 2019

michojel commented Aug 20, 2019

Ma27 commented Aug 20, 2019

Ma27 left a comment

Choose a reason for hiding this comment

michojel commented Aug 22, 2019

turboMaCk left a comment •

edited