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

excalidraw_export: init at v1.1.0 #341078

Merged
merged 3 commits into from
Nov 20, 2024
Merged

Conversation

venikx
Copy link
Contributor

@venikx venikx commented Sep 10, 2024

Description of changes

The following npm package makes it possible to convert .excalidraw files to .svg files: https://github.com/Timmmm/excalidraw_export and is used by https://github.com/wdavew/org-excalidraw to handle excalidraw files inside org-mode in Emacs.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.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.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added 6.topic: nodejs 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` labels Sep 10, 2024
@NixOSInfra NixOSInfra added the 12. first-time contribution This PR is the author's first one; please be gentle! label Sep 10, 2024
@venikx venikx force-pushed the add-excalidraw_export branch from 1d7cc85 to 74fcdc9 Compare September 10, 2024 19:41
@venikx venikx changed the title feat(nodePackages): add excalidraw_export package nodePackages.excalidraw_export: init at v1.1.0 Sep 10, 2024
@venikx
Copy link
Contributor Author

venikx commented Sep 10, 2024

I wasn't sure which way I should provide this package buildNpmPackage or via the node2nix helper. I saw from the commits that some packages are being dropped, and newer ones are added via buildNpmPackage (for example: #309100). Is there a preference to using buildNpmPackage? If yes, I have no issue converting this.

I also noticed that the script I had to run updated all the packages, as they were outdated. Should I perhaps separate these two concerns in different PR's (updating the existing packages (1), then adding the new package (2))?

@tomodachi94
Copy link
Member

tomodachi94 commented Sep 11, 2024

Is there a preference to using buildNpmPackage? If yes, I have no issue converting this.

Yes, there seems to be consensus among Node-involved Nixpkgs maintainers that we should avoid adding new packages to nodePackages. (c.f. #229475)

@venikx
Copy link
Contributor Author

venikx commented Sep 11, 2024

Is there a preference to using buildNpmPackage? If yes, I have no issue converting this.

Yes, there seems to be consensus among Node-involved Nixpkgs maintainers that we should avoid adding new packages to nodePackages. (c.f. #229475)

Updated to use buildNpmPackage instead of the node2nix stuff.

@venikx venikx changed the title nodePackages.excalidraw_export: init at v1.1.0 excalidraw_export: init at v1.1.0 Sep 11, 2024
@venikx venikx force-pushed the add-excalidraw_export branch from 4b80ab5 to d75b5c1 Compare September 13, 2024 13:39
@venikx venikx force-pushed the add-excalidraw_export branch from d75b5c1 to 0248cb7 Compare September 17, 2024 15:11
@venikx venikx requested a review from tomodachi94 September 19, 2024 14:04
Copy link
Member

@tomodachi94 tomodachi94 left a comment

Choose a reason for hiding this comment

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

Lgtm, thank you ❤️

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Sep 19, 2024
@venikx venikx force-pushed the add-excalidraw_export branch from 0248cb7 to 37ffee8 Compare September 27, 2024 19:48
@venikx venikx requested a review from tomodachi94 September 27, 2024 19:49
@venikx
Copy link
Contributor Author

venikx commented Sep 27, 2024

I was able to also verify the build on my desktop linux, so x86_64-linux and aarch64-linux should both work. Flagged anything darwin related as broken, due to earlier automated builds failing.

@wegank wegank removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Sep 29, 2024
Copy link
Member

@tomodachi94 tomodachi94 left a comment

Choose a reason for hiding this comment

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

Lgtm, thank you for your patience! 💖

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Oct 7, 2024
@venikx
Copy link
Contributor Author

venikx commented Nov 20, 2024

@tomodachi94 I'm a bit unfamiliar with the process since it's my first time contributing. Is there something still that is blocking the merge that I can help resolve?

@tomodachi94
Copy link
Member

Nope, just my forgetfulness :) Merging now, thank you ❤️

@tomodachi94 tomodachi94 merged commit 03597e2 into NixOS:master Nov 20, 2024
31 checks passed
@venikx venikx deleted the add-excalidraw_export branch November 21, 2024 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer This PR was created by the maintainer of the package it changes 12.approvals: 1 This PR was reviewed and approved by one reputable person 12. first-time contribution This PR is the author's first one; please be gentle!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants