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

joplin-cli 2.2.2 (new formula) #83099

Closed
wants to merge 1 commit into from
Closed

Conversation

chmac
Copy link
Contributor

@chmac chmac commented Aug 11, 2021

  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?

I tried to create a test, however, joplin relies on the keychain service. So any command (joplin help, joplin version) will always throw a popup telling me that my credentials don't exist. I'm not sure how to workaround this problem as I understood the tests must be non interactive. Any suggestions here would be appreciated. Instead I added a stupid assert_match "joplin", "joplin" test for now. My previous tries were:

assert_match "joplin ", shell_output("#{bin}/joplin version")
  • Does your build pass brew audit --strict <formula> (after doing brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

I get the following error, to do with npm installing a universal binary for fsevents:

joplin-terminal:
  * Unexpected universal binaries were found.
    The offending files are:
      /usr/local/Cellar/joplin-terminal/2.2.1/libexec/lib/node_modules/joplin/node_modules/fsevents/fsevents.node
Error: 1 problem in 1 formula detected

I'm not sure how to fix this. Any input / feedback would be appreciated. I've created this PR as a draft as I don't expect it to be merged with these two issues, but I'm unsure how to resolve them.

I've added this package to the audit_exceptions/universal_binary_allowlist.json file. I'm assuming that this is an acceptable strategy, but I'll defer judgement on that to somebody who knows brew's policies.

@BrewTestBot BrewTestBot added new formula PR adds a new formula to Homebrew/homebrew-core nodejs Node or npm use is a significant feature of the PR or issue labels Aug 11, 2021
@carlocab carlocab added CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. CI-force-linux [DEPRECATED] Don't pass --skip-unbottled-linux to brew test-bot. labels Aug 11, 2021
@carlocab
Copy link
Member

carlocab commented Aug 11, 2021

It seems that this is a GUI application. Wouldn't this be better suited as a cask?

Edit: Oh, I see. This used to be a formula which was removed in #59053.

@chenrui333 chenrui333 changed the title joplin-terminal 2.2.1 joplin-terminal 2.2.1 (new formula) Aug 14, 2021
@laurent22
Copy link
Contributor

For information, Joplin 2.3 which was just released has a fixed joplin version command that doesn't use the keychain.

@laurent22
Copy link
Contributor

joplin-terminal:
  * Unexpected universal binaries were found.
    The offending files are:
      /usr/local/Cellar/joplin-terminal/2.2.1/libexec/lib/node_modules/joplin/node_modules/fsevents/fsevents.node
Error: 1 problem in 1 formula detected

Is that a check that Homebrew is doing? Because based on the error message it seems to mean that there's an unnecessary file in that directory, but that could just be ignored, couldn't it?

@chmac
Copy link
Contributor Author

chmac commented Aug 16, 2021

For information, Joplin 2.3 which was just released has a fixed joplin version command that doesn't use the keychain.

@laurent22 The Joplin desktop app is included in the cask repo rather than this one. Brew separates packages by terminal (here) and GUI (called casks).

I'm as we speak updating this PR to version 2.2.2 of the terminal app!

@chmac
Copy link
Contributor Author

chmac commented Aug 16, 2021

joplin-terminal:
  * Unexpected universal binaries were found.
    The offending files are:
      /usr/local/Cellar/joplin-terminal/2.2.1/libexec/lib/node_modules/joplin/node_modules/fsevents/fsevents.node
Error: 1 problem in 1 formula detected

Is that a check that Homebrew is doing? Because based on the error message it seems to mean that there's an unnecessary file in that directory, but that could just be ignored, couldn't it?

I think the issue is that brew targets multiple architectures and multiple operating systems. I suspect that one of the joplin dependencies is using a file watcher which in turn relies on some native code. That native code needs to somehow be included for each os / architecture separately I guess. But I'm unclear on how to do that in brew. I'll research further. Hopefully somebody more familiar with brew (ping @carlocab) might share some suggestions on this...

@chmac
Copy link
Contributor Author

chmac commented Aug 16, 2021

$ yarn why fsevents
yarn why v1.22.10
[1/4] �  Why do we have the module "fsevents"...?
[2/4] �  Initialising dependency graph...
[3/4] �  Finding dependency...
[4/4] �  Calculating file sizes...
=> Found "fsevents@2.3.2"
info Reasons this module exists
   - "joplin#@joplin#lib#chokidar" depends on it
   - Hoisted from "joplin#@joplin#lib#chokidar#fsevents"
info Disk size without dependencies: "164KB"
info Disk size with unique dependencies: "164KB"
info Disk size with transitive dependencies: "164KB"
info Number of shared dependencies: 0

As I suspected, it seems the chokidar package depends on fsevents, which seems to only apply to macOS. Can anybody point me to another package which has the same issue that I could use as a model?

@laurent22
Copy link
Contributor

webpack is one of them but it doesn't look like they are doing anything special about fsevent: #81747 Perhaps that error, if that's what it is, can be ignored?

@laurent22
Copy link
Contributor

Actually it looks like the package needs to be added to the allowlist: https://github.com/Homebrew/homebrew-core/pull/81747/files#diff-8ea64149238b88cfd570111cdfd02ca2325ebe7cd1930fc6937858a33dd61a50

@chmac
Copy link
Contributor Author

chmac commented Aug 16, 2021

@laurent22 Awesome, nice catch. I'll try adding joplin-terminal to that exceptions list. I suspect that in the case of fsevents, the universal binary is acceptable because the package seems to be only used on macOS. I'll leave it up to somebody who knows brew to decide if this is an acceptable fix.

@BrewTestBot BrewTestBot added the automerge-skip `brew pr-automerge` will skip this pull request label Aug 16, 2021
@chmac chmac changed the title joplin-terminal 2.2.1 (new formula) joplin-terminal 2.2.2 (new formula) Aug 16, 2021
@chmac
Copy link
Contributor Author

chmac commented Aug 16, 2021

Two of the installs failed during automated testing. The tests in question were 11-arm64 and ubuntu-latest. The reasons for the failures differ.

11-arm64

npm ERR! code 1
npm ERR! path /opt/homebrew/Cellar/joplin-terminal/2.2.2/libexec/lib/node_modules/joplin/node_modules/sharp
npm ERR! command failed
npm ERR! command sh -c (node install/libvips && node install/dll-copy && prebuild-install) || (node-gyp rebuild && node install/dll-copy)

I suspect that 11-arm64 is macOS 11 on apple's M1 silicon. My suspicion is that sharp@0.26.3 may not be compatible with macOS M1 silicon. But that's just a guess.

$ yarn list --pattern sharp
yarn list v1.22.10
└─ sharp@0.26.3

Edit: Adding more context to this error:

npm ERR! warning: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/libtool: archive library: Release/nothing.a the table of contents is empty (no object file members in the library define global symbols)
npm ERR! ../src/operations.cc:21:10: fatal error: 'vips/vips8' file not found
npm ERR! #include <vips/vips8>
npm ERR!          ^~~~~~~~~~~~
npm ERR! ../src/pipeline.cc:27:10: fatal error: 'vips/vips8' file not found
npm ERR! #include <vips/vips8>
npm ERR!          ^~~~~~~~~~~~
npm ERR! 1 error generated.
npm ERR! 1 error generated.

...

npm ERR! gyp ERR! build error
npm ERR! gyp ERR! stack Error: `make` failed with exit code: 2
npm ERR! gyp ERR! stack     at ChildProcess.onExit (/opt/homebrew/Cellar/joplin-cli/2.2.2/libexec/lib/node_modules/joplin/node_modules/node-gyp/lib/build.js:262:23)
npm ERR! gyp ERR! stack     at ChildProcess.emit (node:events:394:28)
npm ERR! gyp ERR! stack     at Process.ChildProcess._handle.onexit (node:internal/child_process:290:12)
npm ERR! gyp ERR! System Darwin 20.5.0
npm ERR! gyp ERR! command "/opt/homebrew/Cellar/node/16.6.2/bin/node" "/opt/homebrew/Cellar/joplin-cli/2.2.2/libexec/lib/node_modules/joplin/node_modules/.bin/node-gyp" "rebuild"
npm ERR! gyp ERR! cwd /opt/homebrew/Cellar/joplin-cli/2.2.2/libexec/lib/node_modules/joplin/node_modules/sharp
npm ERR! gyp ERR! node -v v16.6.2
npm ERR! gyp ERR! node-gyp -v v3.8.0
npm ERR! gyp ERR! not ok

ubuntu-latest

npm ERR! code 1
npm ERR! path /home/linuxbrew/.linuxbrew/Cellar/joplin-terminal/2.2.2/libexec/lib/node_modules/joplin/node_modules/keytar
npm ERR! command failed
npm ERR! command sh -c prebuild-install || npm run build

My quick and dirty test of yarn install joplin on Ubuntu 20.04.2 worked fine. No issues building keytar. I'm not sure what's going on here, I assume that joplin does successfully install and run on a typical Ubuntu system.

$ yarn list --pattern keytar
yarn list v1.22.10
└─ keytar@7.7.0

@laurent22 Any idea what might be causing these test failures? It seems they're not related to joplin directly but to dependencies.

Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

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

Apologies for the delayed reply -- been busy. Just a few more comments.

When you push further changes, please squash all of your commits into the first one. Thanks!

Formula/joplin-terminal.rb Outdated Show resolved Hide resolved
audit_exceptions/universal_binary_allowlist.json Outdated Show resolved Hide resolved
Formula/joplin-terminal.rb Outdated Show resolved Hide resolved
sha256 "15b932b51632dbcf8dfa0c39541af49b7e072b1a7c204e304d0fbdea6f1d787a"
license "MIT"

depends_on "node"
Copy link
Member

Choose a reason for hiding this comment

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

Can we use Homebrew vips as a dependency here? It seems to still be needed, and the formula previously had a vips dependency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@carlocab I've added vips as a dependency as requested. However, I can install this on my macOS (intel chip) machine without vips being a dependency (and vips is not installed by homebrew on my system). Also, I believe the tests on mac intel hardware all pass.

It seems that adding this dependency changes the errors that block both the 11-arm64 and ubuntu-latest tests.

11-arm64

==> Installing 'bundler' gem
joplin-cli:
  * Binaries built for a non-native architecture were installed into joplin-cli's prefix.
    The offending files are:
      /opt/homebrew/Cellar/joplin-cli/2.2.2/libexec/lib/node_modules/joplin/node_modules/node-notifier/vendor/mac.noindex/terminal-notifier.app/Contents/MacOS/terminal-notifier	(x86_64)
Error: 1 problem in 1 formula detected

This seems to be because node-notifier doesn't work on apple's m1 chips:

mikaelbr/node-notifier#361

I'd be very interested to hear from anyone who has an m1 chip if yarn add joplin or npm install joplin complete successfully. My suspicion is that joplin's terminal app simply doesn't work on m1 silicon due to a variety of issues with dependencies.

ubuntu-latest

Error: The following formulae cannot be installed from bottles and must be
built from source.
  qt, poppler and vips
Already downloaded: /home/linuxbrew/.cache/Homebrew/downloads/da59d3b87c9ca458f5999e38c39a44b7226af89ff8f6634062a0c37eebe706a1--joplin-2.2.2.tgz
==> Verifying checksum for 'da59d3b87c9ca458f5999e38c39a44b7226af89ff8f6634062a0c37eebe706a1--joplin-2.2.2.tgz'

==> brew audit joplin-cli --online --new-formula
Error: install failed
==> FAILED joplin-cli

I suspect that this issue is caused by the addition of vips as a dependency, which I don't think is actually required. I suspect the original keytar issue was probably easier to resolve than this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed vips as a dependency again. I think I captured the incorrect error for the ubuntu-latest test without vips as a dependency.

@chmac chmac changed the title joplin-terminal 2.2.2 (new formula) joplin-cli 2.2.2 (new formula) Aug 16, 2021
@chmac chmac closed this Aug 16, 2021
@chmac chmac reopened this Aug 16, 2021
@chmac chmac force-pushed the joplin-terminal branch 3 times, most recently from f8cc790 to 6c7c923 Compare August 16, 2021 13:53
@chmac
Copy link
Contributor Author

chmac commented Aug 16, 2021

Having a hard time keeping track of all the changes here, so I'll squash the commits together later.

@chmac
Copy link
Contributor Author

chmac commented Aug 16, 2021

OK, I'm lost as to what the issues are here. This formula installs for me on macOS (intel) and appears to pass the tests on macOS intel machines. It doesn't work on arm64 nor ubuntu-latest. I don't know why, and having spent now >8 hours on this, I am out of available time to invest into this problem. Installing joplin from npm works fine on ubuntu and macOS, but I can't confirm about macOS on m1 arm64. Personally, I'll abandon this PR now, but if somebody else wants to push this forward, perhaps they will have more success than me.

@chmac chmac closed this Aug 16, 2021
@carlocab
Copy link
Member

I'll try to help you finish this off.

@carlocab carlocab reopened this Aug 16, 2021
@carlocab carlocab removed the CI-force-linux [DEPRECATED] Don't pass --skip-unbottled-linux to brew test-bot. label Aug 16, 2021
@carlocab
Copy link
Member

Don't delete the branch in your fork in the meantime -- that'll close this PR automatically.

Co-authored-by: Carlo Cabrera <30379873+carlocab@users.noreply.github.com>
@carlocab carlocab marked this pull request as ready for review August 16, 2021 18:13
Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

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

The only failure is due to the sqlite dependency... But the previous formula already had this, so I think this is fine.

Thanks for your work here, @chmac!

@BrewTestBot
Copy link
Member

:shipit: @carlocab has triggered a merge.

@github-actions github-actions bot added the outdated PR was locked due to age label Sep 16, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
automerge-skip `brew pr-automerge` will skip this pull request CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. new formula PR adds a new formula to Homebrew/homebrew-core nodejs Node or npm use is a significant feature of the PR or issue outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants