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.

Sign up for GitHub

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

Jump to bottom

reveal-md: init at 5.2.0 #144918

Merged
merged 1 commit into from
Nov 15, 2021
Merged

reveal-md: init at 5.2.0 #144918

merged 1 commit into from
Nov 15, 2021

Conversation

sagikazarmark
Copy link
Member

@sagikazarmark sagikazarmark commented Nov 6, 2021 •
edited
Loading

Signed-off-by: Mark Sagi-Kazar [email protected]

Motivation for this change

Build reveal.js slides from markdown.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 21.11 Release Notes (or backporting 21.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.

@ofborg ofborg bot requested a review from midchildan November 6, 2021 19:25
@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label Nov 10, 2021
jonringer
jonringer suggested changes Nov 10, 2021
View reviewed changes
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

Getting a build failure:

ERROR: Failed to download Chromium r674921! Set "PUPPETEER_SKIP_CHROMIUM_DOWNLOAD" env variable to skip download.
Error: getaddrinfo ENOTFOUND storage.googleapis.com
    at GetAddrInfoReqWrap.onlookup [as oncomplete] (dns.js:71:26)
  -- ASYNC --
    at BrowserFetcher.<anonymous> (/nix/store/dqxfrsbgq0v7wscry5s4pl1cd6wdbkx1-reveal-md-5.2.0/lib/node_modules/reveal-md/n>
    at Object.<anonymous> (/nix/store/dqxfrsbgq0v7wscry5s4pl1cd6wdbkx1-reveal-md-5.2.0/lib/node_modules/reveal-md/node_modu>
    at Module._compile (internal/modules/cjs/loader.js:1085:14)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1114:10)
    at Module.load (internal/modules/cjs/loader.js:950:32)
    at Function.Module._load (internal/modules/cjs/loader.js:790:12)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:76:12)
    at internal/main/run_main_module.js:17:47 {
  errno: -3008,
  code: 'ENOTFOUND',
  syscall: 'getaddrinfo',
  hostname: 'storage.googleapis.com'
}
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! [email protected] install: `node install.js`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the [email protected] install script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

you will probably need to do something to:

  reveal-md = super.reveal-md.override({
    prePatch = ''
      export PUPPETEER_SKIP_CHROMIUM_DOWNLOAD=1
    '';
  });

in pkgs/development/node-packages/default.nix

@sagikazarmark
Copy link
Member Author

@jonringer interesting, thanks for pointing that out.

I looked at other examples and went with an existing solution that the mermaid-js package uses as well.
Chromium is not supported on Darwin, so I disabled downloading on every other platform.

Let me know if it's not the right solution.

Thanks again!

@sagikazarmark sagikazarmark mentioned this pull request Nov 13, 2021
Closed
SuperSandro2000
SuperSandro2000 reviewed Nov 14, 2021
View reviewed changes
pkgs/development/node-packages/default.nix
@@ -340,6 +340,19 @@ let
'';
};

reveal-md = super.reveal-md.override (
lib.optionalAttrs (!stdenv.isDarwin) {
Copy link
Member

Choose a reason for hiding this comment

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

What happens if I try to build the package on Darwin?

Copy link
Member Author

Choose a reason for hiding this comment

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

It builds successfully for me. I found this pattern for other node apps based on puppeteer in default.nix

I don't know why it doesn't work on linux, but on darwin we can't use the chromium package.

Copy link
Member Author

Choose a reason for hiding this comment

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

See mermaid-cli

@jonringer
Copy link
Contributor

and the node conflicts begin :(

CONFLICT (content): Merge conflict in pkgs/development/node-packages/node-packages.nix
Auto-merging pkgs/development/node-packages/node-packages.json
Automatic merge failed; fix conflicts and then commit the result.
https://github.com/NixOS/nixpkgs/pull/144918 failed to build

@sagikazarmark
Copy link
Member Author

I'm already rebasing it for like the 10th time now to resolve conflicts, but running the generate script takes like 2 hours every time. :)

@SuperSandro2000
Copy link
Member

I'm already rebasing it for like the 10th time now to resolve conflicts, but running the generate script takes like 2 hours every time. :)

We can also be sneaky and merge it without updating that file and let someone else do it.

@sagikazarmark
reveal-md: init at 5.2.0
5bb83d0 
Signed-off-by: Mark Sagi-Kazar <[email protected]>
@sagikazarmark
Copy link
Member Author

@jonringer @SuperSandro2000 Rebase finally completed.

@jonringer
Copy link
Contributor

other updates to node packages keep causing regressions....

@jonringer
Copy link
Contributor

unrelated regression fix: #146166

jonringer
jonringer approved these changes Nov 15, 2021
View reviewed changes
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

LGTM

failures are broken on target branch

https://github.com/NixOS/nixpkgs/pull/144918

1 package failed to build:
teck-programmer

4 packages built:
gtop lumo pm2 reveal-md

@jonringer jonringer merged commit e7b0920 into NixOS:master Nov 15, 2021
@sagikazarmark sagikazarmark deleted the add-reveal-md branch November 15, 2021 18:05
@sagikazarmark
Copy link
Member Author

Thanks a lot for your help to get this merged!

@jonringer
Copy link
Contributor

yea. node package within nix is not the greatest maintainer experience

@dotlambda dotlambda mentioned this pull request Jul 31, 2023
Merged
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SuperSandro2000 SuperSandro2000 SuperSandro2000 left review comments

@jonringer jonringer jonringer approved these changes

@midchildan midchildan Awaiting requested review from midchildan

Assignees
No one assigned
Labels
8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sagikazarmark @jonringer @SuperSandro2000