Skip to content

Refactor build scripting to support Python 3 only builds, pinned build dependency versions, optional system installed build dependencies #416

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

Merged
merged 39 commits into from
Apr 9, 2018

Conversation

chrissimpkins
Copy link
Member

@chrissimpkins chrissimpkins commented Mar 15, 2018

This PR updates the build shell scripts and Makefile to support:

  • Python 3 only builds (pipenv virtualenv based pinned Python version with Pipfile/Pipfile.lock = default & system installed Python 3 = optional)
  • pinned build dependency versions (with default make command)
    • Python build dependencies - pipenv virtualenv based builds by default
    • compiled dependencies - pinned version local installs off of system PATH by default
  • optional rolling system PATH installed Python and compiled build dependencies (with new make targets) - based upon request from @paride for Debian package that is compiled from source

TODO:

  • add support for ttf builds in pipenv isolated Python environment with pinned Python version, pinned Python build dependency versions, and pinned compiled build dependencies by default
  • add support for ttf builds with system installed Python dependencies and system installed compiled build dependencies as optional approach through different make targets
  • modify ttf build test for executable file so that it does not run during system installed dependency builds (Refactor build scripting to support Python 3 only builds, pinned build dependency versions, optional system installed build dependencies #416 (comment))
  • add support for woff local install with new shell script and make target
  • add support for woff compiles from local pinned version install of sfnt2woff-zopfli executable
  • add support for woff compiles from system installed versions of sfnt2woff-zopfli executable
  • add support for woff2 local install with new shell script and make target
  • add support for woff2 compiles from local pinned version install of woff2_compress executable
  • add support for woff2 compiles from system installed versions of woff2_compress executable
  • add support for woff + woff2 subsets with local installs of build dependencies with new shell script and make target
  • update build CI testing with new default build targets
  • add shellcheck shell script linting for new scripts
  • remove from __future__ import statements in Python scripts
  • Python source formatting updates to pass flake8 tests
  • convert to upstream woff2 now that macOS build bug is fixed
  • test conversion of the scripted pipenv build/installs to pipenv install --ignore-pipfile for user development boxes (Refactor build scripting to support Python 3 only builds, pinned build dependency versions, optional system installed build dependencies #416 (review))
  • add pipenv check Python environment dependency tests to CI testing (Add Python package PEP508 dependency spec and dependency safety check through pipenv (CI testing) #414)
  • update scripts to pull new woff2 commit for updated brotli dependency

Addresses the following IR:

and represents the "backwards incompatible" set of build workflow changes that will be released as v4.000.

@chrissimpkins chrissimpkins added this to the v4.000 milestone Mar 15, 2018
@chrissimpkins chrissimpkins self-assigned this Mar 15, 2018
@chrissimpkins
Copy link
Member Author

chrissimpkins commented Mar 15, 2018

@paride I've implemented an approach for compiles of the desktop ttf files using either pinned versions of all build dependencies (in a pipenv venv with pinned Py3, pinned Python build dependencies, and pinned compiled dependencies) or system installed versions of Python 3, Python build packages, and compiled packages in this branch. I've broken the CI tests until all build artifact scripting is complete so ignore the CI testing results for now. The default approach with pinned versions is working for desktop font builds without issues for me in local testing. Can I ask you to confirm that the approach to build with system PATH installed build executables and Python 3 interpreter works for you and appears to be an approach that adequately addresses the following IR that you submitted:

Confirm that all necessary build dependencies are installed on PATH. The make target for the desktop (*.ttf) compile testing on your end will then be:

$ make ttf-system

*.woff compile scripting is available too if you want to give it a shot:

$ make woff-system

Once all of the builds are refactored to support this new approach, you will be able to use:

$ make build-system

to compile *.ttf, *.woff, *.woff2 (including all subset files) artifacts with a single make target using system installed build dependencies. The default build approach for the upstream project will remain make and will use pinned dependency versions.

This will not address #394 and we will not be making the ttfautohint adjustments to address that IR as part of the v4.000 work. That will require a fair bit of visual examination to push ttfautohint to v1.8.1 since we have had issues with that version in the past. Will be work for down the road. The flag is still supported in v1.8.1 of ttfautohint.

cc @anthrotype - this implements a number of your suggestions in various open IR if you are interested. I ended up going with a pipenv approach and everything is pinned (Python interpreter, Python packages, compiled projects) in the default build. I haven't come up with a way to connect commit times with compile time settings to eliminate the time stamp issue. If you have any thoughts about how to do this let me know.

cc @spstarr build changes that are in the works as previously discussed. No new build dependencies will be introduced but this will change the existing approach, hopefully in a way that supports how you build on Linux (based upon @paride suggestions) out of the box rather than requiring significant modification of our new default approach on your end. Let me know if this can be improved to better support your package.

@paride
Copy link

paride commented Mar 15, 2018

https://github.com/source-foundry/Hack/pull/416/files#diff-4dcc5008412e33290f6089c60c2e1579R68 fails, as $TTFAH does not contain the path to the file.

After bypassing that test the ttf-system target worked fine for me.

@chrissimpkins
Copy link
Member Author

Woops. That shouldn't be tested when you use those make targets. I changed it in the woff builds. Will change it in that script too. Thanks Paride. Confirming that this is what you were looking to achieve in those IR?

@paride
Copy link

paride commented Mar 15, 2018

Yes, this solves both the issues.

As a side note, this part of the script:

# create directory if it does not exist
# (occurs with git + empty directories)
if ! [ -d build/ttf ]; then
        mkdir build/ttf
fi

makes me think that you assume that the built fonts are not part of the git repository. But they actually are (you are not .gitignoring them). This is not a problem at all, of course, I just want to be sure this is intended.

@chrissimpkins
Copy link
Member Author

That was carried over from the old scripts. The comment that I left in the code suggests that I once had a problem when the dir was empty. Don't recall the issue. Will have a look at it and can eliminate if I can't reproduce the previous problem. Thanks Paride.

@chrissimpkins
Copy link
Member Author

chrissimpkins commented Mar 15, 2018

Will add system installed dependency build testing to our CI tests to confirm that future changes to the scripts do not break this support. Added as a TODO in the OP.

Actually not going to do this. The installation of the build dependencies (particularly the compiled ones) is not straightforward on the CI testing boxes, it increases the duration of our test times, and the system installed build dependency approach is not our default (or recommended) approach to builds. Will defer this testing to downstream projects that intend to use it. It is there if needed and we can modify as necessary for any problems that are found downstream.


[packages]

fonttools = "*"

Choose a reason for hiding this comment

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

you don't need to specify fonttools in addition to fontmake, because the former is a dependency of the latter.

Copy link
Member Author

Choose a reason for hiding this comment

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

Safe to depend on the fact that this will always be the case? We have local Python scripts that depend on fontTools and will not pull it in with a pip install. pipenv simply ignores it since it was pulled in with fontmake.

Choose a reason for hiding this comment

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

ok, if you've scripts that directly depend on fonttools then it makes sense to have it there.

build-pipenv.sh Outdated
fi

# install fontTools and fontmake build dependencies with pipenv
pipenv install --python $BUILD_PYTHON_VERSION --ignore-pipfile fontmake fontTools

Choose a reason for hiding this comment

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

if you checked in the Pipfile (which already states the top-level dependencies), why do you need to duplicate them here on the command line? Shouldn't pipenv install just work?

Copy link
Member Author

Choose a reason for hiding this comment

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

Believe that you are correct. We should be able to simply do this now that I have defined the venv in my development repo with the above command:

pipenv install --ignore-pipfile

My understanding is that without the --ignore-pipfile flag, it defaults to the specification in the Pipfile rather than Pipfile.lock (i.e. `ignore pipfile.lock is default). Kenneth's docs state that Pipfile should keep versions unpinned. Unclear why this is the approach. It seems to me that if a Pipfile.lock is present, the behavior should be to default to pinned versions defined there, else use Pipfile spec which is defined by the versions contained in the current versions of the dependencies at any given time.

I will test in a cloned repo so that I can replicate what a user's development repo will see when they go through the build.

Choose a reason for hiding this comment

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

hm.. what's the point of you checking in the Pipfile.lock if you're going to ignore it?

Choose a reason for hiding this comment

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

ah, ok --ignore-pipfile option

Ignore Pipfile when installing, using the Pipfile.lock.

Choose a reason for hiding this comment

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

Kenneth's docs state that Pipfile should keep versions unpinned.

correct. Because the abstract Pipfile is what generates the concrete Pipfile.lock (a bit like setup.py vs requirements.txt)

If you did pyenv install without the --ignore-pipfile the Pipfile.lock would probably be updated with the current versions? I'm not a pipenv users so I'm not familiar with it.

Copy link
Member Author

@chrissimpkins chrissimpkins Mar 17, 2018

Choose a reason for hiding this comment

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

If you did pyenv pipenv install without the --ignore-pipfile the Pipfile.lock would probably be updated with the current versions? I'm not a pipenv users so I'm not familiar with it.

Yes, believe that this is correct Cosimo. Docs state that it is a goal of the project to use current versions of packages when possible for safety (security patch releases) reasons. You can default to Pipfile.lock specified pinned installs with the above flag as I understand it.

Updated the shell script with your suggestion here and it is working as intended. It eliminates forced rebuilds of the entire venv with each font compile. Thanks for catching this!

Copy link
Member Author

@chrissimpkins chrissimpkins Mar 17, 2018

Choose a reason for hiding this comment

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

Will be interested to see how this works once system Py3 bumps to Py3.7 on my development machine. The intended behavior is for pyenv to automatically pull the Py3.6 interpreter and install into the venv when it is not available locally. pipenv is supposed to automate this if pyenv is installed.

@paride
Copy link

paride commented Mar 16, 2018

@chrissimpkins I did try build-system and it works fine now!

@anthrotype
Copy link

we simply fix the woff2 version and a fixed brotli version comes with it.

right, that's how git submodules work

@chrissimpkins
Copy link
Member Author

@paride

I did try build-system and it works fine now!

Excellent! Are we at a place that addresses all of the recommendations in your issue reports and needs in your package?

@chrissimpkins
Copy link
Member Author

chrissimpkins commented Mar 16, 2018

@anthrotype

right, that's how git submodules work

OK, I must have misunderstood the SO article that I read. It discussed checking out specific version tags in the submodule but perhaps that was to modify the version specified in the top level git repo? I was interpreting it as an attempt to pin what is pulled in as the HEAD of the submodule.

For my own knowledge, is it possible to do what I suggested should you want to modify the git submodule relative to that defined in the top level git repo? I know, likely not advisable. Trying to understand how these work. Is it simply like an semi-isolated (other than top level project defines the commit where it is fixed) embedded git repository that can be manipulated as with a standard git repo when you are located within one of its directories/sub-directories?

Relevant to this project because it does not appear that woff2 is very actively developed whereas brotli is. I may toy with pushing the brotli version since more recent releases tout better compression ratios. Need to figure out how to test the woff2 files that are produced... I suppose that manual visual testing will suffice for now.

@chrissimpkins
Copy link
Member Author

@paride

I also removed the from __future__ import statement in e20d786. This was a request in your Python 3 transition IR. I believe that this is the only location where this is found in local scripts in the project.

@chrissimpkins
Copy link
Member Author

And in case you missed it, when you build-system now, you are building with python3. This is explicit in the script. Both approaches use python3 only.

@anthrotype
Copy link

Is it simply like an semi-isolated (other than top level project defines the commit where it is fixed) embedded git repository that can be manipulated as with a standard git repo when you are located within one of its directories/sub-directories?

pretty much, yes.

it does not appear that woff2 is very actively developed

send a PR to update the brotli submodule and I can ping the right people ;)

@paride
Copy link

paride commented Mar 16, 2018

@chrissimpkins It seems to me that all my packaging related issues are now solved, and Hack 4 will be packaged without the need to patch anything. Thanks a lot!

@chrissimpkins
Copy link
Member Author

chrissimpkins commented Mar 16, 2018

@anthrotype

Thanks for all of the input Cosimo. I appreciate it! I will prompt woff2 with a new issue report (or PR if I can figure out development with submodules) :)

@chrissimpkins
Copy link
Member Author

@paride

It seems to me that all my packaging related issues are now solved, and Hack 4 will be packaged without the need to patch anything. Thanks a lot!

Great to hear that! Happy to help do our part to make what you do easier.

@fabiangreffrath you mentioned that you would like to get to reproducible builds in the Debian package during our discussions about these changes. See the changes that we made here, in part based upon requests from Paride. There will be new optional make targets available as of v4.0 that allow you to build fonts with system PATH installed versions of all build dependencies in this project. We also moved to use whatever is defined as python3 as the python interpreter for all Python packages and local Python scripts. If you have any feedback on the approach, particularly if you suggest changes to help you support the desire for reproducible builds better, please weigh in.

@chrissimpkins
Copy link
Member Author

chrissimpkins commented Mar 16, 2018

For all other stakeholders in the build, now is your chance to weigh in on the changes. Once we push this I would like to avoid further build process breaking changes for some time. Let's have the conversations now.

My next planned major version release after these changes will include the Python bindings for brotli and zopfli so that we can transition to the use of Python tools to create our web fonts. This will be most important to me for our webfont subsets because it will simplify the process significantly relative to our current fontmake hack to create subsets. The introduction of these two (at least) new build dependencies will take a bit of time to propagate through the downstream packaging approval processes on Linux distros and I would like to allow projects some time (~ 6-12 months) to achieve this before we make this move. I am hoping to avoid other "backwards incompatible" changes to the build process during this period. Lots of design work to do and we will use this block of time to do it before we delve into build changes again.

Everything is open to feedback and debate. Make target names, transition to Python 3 default, other build needs that exist out there, etc. Let us know.

…es brotli v1.0.1)

the macOS build issue has been addressed and compiles work on macOS
@chrissimpkins
Copy link
Member Author

chrissimpkins commented Mar 16, 2018

Converted to upstream woff2 in 96a35cb. This is pinned at v1.0.2 (woff2) and includes v1.0.1 of brotli.

Submitted PR google/woff2#110 to woff2 project in order to bump brotli submodule to v1.0.3. Changelog states that compression ratios improved with brotli 1.0.2 --> 1.0.3 changes. Unclear impact on font compiles. We will see if this leads to smaller woff2 web fonts.

@@ -14,6 +14,7 @@
# The woff2 git clone directory.
BUILD="$HOME"
INST="$HOME/woff2"
WOFF2_VERSION_TAG="v1.0.2"
Copy link
Member Author

Choose a reason for hiding this comment

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

location of new definition of woff2 project version (by version git tag)

# checkout desired version tag
echo " "
echo "Checking out woff2 version tag $WOFF2_VERSION_TAG"
git checkout $WOFF2_VERSION_TAG
Copy link
Member Author

@chrissimpkins chrissimpkins Mar 17, 2018

Choose a reason for hiding this comment

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

woff2 is now checked out at the shell script constant defined git version tag from the upstream Google repo instead of pulling from a version pinned downstream branch of the woff2 repository

@chrissimpkins
Copy link
Member Author

As of bc6a683 updated build documentation is available that is current with all changes here.

@chrissimpkins
Copy link
Member Author

chrissimpkins commented Mar 21, 2018

Updated woff2_compress to use brotli v1.0.3 in 2a46cad. Renders with updated build tooling 👍 c/w previous builds. Pre:post waterfall of woff2 renders (Firefox on macOS):

k7nka-image

@chrissimpkins
Copy link
Member Author

No significant decrease in the woff2 file sizes with the brotli v1.0.3 updates.

@chrissimpkins chrissimpkins merged commit 2a46cad into dev Apr 9, 2018
@chrissimpkins
Copy link
Member Author

Merged into dev branch and will be included in v4.000 release of Hack

@chrissimpkins chrissimpkins deleted the build-scripting branch April 9, 2018 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

3 participants