Skip to content

Conversation

sarahec
Copy link
Contributor

@sarahec sarahec commented Feb 14, 2025

Replaced by #386207

13.30.0 -> 13.31.2

Enabled auto-updates via passthru.updateScript

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • 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/)
  • 25.05 Release Notes (or backporting 24.11 and 25.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.

Add a 👍 reaction to pull requests you find important.

@sarahec sarahec force-pushed the firebase-tools-13-31-1 branch from b486477 to 8780ae6 Compare February 14, 2025 19:16
@github-actions github-actions bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Feb 14, 2025
@github-actions github-actions bot added 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. labels Feb 14, 2025
@momeemt
Copy link
Member

momeemt commented Feb 15, 2025

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 382118


aarch64-darwin

❌ 1 package failed to build:
  • firebase-tools

Copy link
Member

@momeemt momeemt left a comment

Choose a reason for hiding this comment

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

It failed to build by the following error even though linking package-lock.json in postPatch, I'll look into the cause.

❯ nix-build -A firebase-tools
this derivation will be built:
  /nix/store/ks3yf0x7jgljpwd86rylxl62iy8sbcf5-firebase-tools-13.31.1.drv
building '/nix/store/ks3yf0x7jgljpwd86rylxl62iy8sbcf5-firebase-tools-13.31.1.drv'...
Running phase: unpackPhase
unpacking source archive /nix/store/6m5h6142z34fmwan06sg5vfq0s9nib3i-source
source root is source
Running phase: patchPhase
Executing npmConfigHook
Configuring npm
Validating consistency between /private/tmp/nix-build-firebase-tools-13.31.1.drv-0/source/package-lock.json and /nix/store/ynq87csgm3k1ay9gw6m9yjirp56h70f6-firebase-tools-13.31.1-npm-deps/package-lock.json
Fixing lockfile
\Installing dependencies
npm error code ENOTCACHED
npm error request to https://registry.npmjs.org/ajv-formats failed: cache mode is 'only-if-cached' but no cached response is available.
npm error Log files were not written due to an error writing to the directory: /nix/store/ynq87csgm3k1ay9gw6m9yjirp56h70f6-firebase-tools-13.31.1-npm-deps/_logs
npm error You can rerun the command with `--loglevel=verbose` to see the logs in your terminal

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" ]`

error: builder for '/nix/store/ks3yf0x7jgljpwd86rylxl62iy8sbcf5-firebase-tools-13.31.1.drv' failed with exit code 1;
       last 10 log lines:
       > npm error Log files were not written due to an error writing to the directory: /nix/store/ynq87csgm3k1ay9gw6m9yjirp56h70f6-firebase-tools-13.31.1-npm-deps/_logs
       > npm error You can rerun the command with `--loglevel=verbose` to see the logs in your terminal
       >
       > 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" ]`
       >
       For full logs, run 'nix-store -l /nix/store/ks3yf0x7jgljpwd86rylxl62iy8sbcf5-firebase-tools-13.31.1.drv'.

Copy link
Contributor Author

@sarahec sarahec left a comment

Choose a reason for hiding this comment

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

This is unnecessary, r-ryantm can handle updates without an explicit update script.

Thank you, I'll drop this change.

@sarahec sarahec force-pushed the firebase-tools-13-31-1 branch from 8780ae6 to b8fcb0c Compare February 15, 2025 22:24
@sarahec
Copy link
Contributor Author

sarahec commented Feb 15, 2025

It failed to build by the following error even though linking package-lock.json in postPatch, I'll look into the cause.

Well, nuts. It worked for me yesterday but not today. Thank you for looking into this.

@sarahec sarahec force-pushed the firebase-tools-13-31-1 branch from b8fcb0c to 317ed81 Compare February 19, 2025 19:58
@aspauldingcode
Copy link
Contributor

Hi @sarahec, I see I am requested for review.
How do I do this?

@sarahec
Copy link
Contributor Author

sarahec commented Feb 20, 2025

Hi @sarahec, I see I am requested for review. How do I do this?

That request was by accident, this picked up an unrelated code-cursor change that I then fixed.

@vkryachko
Copy link

Upon some review, I've noticed that firebase-tools updated their package.json in the following way:

https://github.com/firebase/firebase-tools/blob/dad1b1a23daaed226ca6dde257c4fcf14acbb4f3/package.json#L260-L268

This resulted in their lock file to contain entries that don't seem to be supported by fetchNpmDeps, the lock file ended up like this:

Screenshot 2025-02-21 at 10 41 16 AM

Notice how the link moved into the version. I am not sure why this was reflected in the lock file this way, but it looks like npm does it this way.

I tried running this jq in postPatch:

jq '.packages |= map_values(
      if (. | type) == "object" and (.dependencies | type) == "object" then
        .dependencies |= map_values(
          if (. | type) == "object" and (.version | startswith("https"))
          then 
            .resolved = .version | .version |= capture(".*-(?<version>[^-]+)\\.tgz").version
          else 
            .
          end
        )
      else . end)' npm-shrinkwrap.json | sponge npm-shrinkwrap.json

But it did not fix the problem. Any ideas on how to overcome this?

@joehan
Copy link

joehan commented Feb 21, 2025

firebase-tools maintainer here - chiming in to keep an eye on this and help out if needed.

The shrinkwrap changes that @vkryachko mentioned were a result of adding overrides for ajv and ajv-format (https://github.com/firebase/firebase-tools/blob/master/package.json#L261).

@sarahec
Copy link
Contributor Author

sarahec commented Feb 25, 2025

@joehan could you review firebase/firebase-tools#8253 ? If it looks good, I can fettchpatch or bump this to the release containing it.

@sarahec sarahec changed the title firebase tools: 13.30.0 -> 13.31.1 firebase tools: 13.30.0 -> 13.31.2 Feb 25, 2025
@vkryachko
Copy link

@joehan could you review firebase/firebase-tools#8253 ? If it looks good, I can fettchpatch or bump this to the release containing it.

Hi @sarahec , thanks for taking a look at this issue. I am wondering how have you arrived at this patch? Was it a manual change? if not, would be very interested to know how you did it and not recreating the lock file from scratch :)

Also running npm i on your change results in this diff, worth including?

--- a/npm-shrinkwrap.json
+++ b/npm-shrinkwrap.json
@@ -21059,6 +21059,8 @@
           "integrity": "sha512-hTdwr+7yYNIT5n4AMYp85KA6yw2Va0FLa3Rguvbpa4W3I5x>
           "dev": true,
           "requires": {
+            "ajv": "^8.17.1",
+            "ajv-formats": "3.0.1",
             "tslib": "^1.9.0"
           }
         },
@@ -24875,7 +24877,7 @@
       "resolved": "https://registry.npmjs.org/ajv-formats/-/ajv-formats-3.0.1.>
       "integrity": "sha512-8iUql50EUR+uUcdRQ3HDqa6EVyo3docL8g5WJ3FNcWmu62IbkGU>
       "requires": {
-        "ajv": "^8.0.0"
+        "ajv": "^8.17.1"
       }
     },
     "ansi-align": {

@joehan
Copy link

joehan commented Feb 25, 2025

Thanks for the contribution @sarahec - I did some testing on it this morning and it looks good to me. I ran npm i to stabilize npm-shirnkwrap (required for our CI), and merged the change. This would be included in the next release of firebase-tools sometime later this week.

@sarahec
Copy link
Contributor Author

sarahec commented Feb 25, 2025

@vkryachko I applied this as a manual change. The shrinkwrap has many such changes due to the age of some of its components. @joehan just integrated your suggestion upstream, so we'll wait for that.

@joehan thank you. I'll await the release later this week and bump this patch one last time.

@sarahec sarahec force-pushed the firebase-tools-13-31-1 branch from d15f92c to 54509d8 Compare March 1, 2025 00:20
@sarahec
Copy link
Contributor Author

sarahec commented Mar 1, 2025

@joehan Thanks for all your help upstream. It's time to put this to bed.

@sarahec sarahec requested review from momeemt and NickCao March 1, 2025 00:23
@sarahec sarahec force-pushed the firebase-tools-13-31-1 branch from 54509d8 to 68fbe42 Compare March 1, 2025 20:44
@sarahec sarahec mentioned this pull request Mar 1, 2025
13 tasks
@sarahec
Copy link
Contributor Author

sarahec commented Mar 1, 2025

Replaced by #386207