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

fontforge-gtk: Add support for libspiro. #22521

Merged
merged 3 commits into from Apr 3, 2017
Merged

fontforge-gtk: Add support for libspiro. #22521

merged 3 commits into from Apr 3, 2017

Conversation

lprndn
Copy link
Contributor

@lprndn lprndn commented Feb 7, 2017 •
edited

Motivation for this change

fontforge-gtk is mainly for font editing. Adding Spiro tools let us have a complete toolkit.

Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@lprndn lprndn changed the title libspiro: Init at c26afeb fontforge-gtk: Add support for libspiro. Feb 7, 2017
joachifm
joachifm reviewed Feb 9, 2017
View reviewed changes
pkgs/development/libraries/libspiro/default.nix Outdated
stdenv.mkDerivation rec {
name = "libspiro";
version = "20170128";
src = fetchgit {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this rely on submodules? If not, source archives are strongly preferred. You can use fetchFromGitHub for convenience.

@lprndn
Copy link
Contributor Author

lprndn commented Feb 10, 2017

Switched to stable release. Therefore used fetchurl to fetch sources.

joachifm
joachifm reviewed Feb 10, 2017
View reviewed changes
pkgs/development/libraries/libspiro/default.nix Outdated

nativeBuildInputs = [pkgconfig];

meta = with stdenv.libs; {
Copy link
Contributor

Choose a reason for hiding this comment

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

stdenv.libs should be stdenv.lib.

@joachifm
Copy link
Contributor

The travis build error looks legit

@lprndn
Copy link
Contributor Author

lprndn commented Feb 12, 2017

@joachifm pdf2htmlEX, which use fontforge-gtk as a dependency, doesn't seem to build with the addition of libspiro. I'm just asking myself if it's really necessary to use fontforge-gtk instead of fontforge there. To my knowledge, it's a CLI tool and, after testing, it actually builds without problems with standard fontforge. What do you think?

@joachifm
Copy link
Contributor

cc @taktoa may know more about pdf2htmlEx.

@taktoa
Copy link
Member

taktoa commented Feb 12, 2017

pdf2htmlEX should be fine with fontforge; I think I had suspected that some features would be enabled by switching to -gtk and then forgot to switch it back (I packaged it a while ago, so I don't remember my exact thought process).

@joachifm
Copy link
Contributor

@lprndn this needs a rebase

@lprndn
fontforge-gtk: Add support for libspiro.
4cd7b28 
Add Spiro toolkit in fontforge-gtk for designing fonts.
libspiro: Init at c26afeb.
@lprndn
pdf2htmlEX: Switch from fontforge-gtk to fontforge.
8a534d5
@lprndn
Copy link
Contributor Author

lprndn commented Feb 17, 2017

Rebased. Kept 2 commits, I think it might be nice to leave pdef2htmlEX modifications on their own.

@lprndn
Copy link
Contributor Author

lprndn commented Apr 3, 2017

Since 17.03 release rush is over, is there any blocking issue to merge this commit?

@vcunat
Copy link
Member

vcunat commented Apr 3, 2017

I see a merge conflict, though it might be something trivial.

@lprndn
Copy link
Contributor Author

lprndn commented Apr 3, 2017

Merge conflict comes from pdf2htmlEX modifications discussed before. The maintainer of the package seems to agree for the changes.

@taktoa
Copy link
Member

taktoa commented Apr 3, 2017

Does this mean I need to make a separate pull request for the pdf2htmlEX changes or will this PR include those changes? In other words, do you need me to take any action?

@lprndn
Copy link
Contributor Author

lprndn commented Apr 3, 2017 •
edited

I made the changes myself in this commit so I don't think you need to worry. Conflict is simple so it can be resolved directly in github.

-Edit: Crazy! I didn't know I could solve the conflict myself... Should be ok now.

@vcunat vcunat merged commit f259fa6 into NixOS:master Apr 3, 2017
vcunat added a commit that referenced this pull request Apr 3, 2017
@vcunat
Merge #22521: fontforge-gtk: Add support for libspiro
8165979
@vcunat
Copy link
Member

vcunat commented Apr 3, 2017

Yes, it's also possible to do it locally, e.g. the same way (merge master into your branch).

@lprndn lprndn deleted the lprndn_Nixpkgs branch April 3, 2017 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joachifm joachifm joachifm left review comments

Assignees
No one assigned
Labels
0.kind: enhancement 8.has: package (new) 12. first-time contribution
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@lprndn @joachifm @taktoa @vcunat @Janik-Haag