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

logseq: fix build, move to by-name #321236

Merged
merged 1 commit into from
Jun 20, 2024

Conversation

superherointj
Copy link
Contributor

@superherointj superherointj commented Jun 20, 2024

logseq: fix build, move to by-name

CC @Aman9das @bclaud

Proper fix depends on upstream:

@superherointj superherointj requested a review from wegank June 20, 2024 11:30
@superherointj superherointj requested a review from onny June 20, 2024 11:35
@superherointj superherointj force-pushed the logseq-fix-build branch 2 times, most recently from d58fb3b to 1ea284c Compare June 20, 2024 11:46
@bclaud
Copy link

bclaud commented Jun 20, 2024

Logseq is building and working. But electron 28 reached EOL.

For more information on this issue: logseq/logseq#11378

Thanks @superherointj 🥇

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Jun 20, 2024
@superherointj
Copy link
Contributor Author

superherointj commented Jun 20, 2024

But electron 28 reached EOL.

I had unpinned electron (30). But then, wegank told me to pin it to upstream. Then, it is EOL.
This has to be fixed at upstream.

Still the process here is correctly defined now.
So it is worth merging as is. (And make a new PR when upstream decides to bump electron.)

@wegank
Copy link
Member

wegank commented Jun 20, 2024

Oh, that's too bad, we're not really fixing the build as both Electrons are EOL...

Electron 30 does seem preferable in this case, I think.

@superherointj
Copy link
Contributor Author

Oh, that's too bad, we're not really fixing the build as both Electrons are EOL...
Electron 30 does seem preferable in this case, I think.

But then, we're not fixing the process. And there's the issue of application not working properly still.
The fix has to come from upstream. We don't have a good fix here.
I'd keep format right and seek a fix at upstream.
Also, whoever wants to still use this package can add the security exception.

I'd favor keeping package right.

@wegank What do we do?

@superherointj superherointj merged commit 0abe6ca into NixOS:master Jun 20, 2024
27 of 29 checks passed
@superherointj superherointj deleted the logseq-fix-build branch June 20, 2024 13:53
@RobFisher
Copy link

RobFisher commented Jun 24, 2024

I appreciate the work you're doing, @superherointj . Could this explain also logseq/logseq#10851 :

Error: The module '/nix/store/qiv0cgr5l5cl5j8x0wxl267519b2zw3v-logseq-0.10.9/share/logseq/resources/app/node_modules/better-sqlite3/build/Release/better_sqlite3.node'
was compiled against a different Node.js version using
NODE_MODULE_VERSION 118. This version of Node.js requires
NODE_MODULE_VERSION 119. Please try re-compiling or re-installing
the module (for instance, using `npm rebuild` or `npm install`).

?

btw, in case anyone is wondering, to allow Electron 28:

  nixpkgs.config.permittedInsecurePackages = [
    "electron-28.3.3"
  ];

@superherointj
Copy link
Contributor Author

superherointj commented Jun 24, 2024

Could this explain also logseq/logseq#10851 :

Error: The module '/nix/store/qiv0cgr5l5cl5j8x0wxl267519b2zw3v-logseq-0.10.9/share/logseq/resources/app/node_modules/better-sqlite3/build/Release/better_sqlite3.node'
was compiled against a different Node.js version using
NODE_MODULE_VERSION 118. This version of Node.js requires
NODE_MODULE_VERSION 119.

I don't use logseq. So I haven't encountered this problem myself. But you can test to pin nodejs to 18:

At nixpkgs, change:

image

Then test it. If it works, feel free to propose a PR pinning nodejs to 18. And explain the reasoning at nodejs parameter at logseq package file.

Update: This answer is invalid.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants