silnrsi / smith

font development, testing and release
Other
14 stars 5 forks source link

WOFFs appear in wrong packages when building multiple families #85

Closed jvgaultney closed 3 months ago

jvgaultney commented 5 months ago

When a wscript builds two families, typically a main one and a supplemental one with a different name, Smith incorrectly places all the WOFFs for both families in the web/ folder of both packages. See Gentium.

This even happens if the supplemental family does not produce WOFFs, as in Lisu Bosa.

devosb commented 4 months ago

One work-around is to specify a different folder for the WOFFs in wscript, such as woff. Now the correct WOFFs (and none from the other family) will be in the woff folder. Downside is that the HTML and CSS is no longer a working example, but it is still a readable example. And, a user sees two folders, web and woff.

Another work around is to remove the web folder from the DOCDIR variable in wscript. If the remaining folder is documentation you can just comment out the whole line, as the documentation folder is a smith default. Note that DOCDIR might be referenced later in wscript for handling multiple packages, so might want to keep the DOCDIR assignment, even with just the default value. The downside is that the HTML and CSS would no longer be in the release package, but I would suspect most users just need the TTFs (and maybe the WOFFs), and are not going to be looking at the HTML and CSS.

Maybe there is a way to avoid all of the mentioned downsides by carefully specifying DOCDIR (at the top of wscript) and docdir (in the package function).

jvgaultney commented 4 months ago

Neither of those workarounds is acceptable as they would remove documentation files (that I know people actually use) or change the standard structure of our release packages. If you have any ideas of how we might use DOCDIR and docdir as you suggest please give it a try. Lisu Bosa will be easier to use as a experiment. (Although you'd need to test with Gentium too as that project created WOFFs for both families.)

mhosken commented 4 months ago

I will start out by suggesting that each of the packages have a different web/ directory. I notice that the example css file only references GenitumPlus and not GentiumBookPlus, so there may need to be separate files for those. Then in the wscript where there is package(..., docdir=DOCDIR), I would change that to package(..., docdir=['documentation', 'web_book']) or somesuch with a corresponding change to the woff parameter to the dspace for the book family.

If we want to have the book package take its separate web_book and rename it to web inside the package, then that's probably an enhancement request and we'll have to think about how to express that in the wscript.

jvgaultney commented 4 months ago

We would want the structure of both packages to be the same - where the WOFFs are found in the 'web' folder inside the each package. That's how all our other font release packages are structured, and we wouldn't want supplemental ones to be oddly different.

We will need some way to have separate sets of files for things like the HTML example in the web folder, but they would all need to end up in the 'web' folder.

jvgaultney commented 4 months ago

(BTW - the need for separate packages for primary/supplemental families and for things like language-specific versions will only be increasing I expect.)

mhosken commented 4 months ago

so how do you want to describe the docdir? docdir=["documentation", ("web_book", "web")] or docdir = {"documentation": "documentation", "webdir": "web"} or?

jvgaultney commented 4 months ago

What's confusing is that DOCDIR and docdir as discussed above seems to have an effect on where the WOFF files end up, not just the documentation files. It would be good to separate the two uses. It would be good to limit `docdir' to only define documentation sources/targets:

docdir = {sourcedir: targetdir, sourcedir2: targetdir2, ...}

for example,

docdir = {"documentation_book": "documentation", "web_book": "web"}

Then a `dir = targetwoffdir' parameter could be added to the woff() command to specify where the WOFFs should be placed.

jvgaultney commented 4 months ago

Also - if the package does not have a woff() command no woffs should end up in the package. docdirshould also only place files in a "web" folder if explicitly stated.

IOW if there is no woff() command and no "web" directory defined via docdir then there should be no "web" directory in the release package.

n7s commented 3 months ago

@mhosken I can cherry-pick your fix in 3321f28 to the docker-noble branch which we haven't merged back to main yet, IOW your fix will not show up in the CI or interactive image right now.

nrsiward commented 3 months ago

I tested with Gentium. I checked out the docker-noble branch and cherry-picked MH's commit above (in a smith container running from WSL Ubuntu 24.04). I was able to run smith from source instead of using the standard smith in the container.

I modified the wscript to put the Book woffs in the results/web_book folder and changed docdir for the Book family to

docdir = {"documentation": "documentation", "web_book": "web"}

Gentium did build and zip files could be created. In the zip file for Book, the web folder had the expected contents, but there was also a web_book folder which had identical contents.

Is the syntax I used for doc_dir correct?

jvgaultney commented 3 months ago

You also need a 'dontship' in the woff(). Here are the recommended changes needed:

--- a/wscript
+++ b/wscript
@@ -50,7 +50,7 @@ for dspace in ('Roman', 'Italic'):
 #                pdf=fret(params = '-r -oi')
                 )

-bookpackage = package(appname = "GentiumBookPlus", docdir = DOCDIR)
+bookpackage = package(appname = "GentiumBookPlus", docdir = {"documentation": "documentation", "web_dir": "web"})
 bookfamily = "GentiumBookPlus"

 getufoinfo('source/masters/' + sourcefontfamily + '-Regular' + '.ufo', bookpackage)
@@ -70,8 +70,9 @@ for dspace in ('Roman', 'Italic'):
                     to_ufo = 'False' # copies to instance UFOs
                     ),
                 #typetuner = typetuner('source/typetuner/feat_all.xml'),
-                woff = woff('web/${DS:FILENAME_BASE}.woff',
-                    metadata=f'../source/gentiumbookplus-WOFF-metadata.xml'),
+                woff = woff('web_dir/${DS:FILENAME_BASE}.woff',
+                    metadata=f'../source/gentiumbookplus-WOFF-metadata.xml',
+                    dontship=True),
                 version = VERSION,
                 shortcircuit = False,
                 package = bookpackage
nrsiward commented 3 months ago

Adding dontship=true to the woff() parameter fixed the problem! IOW, it prevented the web_book folder from being in the zip file.

n7s commented 3 months ago

After testing and verifying the fix works as intended, 3321f28 was cherry-picked to the docker-noble branch for deployment.