Skip to content

Fallback mechanism for the woff/woff2 dependencies #403

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

Closed

Conversation

paride
Copy link

@paride paride commented Feb 22, 2018

If sfnt2woff-zopfli and woff2_compress binaries are not found in the
installation directory targeted when the scripts are called with
--install-dependencies, then look if the binaries are installed
system-wide (i.e., in the $PATH).

IR #395.

chrissimpkins and others added 5 commits February 7, 2018 15:07
If sfnt2woff-zopfli and woff2_compress binaries are not found in the
installation directory targeted when the scripts are called with
--install-dependencies, then look if the binaries are installed
system-wide (i.e., in the $PATH).

Fixes source-foundry#395.
Copy link
Member

@chrissimpkins chrissimpkins left a comment

Choose a reason for hiding this comment

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

Version pinning issue applies here too. Should we be trying to pin these to specific versions or do you want the flexibility to build with whatever version you have available to you and test for unintended changes in the web fonts yourself? Upstream we will be using the approach defined by the build-with-dependencies target.

INSTALLFLAG=1
else
SFNTWOFF_BIN="sfnt2woff-zopfli"
echo "sfnt2woff-zopfli executable identified"
Copy link
Member

Choose a reason for hiding this comment

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

Possible to modify the stdout for users to explicitly indicate that they are building with the system installed version of sfnt2woff-zopfli?

INSTALLFLAG=1
else
WOFF2_BIN="woff2_compress"
echo "woff2_compress executable identified"
Copy link
Member

Choose a reason for hiding this comment

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

and here for woff2_compress

@@ -93,6 +93,13 @@ if [ "$1" = "--install-dependencies" ]
cd "$CUR_DIR" || exit 1
fi

# If the binary is not found, look if it is installed system-wide
Copy link
Member

Choose a reason for hiding this comment

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

or perhaps better to introduce it in these blocks. Somewhere so that it is clear in the standard output trail that a system installed version is being applied rather than the approach used with the build-with-dependencies target. I will do the same for the Python installs depending upon how we approach them with version pinning.

@chrissimpkins
Copy link
Member

In light of @anthrotype's excellent suggestion to establish reproducible builds from a Python build dependency standpoint and my desire to make this the case across all build dependencies, let's move this to a separate script with a new make target rather than creating a new build path that is determined by the presence of system wide installs of these web font build dependencies. I am happy to support this here but we should not make this the default if reproducible builds are to be a goal going forward. This PR proposal takes us in the opposite direction and is probably best suited for a new make target that is explicitly requested by the builder and is not the project build make target default.

To support this, I need to change the current approach for both Python dependencies and ttfautohint as currently defined in the dependency install scripting.

I am going to postpone the remaining planned design objectives for the v3.003 release and we will push this transition in our build chain as the last set of changes that we will make before we release the existing dev branch changes as v3.003. I will create a branch off of dev where we can work on this and will rebase this PR on that branch.

@chrissimpkins chrissimpkins changed the base branch from master to build-scripting February 26, 2018 01:16
@chrissimpkins
Copy link
Member

chrissimpkins commented Feb 26, 2018

Rebased this PR on the build-scripting branch where the build scripting work for outstanding v3.003 milestone IR work will occur. Added this PR to the v3.003 milestone.

@chrissimpkins
Copy link
Member

Modified your OP to prevent automated closure of the associated issue report. I am going to merge this and use it as a basis for the script updates. This work will likely wind up on a different script path. Still trying to decide on the approach. Let's leave this open to continue the work. I will take care of the necessary changes to support the new make targets and the needs included in the work here will be part of those new targets.

@chrissimpkins
Copy link
Member

Current work in #416 invalidates this PR. If you are interested in performing any of the work to support the remaining open build source refactor items that remain open in #416, please open a new PR. Thanks Paride.

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

Successfully merging this pull request may close these issues.

2 participants