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

fira-code-symbols: 20160811 -> 20191205 #75034

Closed
wants to merge 3 commits into from

Conversation

@Vonfry
Copy link
Contributor

@Vonfry Vonfry commented Dec 5, 2019

Motivation for this change

The old version of fira-code-symbols is made from an old version fira-code. I use a script from the fira-code repo's issue to update it to current version of fira-code.

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 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 @Profpatsch

@Vonfry Vonfry force-pushed the Vonfry:fix/fira-code-symbols-update branch from c1702f2 to e4b0f25 Dec 5, 2019
Generate font from current fira-code font instead of downloading the old one.
@Vonfry Vonfry force-pushed the Vonfry:fix/fira-code-symbols-update branch from e4b0f25 to bffeb3c Dec 5, 2019
@ofborg ofborg bot requested a review from Profpatsch Dec 5, 2019
clean generate script, remove unused variable.
Copy link
Contributor

@drewrisinger drewrisinger left a comment

Thanks for making your first contribution! This looks like a great first contribution. Most of the notes here are style/documentation-related.

A few other side notes:

  • Make sure your commits & contributions fit CONTRIBUTING.md (specifically, squash the fira-code-symbols commits together)
  • I'm not 100% sure the python script should be linked as source.
  • Python environment looks a little funky, see comments.
'';
license = licenses.ofl;
maintainers = [ maintainers.Profpatsch ];
maintainers = [ maintainers.Profpatsch maintainers.vonfry ];

This comment has been minimized.

@drewrisinger

drewrisinger May 5, 2020
Contributor

Suggested change
maintainers = [ maintainers.Profpatsch maintainers.vonfry ];
maintainers = with maintainers; [ Profpatsch vonfry ];
Use https://gist.github.com/xieve/d5a01cc59896c3973cb16df9ba8d30d4 script
to patch fira code with current version.
Comment on lines +29 to +30

This comment has been minimized.

@drewrisinger

drewrisinger May 5, 2020
Contributor

I'm not sure this needs to be in the description. I would rather have this information up in the source near where the script is actually called.

mkdir -p $out/share/fonts
unzip -j $downloadedFile -d $out/share/fonts/opentype
'';
phases = [ "buildPhase" "installPhase" ];

This comment has been minimized.

@drewrisinger

drewrisinger May 5, 2020
Contributor

It's atypical to override the phases, typically it's fine to let the phases do nothing. Is setting phases necessary?

in stdenv.mkDerivation {
name = "fira-code-symbols-20191205";
# https://gist.github.com/xieve/d5a01cc59896c3973cb16df9ba8d30d4
src = ./fira_code_patch.py;

This comment has been minimized.

@drewrisinger

drewrisinger May 5, 2020
Contributor

Do we need to have this script in-tree? Would it be better to grab it using something like fetchurl?

This comment has been minimized.

@Mic92

Mic92 May 5, 2020
Contributor

Agreed. fetchurl using the gist commit is better.

let pythonEnv = python3.withPackages (p: [ p.fontforge ]);
in stdenv.mkDerivation {
name = "fira-code-symbols-20191205";
# https://gist.github.com/xieve/d5a01cc59896c3973cb16df9ba8d30d4

This comment has been minimized.

@drewrisinger

drewrisinger May 5, 2020
Contributor

Can you add a few words of detail to this gist link? What the script does, that the script is from this link, etc.


fetchzip {
name = "fira-code-symbols-20160811";
let pythonEnv = python3.withPackages (p: [ p.fontforge ]);

This comment has been minimized.

@drewrisinger

drewrisinger May 5, 2020
Contributor

I honestly don't know whether this is best practice. This feels a little wrong to me to build a python environment for building an in-tree dependency...

Maybe something like this would work:

  • add pythonFontForge as a dependency (set equal to python3Packages.fontforge in calling package)
  • Add pythonFontForge to nativeBuildInputs
  • check that the fontforge package is available in buildphase (something like python -m fontforge)

Just guessing here.


sha256 = "19krsp22rin74ix0i19v4bh1c965g18xkmz1n55h6n6qimisnbkm";
buildPhase = ''
python3 $src -o . ${fira-code}/share/fonts/opentype/*.otf

This comment has been minimized.

@drewrisinger

drewrisinger May 5, 2020
Contributor

no need to specify python3, I'd just do either python or ${python.interpreter}

@@ -0,0 +1,50 @@
#!/bin/env python3

This comment has been minimized.

@drewrisinger

drewrisinger May 5, 2020
Contributor

#! not necessary b/c it's being run by python interpreter explicitly


import fontforge

def run(fontpath, outpath, starting_point=0xe100):

This comment has been minimized.

@drewrisinger

drewrisinger May 5, 2020
Contributor

Docstrings somewhere in this code would be necessary to tell what it's doing.

os.makedirs(output, exist_ok=True)

for f in chain.from_iterable(map(glob, fonts)):
run(f, os.path.join(output, os.path.basename(f)))
Comment on lines +43 to +46

This comment has been minimized.

@drewrisinger

drewrisinger May 5, 2020
Contributor

I personally prefer using python's pathlib library vs os, but if this script works, don't fix it!

@Vonfry
Copy link
Contributor Author

@Vonfry Vonfry commented Jun 7, 2020

I find this pr will break someone using the original font.

Close this.

@Vonfry Vonfry closed this Jun 7, 2020
@Vonfry Vonfry deleted the Vonfry:fix/fira-code-symbols-update branch Oct 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.