Skip to content

Make fontkit cross-platform compatible again#119

Merged
devongovett merged 1 commit into
foliojs:masterfrom
Pomax:cross-platform
Jul 29, 2018
Merged

Make fontkit cross-platform compatible again#119
devongovett merged 1 commit into
foliojs:masterfrom
Pomax:cross-platform

Conversation

@Pomax

@Pomax Pomax commented Jun 27, 2017

Copy link
Copy Markdown
Contributor

Closes #118

This makes everything node-runnable again, obviating the need for OS-presupplied utilities. It adds npm-run-all as a script running convenience, and it adds shx as cross-platform utility runner for common unix utils such as cp and rm. Finally, it also adds cross-env to ensure that environment variables are set in a way that work in unix, linux, OSX, and Windows alike.

npm test shows all tests passing, with the same coverage.

@Pomax

Pomax commented Jun 27, 2017

Copy link
Copy Markdown
Contributor Author

@devongovett probably worth landing, to unblock Windows users (either by choice or because work demands it) since CI says it's all good =)

@devongovett

Copy link
Copy Markdown
Member

Not sure how I feel about this. Doesn't Windows have bash now? What about cygwin?

Admittedly my knowledge of windows is very limited, but Make is a tool that's designed exactly for this purpose. npm scripts are not.

@Pomax

Pomax commented Jul 18, 2017

Copy link
Copy Markdown
Contributor Author

This is exactly what npm scripts were designed for, too, and have the benefit of actually working on Windows, whereas make simply doesn't (unless you make people jump through painful hoops).

That said, the Linux Subsystem is still in beta, so you can't really rely on it. Cygwin is massively huge, and for node projects there isn't a single thing in Cygwin that isn't already covered by node/npm packages that have clean CLI invocations. There is literally no reason to have any kind of reliance on make and Makefile if you're writing a node project =)

This PR simply ports the instructions you had in the Makefile to a form that fits "the proper node way" of bootstrapping and using CLI-based task running without baking in anything that's platform specific and prevents the project from running on one or more operating systems.

@davelab6

davelab6 commented Jul 18, 2017 via email

Copy link
Copy Markdown

@Pomax

Pomax commented Sep 25, 2017

Copy link
Copy Markdown
Contributor Author

hm. bump?

@Pomax

Pomax commented Jan 18, 2018

Copy link
Copy Markdown
Contributor Author

bump, again? =)

@Pomax

Pomax commented Jun 4, 2018

Copy link
Copy Markdown
Contributor Author

rerebump =)

(feel free to close it, but it would still benefit loads of people to merge this without any change in build funcationality)

@Pomax

Pomax commented Jul 29, 2018

Copy link
Copy Markdown
Contributor Author

@devongovett this is still very much worth landing

If you no longer really work on fontkit, could I selfishly ask you just hit merge so that people who do still use it a lot in the font work can use this normally on windows, without affecting anyone on nx operating systems? It doesn't get in your or anyone else's way, but does make life much easier for an estimated 50% of the developer community.

@devongovett devongovett merged commit bdd50f6 into foliojs:master Jul 29, 2018
@davelab6

davelab6 commented Jul 30, 2018 via email

Copy link
Copy Markdown

@devongovett

Copy link
Copy Markdown
Member

Yeah sorry guys, I've been busy with other projects lately. Today I made an org https://github.com/foliojs and I'll be moving fontkit, textkit, pdfkit, and all related dependencies across the whole stack there so more contributors can be involved with reviewing and merging code. @diegomura of react-pdf is already helping out a bit, and I hope to bring others on as maintainers as well. Hopefully that will help with the long term support for these projects.

@Pomax Pomax deleted the cross-platform branch July 30, 2018 16:19
@Pomax

Pomax commented Jul 30, 2018

Copy link
Copy Markdown
Contributor Author

That's great to hear!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants