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

protoc-gen-es: init at 1.10.0, protoc-gen-connect-es: init at 1.4.0 #243432

Merged
merged 4 commits into from
Jun 24, 2024

Conversation

felschr
Copy link
Member

@felschr felschr commented Jul 14, 2023

Description of changes

Add 2 protoc plugins for the ECMAScript ecosystem developed by Buf for use with Protobuf & Connect:
protoc-gen-es
protoc-gen-connect-es

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 101-500 10.rebuild-linux: 101-500 labels Jul 14, 2023
@felschr
Copy link
Member Author

felschr commented Jul 14, 2023

For some reason the npm deps for protoc-gen-es fail to build:

unpacking sources
unpacking source archive /nix/store/12md47d55g0an07msckkazlynw6y1fz4-source
source root is source
patching sources
updateAutotoolsGnuConfigScriptsPhase
configuring
no configure script, doing nothing
building
DEBUG 1, out: "/nix/store/k7pzr462jply2x2mrjs9gg9ad1jpy8z1-protoc-gen-es-1.2.1-npm-deps"
Error: No such file or directory (os error 2)

I've added some logging to prefetch-npm-deps which can be seen in the output above:

https://github.com/NixOS/nixpkgs/blob/a226f103d3ed01e04c68d1a5f0aaf424722fcfcd/pkgs/build-support/node/fetch-npm-deps/src/main.rs#L259-L265

I've tried creating the $out directory before invoking prefetch-npm-deps or within prefetch-npm-deps, but then it'll get a different os error 2 during the consistency validation instead:

Validating consistency between /build/source/package-lock.json and /nix/store/k75l3phibjdsqgnbwfgz7mfwrn6r8rhn-protoc-gen-es-1.2.1-npm-deps/package-lock.json
Error: IO error for operation on /nix/store/k75l3phibjdsqgnbwfgz7mfwrn6r8rhn-protoc-gen-es-1.2.1-npm-deps/_cacache/index-v5: No such file or directory (os error 2)

Caused by:
    No such file or directory (os error 2)

The build for protoc-gen-connect-es works fine, though, and I can't figure out why it fails for protoc-gen-es, especially since the setup is basically identical.

@felschr
Copy link
Member Author

felschr commented Jul 14, 2023

@winterqt, @lilyinstarlight, do you have any idea what might cause this?

@felschr
Copy link
Member Author

felschr commented Jul 14, 2023

I just noticed, I've used the wrong npmDepsHash for a while. After fixing it I now always get the os error 2 for missing [...]/_cacache/index-v5 after the "Validating consistency" step, even without adding mkdir -p $out before invoking prefetch-npm-deps.

The exact place where it fails is in prefetch-npm-deps invoked by @prefetchNpmDeps@ --map-cache in this line:

@felschr
Copy link
Member Author

felschr commented Jul 14, 2023

@ofborg build protoc-gen-es

@lilyinstarlight
Copy link
Member

lilyinstarlight commented Jul 14, 2023

@felschr, these upstream lockfiles are busted and need to be deleted+regenerated. They contain hardly any resolved URLs or integrity hashes which are required to be able to cache them

The connect-es repo only works now because the package-lock.json in the latest release had that info, but it has since disappeared in the main branch since connectrpc/connect-es@411bba5

You'll probably either need to vendor a working package-lock.json or bug upstream to fix their package-lock.json files (are they using some old npm version or something? why would that information disappear?)

We definitely need to have a better error message for this case in prefetch-npm-deps though, so I'll open a PR in a bit to detect and error out in this scenario

@felschr
Copy link
Member Author

felschr commented Jul 14, 2023

@lilyinstarlight, ah, thank you. It's indeed been quite painful to debug this.

@lilyinstarlight
Copy link
Member

@lilyinstarlight, ah, thank you. It's indeed been quite painful to debug this.

Yeah, sorry about that 😅

I've opened #243458 with a better error message and an escape hatch if someone really was trying to generate an empty cache for some reason

@felschr
Copy link
Member Author

felschr commented Jul 14, 2023

I've updated the PR to fix the package-lock.json, but I get an error now, that is similar to #223964, but I actually have the same package in different versions:

Installing dependencies
npm WARN deprecated @types/lz-string@1.5.0: This is a stub types definition. lz-string provides its own type definitions, so you do not need this installed.
npm ERR! code ENOTCACHED
npm ERR! request to https://registry.npmjs.org/typescript failed: cache mode is 'only-if-cached' but no cached response is available.

npm ERR! A complete log of this run can be found in:
npm ERR!     /build/cache/_logs/2023-07-14T15_29_02_932Z-debug-0.log

ERROR: npm failed to install dependencies

Here are a few things you can try, depending on the error:
1. Set `makeCacheWritable = true`
  Note that this won't help if npm is complaining about not being able to write to the logs directory -- look above that for the actual error.
2. Set `npmFlags = [ "--legacy-peer-deps" ]`

The typescript package is specified multiple times with different versions:
https://github.com/NixOS/nixpkgs/blob/4743531e505c34c29267d17460c7d9f04c61f63d/pkgs/development/tools/protoc-gen-es/package-lock.json#L5847-L5972

@felschr
Copy link
Member Author

felschr commented Jul 14, 2023

Same problem when I completely regenerate the package-lock.json instead of using npm-lockfile-fix.

@lilyinstarlight
Copy link
Member

Same problem when I completely regenerate the package-lock.json instead of using npm-lockfile-fix.

Did you update the npmDepsHash? I just downloaded the diff from this PR and the hash was wrong on my system

After fixing the hash, the build still fails but that's due to missing type stubs for typescript

@felschr
Copy link
Member Author

felschr commented Jul 15, 2023

Did you update the npmDepsHash? I just downloaded the diff from this PR and the hash was wrong on my system

Might be, but I still get the cache error even with the correct hash.
I've now updated this PR to protoc-gen-es 1.3.0 which includes the fixed package-lock.json and it's the same error there:

Installing dependencies
npm WARN deprecated @types/lz-string@1.5.0: This is a stub types definition. lz-string provides its own type definitions, so you do not need this installed.
npm ERR! code ENOTCACHED
npm ERR! request to https://registry.npmjs.org/typescript failed: cache mode is 'only-if-cached' but no cached response is available.

@felschr
Copy link
Member Author

felschr commented Jul 17, 2023

@ofborg build protoc-gen-es protoc-gen-connect-es

@felschr
Copy link
Member Author

felschr commented Nov 2, 2023

Moved the packages to pkgs/by-name now.

@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Mar 20, 2024
@jtszalay
Copy link
Contributor

👋 Hi! I'm looking to use the same npm packages via nix. Is there anything that can be done to move this PR along?

@felschr
Copy link
Member Author

felschr commented Jun 21, 2024

@jtszalay I'm actually resuming work on the project where I need these dependencies.
I'll update this PR and then I just need someone to review it.

@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Jun 22, 2024
@felschr felschr force-pushed the protoc-gen-connect-es branch 3 times, most recently from 4cc200b to a24c9b5 Compare June 22, 2024 12:46
@felschr
Copy link
Member Author

felschr commented Jun 22, 2024

I've updated the packages to the latest versions and automated the npm-lockfile-fix including the update scripts.

@jtszalay would you be interested in maintaining these packages with me?

@felschr felschr changed the title protoc-gen-es: init at 1.4.1, protoc-gen-connect-es: init at 1.1.3 protoc-gen-es: init at 1.10.0, protoc-gen-connect-es: init at 1.4.0 Jun 22, 2024
@felschr
Copy link
Member Author

felschr commented Jun 22, 2024

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

2 packages built:
  • protoc-gen-connect-es
  • protoc-gen-es

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/1016/149

@jtszalay
Copy link
Contributor

@felschr We'll be using them at my work so, yes, I can do that. 👍

@felschr
Copy link
Member Author

felschr commented Jun 22, 2024

@jtszalay Awesome. You'll need to add yourself to the maintainer list first.

@jtszalay
Copy link
Contributor

@felschr I've submitted a PR for that now
#321883

@drupol drupol merged commit 35ee941 into NixOS:master Jun 24, 2024
24 checks passed
@felschr felschr deleted the protoc-gen-connect-es branch June 24, 2024 12:04
@jtszalay
Copy link
Contributor

jtszalay commented Jun 24, 2024

@felschr I started pulling this into my repo but I see the following:

error: collision between /nix/store/ds3v28pivv80lvwr4xfa39g9hjrar015-protoc-gen-connect-es-1.4.0/lib/node_modules/null/dist/cjs/src/typescript.js' and /nix/store/yrh9ikqfchzg00dv8j7i2gxirdvylh6x-protoc-gen-es-1.10.0/lib/node_modules/null/dist/cjs/src/typescript.js'

@felschr
Copy link
Member Author

felschr commented Jun 24, 2024

@jtszalay I can't reproduce that issue. I was able to add both dependencies to a dev shell without issues. If you referenced the packages from this PR before, you might have to garbage collect your nix store.

However, I do get a runtime error when trying to run protoc-gen-es (protoc-gen-connect-es works):

protoc-gen-es
node:internal/modules/cjs/loader:1146
  throw err;
  ^
                                                                                                                                            
Error: Cannot find module '@bufbuild/protoplugin'
Require stack:
- /nix/store/yrh9ikqfchzg00dv8j7i2gxirdvylh6x-protoc-gen-es-1.10.0/lib/node_modules/null/bin/protoc-gen-es
    at Module._resolveFilename (node:internal/modules/cjs/loader:1143:15)
    at Module._load (node:internal/modules/cjs/loader:984:27)
    at Module.require (node:internal/modules/cjs/loader:1231:19)
    at require (node:internal/modules/helpers:179:18)
    at Object.<anonymous> (/nix/store/yrh9ikqfchzg00dv8j7i2gxirdvylh6x-protoc-gen-es-1.10.0/lib/node_modules/null/bin/protoc-gen-es:3:21)
    at Module._compile (node:internal/modules/cjs/loader:1369:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1427:10)
    at Module.load (node:internal/modules/cjs/loader:1206:32)
    at Module._load (node:internal/modules/cjs/loader:1022:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:135:12) {
  code: 'MODULE_NOT_FOUND',
  requireStack: [
    '/nix/store/yrh9ikqfchzg00dv8j7i2gxirdvylh6x-protoc-gen-es-1.10.0/lib/node_modules/null/bin/protoc-gen-es'
  ]
}

I'm looking into this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes needs_reviewer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants