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

Jump to bottom

inkscape: allow loading external extensions #95114

Merged
merged 2 commits into from Sep 10, 2020
Merged

inkscape: allow loading external extensions #95114

merged 2 commits into from Sep 10, 2020

Conversation

raboof
Copy link
Member

@raboof raboof commented Aug 10, 2020 •
edited

Motivation for this change

My longer-term motivation is that I want to be able to load inkcut as an
extension, but let's start with getting the infrastructure ready and adding
a simple one.

I added the 'hexmap' extension to show the process. You could use this for
example like:

{ pkgs ? import <nixpkgs> {} }:

pkgs.mkShell {
  buildInputs = [
    (pkgs.inkscape-with-extensions.override {
      inkscapeExtensions = [
        pkgs.inkscape-extensions.hexmap
      ];
    })
  ];
}
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

/cc @jtojnar

@raboof
Copy link
Member Author

raboof commented Aug 11, 2020

/marvin opt-in
/state needs_reviewer

@marvin-mk2 marvin-mk2 bot added the marvin label Aug 11, 2020
@marvin-mk2
Copy link

marvin-mk2 bot commented Aug 11, 2020

Hi! I'm an experimental bot. My goal is to guide this PR through its stages, hopefully ending with a merge. You can read up on the usage here.

@raboof
Copy link
Member Author

raboof commented Aug 11, 2020

/status needs_reviewer

@raboof raboof requested a review from jtojnar August 12, 2020 12:01
@raboof raboof requested a review from symphorien August 30, 2020 09:32
@symphorien
Copy link
Member

I could run inkscape with hexmap and it works. The inkscape-with-extensions attrset probably needs recurseIntoAttrs for discoverability.

@raboof
Copy link
Member Author

raboof commented Aug 30, 2020 •
edited

I could run inkscape with hexmap and it works.

👍

The inkscape-with-extensions attrset probably needs recurseIntoAttrs for discoverability.

Nice, I didn't know about that! Tested with nix search -u hexmap and indeed adding recurseIntoAttrs makes it show up. Updated the PR.

@risicle
Copy link
Contributor

risicle commented Sep 8, 2020 •
edited

On macos (tested 10.14), this has a slight quirk...

Screenshot 2020-09-08 at 23 18 39

It's far from a showstopper and I wouldn't know where to start fixing it.

But I like this - think it's nearly merge-o-clock? Hold on, my inkscape-with-extensions doesn't have links to the hexmap files in its extensions directory, on both linux and macos. And sure enough, I can't see an entry in the extensions menu.

Also, could you not have used symlinkJoin from pkgs/build-support/trivial-builders.nix instead of writing your own linking loop?

@raboof
Copy link
Member Author

raboof commented Sep 9, 2020

my inkscape-with-extensions doesn't have links to the hexmap files in its extensions directory, on both linux and macos

Did you add it to the inkscapeExtensions attribute? I followed the convention by vscode to have this list empty by default, you only get the extensions that you add yourself.

could you not have used symlinkJoin from pkgs/build-support/trivial-builders.nix instead of writing your own linking loop?

Possibly! I'll give it a try later.

@raboof raboof force-pushed the inkscape-extensions branch 2 times, most recently from 39e5b12 to 2c870fa Compare September 9, 2020 15:28
@raboof
Copy link
Member Author

raboof commented Sep 9, 2020

Also, could you not have used symlinkJoin from pkgs/build-support/trivial-builders.nix instead of writing your own linking loop?

Yes, very nice simplification - done, thanks!

@risicle
Copy link
Contributor

risicle commented Sep 9, 2020

I think the last version you pushed is a little broken: https://gist.github.com/GrahamcOfBorg/b268ea1ccbe05077fa97f8288a740dd5

@risicle
Copy link
Contributor

risicle commented Sep 9, 2020

Did you add it to the inkscapeExtensions attribute? I followed the convention by vscode to have this list empty by default, you only get the extensions that you add yourself.

Ok - so I guess the intended way to use this would be inkscape-with-extensions.override { inkscapeExtensions = [ inkscape-extensions.hexmap ]; }?

Do you think there might be value in having something like an inkscapeFull with all known extensions to make this available in a more... ergonomic way? i.e. possible to address through a single attribute, not having to break into expression-writing.

(I also came across how terraform handles extensions today, which allows you to do terraform_0_12.withPlugins (p: p.fooplugin), which is pretty neat)

@raboof @stesie
inkscape: introduce a way to add external extensions
67b4880 
Co-Authored-By: Stefan Siegl <stesie@brokenpipe.de>
@raboof
Copy link
Member Author

raboof commented Sep 10, 2020

Did you add it to the inkscapeExtensions attribute? I followed the convention by vscode to have this list empty by default, you only get the extensions that you add yourself.

Ok - so I guess the intended way to use this would be inkscape-with-extensions.override { inkscapeExtensions = [ inkscape-extensions.hexmap ]; }?

Indeed! I was following the lead of e.g. vscode-with-extensions and dde-dock-with-plugins.

Do you think there might be value in having something like an inkscapeFull with all known extensions to make this available in a more... ergonomic way? i.e. possible to address through a single attribute, not having to break into expression-writing.

Sounds good, yeah - though that could get 'heavy' (with some extensions pulling in further dependencies), so I guess it'd be good to have both options?

(I also came across how terraform handles extensions today, which allows you to do terraform_0_12.withPlugins (p: p.fooplugin), which is pretty neat)

I agree that does look quite neat.

risicle
risicle approved these changes Sep 10, 2020
View reviewed changes
Copy link
Contributor

@risicle risicle left a comment

Choose a reason for hiding this comment

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

I can confirm this WFM on non-nixos linux x86_64 & macos 10.14.

@risicle
Copy link
Contributor

risicle commented Sep 10, 2020

Regarding an inkscapeFull, I realize it could get a bit heavy but I think without it or something similar we'd be damning some of our less technical (or simply newer) users from being able to access these at all.

Anyway, I see you're looking to do more work around inkscape extensions in the future, so I'll leave it to you to consider it.

@risicle
Copy link
Contributor

risicle commented Sep 10, 2020

Oh one further thing - would you add yourself as maintainer of the extension?

@raboof
Copy link
Member Author

raboof commented Sep 10, 2020

I realize it could get a bit heavy but I think without it or something similar we'd be damning some of our less technical (or simply newer) users from being able to access these at all.

Yeah, I like the idea, but let's keep it for a later PR

Oh one further thing - would you add yourself as maintainer of the extension?

Sure, done

@risicle risicle merged commit 20e90aa into NixOS:master Sep 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@risicle risicle risicle approved these changes

@symphorien symphorien symphorien approved these changes

@jtojnar jtojnar Awaiting requested review from jtojnar

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

Successfully merging this pull request may close these issues.

None yet
4 participants
@raboof @symphorien @risicle @jtojnar