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

noto-fonts-emoji-blob-bin: init at 2019-06-14-Emoji-12 #69734

Merged
merged 1 commit into from Jun 18, 2020
Merged

noto-fonts-emoji-blob-bin: init at 2019-06-14-Emoji-12 #69734

merged 1 commit into from Jun 18, 2020

Conversation

rileyinman
Copy link
Contributor

Motivation for this change

I like the blob emoji more than the new Noto style (and I know some other people do too!) and having this fork available seems like a good idea :)

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nix-review --run "nix-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.
Notify maintainers

cc @Mathnerd314

@rileyinman
Copy link
Contributor Author

I just noticed that #65507 also exists, but builds from source in a separate package while this one grabs the .ttf as part of the noto-fonts-emoji package. If that one is the preferred PR, I can close this one.

@dasJ
Copy link
Member

dasJ commented Dec 23, 2019

@rileyinman While I prefer my PR because it uses sources instead of just fetching the binaries, yours is probably smarter. Our private Hydra often times out trying to build Blobmoji. We don't really get anything building Blobmoji, because I don't see why anything would be overriden (which is the only reason for me to have the package built from source).

dasJ
dasJ requested changes Dec 23, 2019
View reviewed changes
pkgs/data/fonts/noto-fonts/default.nix Outdated Show resolved Hide resolved
pkgs/data/fonts/noto-fonts/default.nix Show resolved Hide resolved
@dasJ
Copy link
Member

dasJ commented Feb 1, 2020

ping @rileyinman ?

@rileyinman rileyinman changed the title noto-fonts-emoji-blob: init at 2019-06-14-Emoji-12 noto-fonts-emoji-blob-bin: init at 2019-06-14-Emoji-12 Feb 4, 2020
@rileyinman
Copy link
Contributor Author

rileyinman commented Feb 4, 2020 •
edited

It looks like GitHub's merge conflict resolution didn't quite work. I fixed and force pushed it back up, sorry about that 😅

@rileyinman
Copy link
Contributor Author

How's this @dasJ ?

@rileyinman rileyinman requested a review from dasJ February 8, 2020 04:03
Mathnerd314
Mathnerd314 approved these changes Feb 21, 2020
View reviewed changes
Copy link
Contributor

@Mathnerd314 Mathnerd314 left a comment

Choose a reason for hiding this comment

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

Looks fine, no real issues. I'm not sure about the -bin as most fonts in NixOS aren't built from source, but it's not like there's an official package naming policy guide I could point you to.

pkgs/data/fonts/noto-fonts/default.nix Outdated Show resolved Hide resolved
@rileyinman
Copy link
Contributor Author

@Mathnerd314 Yeah, I'm also not too familiar with it but it seems most of the noto fonts are built from source. It also follows what I know of (for example) iosevka and iosevka-bin.

@rileyinman
Copy link
Contributor Author

Rebased to master to fix merge conflict.

@Mathnerd314
Copy link
Contributor

@yegortimoshenko you merged #79762, can you merge this one too?

@rileyinman
Copy link
Contributor Author

Anyone willing to merge this?

@ajs124
Copy link
Member

ajs124 commented Jun 17, 2020

cc @Lassulus

@Lassulus Lassulus merged commit 9480bae into NixOS:master Jun 18, 2020
@rileyinman
Copy link
Contributor Author

Thank you!

@rileyinman rileyinman deleted the blobmoji branch June 18, 2020 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mathnerd314 Mathnerd314 Mathnerd314 approved these changes

@dasJ dasJ dasJ approved these changes

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

Successfully merging this pull request may close these issues.

None yet
5 participants
@rileyinman @dasJ @Mathnerd314 @ajs124 @Lassulus