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

deps [nfc]: Drop resolution for node-gyp under sqlite3 #5412

Merged
merged 2 commits into from
Jun 14, 2022

Conversation

gnprice
Copy link
Member

@gnprice gnprice commented Jun 13, 2022

We introduced this line in a7d19c0 "deps: Force sqlite3 to use a
recent node-gyp", at a time when the sqlite3 package unnecessarily
required a much older version, [email protected].

There's now a sqlite3 release (which we've taken) that updates that
dependency to [email protected], the same thing our resolution was
requiring. So drop the resolution and let the package's own metadata
govern from here.

This doesn't immediately change what version we actually use anywhere.
Before and after the change, yarn why node-gyp reports:

=> Found "[email protected]"
info Reasons this module exists

  • "sqlite3" depends on it
  • Hoisted from "sqlite3#node-gyp"

    => Found "ttf2woff2#[email protected]"
    info This module exists because "@vusion#webfonts-generator#ttf2woff2" depends on it.

@gnprice gnprice mentioned this pull request Jun 13, 2022
chrisbobbe and others added 2 commits June 13, 2022 19:14
This should have been included in the recent 815d9f1; oops.
We introduced this line in a7d19c0 "deps: Force sqlite3 to use a
recent node-gyp", at a time when the sqlite3 package unnecessarily
required a much older version, [email protected].

There's now a sqlite3 release (which we've taken) that updates that
dependency to [email protected], the same thing our resolution was
requiring.  So drop the resolution and let the package's own metadata
govern from here.

This doesn't immediately change what version we actually use anywhere.
Before and after the change, `yarn why node-gyp` reports:

=> Found "[email protected]"
info Reasons this module exists
   - "sqlite3" depends on it
   - Hoisted from "sqlite3#node-gyp"
…
=> Found "ttf2woff2#[email protected]"
info This module exists because "@vusion#webfonts-generator#ttf2woff2" depends on it.
@chrisbobbe chrisbobbe force-pushed the pr-resolution-sqlite3 branch from 0f3ca09 to 70b929e Compare June 13, 2022 23:16
@chrisbobbe
Copy link
Contributor

chrisbobbe commented Jun 13, 2022

LGTM, thanks!

In checking this out and tools/testing it, I realized that in my yarn upgrade PR, I'd just run yarn upgrade without then running yarn afterward, which triggers the postinstall script. So I've added a commit with the result of that: some changes to Podfile.lock, which I've pushed here. If all looks good, please merge at will.

@chrisbobbe
Copy link
Contributor

Hmm, a flake; just filed #5414 and will send a PR shortly.

@gnprice
Copy link
Member Author

gnprice commented Jun 14, 2022

Thanks!

There was a CI failure. Looks like it was all from Jest failures in one test file, all looking like this first one:

PASS android src/webview/__tests__/generateInboundEventEditSequence-test.js (12.191 s)
172
FAIL ios src/webview/__tests__/generateInboundEventEditSequence-test.js (9.761 s)
173
  ● messages -> piece descriptors -> content HTML is stable/sensible › other interesting cases (single messages) › message in unsubscribed stream
174
175
    expect(received).toMatchSnapshot()
176
177
    Snapshot name: `messages -> piece descriptors -> content HTML is stable/sensible other interesting cases (single messages) message in unsubscribed stream 1`
178
179
    - Snapshot  - 2
180
    + Received  + 2
181
182
    @@ -1,12 +1,12 @@
183
      <div class="msglist-element timerow" data-msg-id="-1">
184
        <div class="timerow-left"></div>
185
        Dec 31, 1969
186
        <div class="timerow-right"></div>
187
      </div><div class="msglist-element header-wrapper header stream-header topic-header" data-msg-id="-1" data-narrow="dG9waWM6MTpleGFtcGxlIHRvcGlj">
188
    -   <div class="header stream-text" style="color: black;
189
    +   <div class="header stream-text" style="color: white;
190
    -               background: hsl(0, 0%, 80%)" data-narrow="c3RyZWFtOjE=">
191
    +               background: #123456" data-narrow="c3RyZWFtOjE=">
192
          # stream 1
193
        </div>
194

Now #123456 is the default color in eg.makeSubscription. So it seems like that test was ending up with a subscription to that stream by accident.

That's a flake we should fix, but seems clearly unrelated to these changes. I also just ran tools/test --all locally, and everything passed. So, going ahead and merging.

@gnprice gnprice merged commit 70b929e into zulip:main Jun 14, 2022
@gnprice
Copy link
Member Author

gnprice commented Jun 14, 2022

Hmm, a flake; just filed #5414 and will send a PR shortly.

Ah excellent -- I was going to do the same (rather, file an issue), but you'd already done so while I was writing my comment :)

@gnprice gnprice deleted the pr-resolution-sqlite3 branch June 14, 2022 00:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants