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

Some build fixes (changed sq API, and a missing file on iOS) #5896

Merged
merged 2 commits into from
Oct 1, 2024

Conversation

chrisbobbe
Copy link
Contributor

I ran into the sq changes by installing sq on my new laptop; this was the first release build I've done on it.

Comment on lines 13 to 14
( cd node_modules/react-native/packages/react-native-codegen/ && \
yarn clean && node scripts/build.js )
Copy link
Member

Choose a reason for hiding this comment

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

Given the set -e, this doesn't need &&:

Suggested change
( cd node_modules/react-native/packages/react-native-codegen/ && \
yarn clean && node scripts/build.js )
(
cd node_modules/react-native/packages/react-native-codegen/
yarn clean
node scripts/build.js
)

(Or maybe the latter two commands can stay in && form to stick closer to the yarn build script.)

Then a comment mentioning that this is based on the package's yarn build would be helpful. Otherwise it's left a bit mysterious why we do these particular steps.

@@ -57,8 +57,8 @@ filter_sq_stderr() {
grep -vE '^(Compressed|Encrypted) '
}

sq decrypt --recipient-key "${recipient_key}" \
-o "${cleartext}" "${cryptotext}" \
sq decrypt --recipient-file "${recipient_key}" \
Copy link
Member

@gnprice gnprice Sep 30, 2024

Choose a reason for hiding this comment

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

The Sequoia CLI's changelog doesn't seem to mention these changes:
  https://gitlab.com/sequoia-pgp/sequoia-sq/-/blob/b4158f40dea64655cf7357d3c8f220e8bd1708d9/NEWS

But they're easy enough to adapt to; shrug.

Looks like this was here:
https://gitlab.com/sequoia-pgp/sequoia/-/commit/db90178d5b61e3310f979fdaefc66610adf0d33e
https://gitlab.com/sequoia-pgp/sequoia/-/issues/933

Happened a couple of years ago; guess I hadn't updated my sq install either. (But I have now, and it has the new names.)

Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

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

Thanks! Changes look good; nits below.

The Sequoia CLI's changelog doesn't seem to mention these changes:
  https://gitlab.com/sequoia-pgp/sequoia-sq/-/blob/b4158f40dea64655cf7357d3c8f220e8bd1708d9/NEWS

The `--recipient-file` rename, anyway, looks like it happened here:
  https://gitlab.com/sequoia-pgp/sequoia/-/commit/db90178d5b61e3310f979fdaefc66610adf0d33e
  https://gitlab.com/sequoia-pgp/sequoia/-/issues/933

Shrug; these changes are easy enough to adapt to.

```
$ sq version
sq 0.38.0
using sequoia-openpgp 1.21.2
with cryptographic backend Nettle 3.10 (Cv448: true, OCB: true)
```
…egen

We recently started building RN from Git, in 0509575. This caused
an iOS build failure in the shell script '[CP-User] Generate Specs':

  Error: Cannot find module '/Users/chrisbobbe/dev/zulip-mobile/node_modules/react-native/packages/react-native-codegen/lib/cli/combine/combine-js-to-schema-cli.js'

Note the last part:
  [...]/lib/cli/combine/combine-js-to-schema-cli.js'

There is a [...]/src/[...] version of that file, but the /lib/ one
is created by `yarn build`, and when I ran that, the app build
succeeded.
@chrisbobbe
Copy link
Contributor Author

Thanks! Revision pushed.

@gnprice gnprice merged commit 1db6467 into zulip:main Oct 1, 2024
1 check passed
@gnprice
Copy link
Member

gnprice commented Oct 1, 2024

Thanks! Looks good; merging.

@chrisbobbe chrisbobbe deleted the pr-build-fixes branch October 5, 2024 00:54
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