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

Bundle the font #412

Closed
wants to merge 1 commit into from
Closed

Bundle the font #412

wants to merge 1 commit into from

Conversation

tstarling
Copy link

For performance, offline support and privacy.

Cut down the CSS file from the package so that we only need to bundle
the one font file, like what Google was doing.

For performance, offline support and privacy.

Cut down the CSS file from the package so that we only need to bundle
the one font file, like what Google was doing.
Copy link
Contributor

@Krinkle Krinkle left a comment

Choose a reason for hiding this comment

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

Tested locally and confirmed that Parcel indeed knows to copy and bundle the file from node_modules.

Opening index.htm from the latest stable release zip, involves two network requests to fonts.googleapis.com. Checking out this patch, running scripts/prepare-test-installation.sh, and opening dist/release/index.html involves no network requests.

@jlfwong
Copy link
Owner

jlfwong commented Dec 16, 2022

Thank you! Next time I'm ready to cut a release, I'll test this myself and merge

@tstarling
Copy link
Author

Hi @jlfwong, thanks for your response. @Krinkle and I are on the performance team at Wikimedia and we're looking at integrating speedscope with Wikimedia websites using our own sampling profiler for PHP. We're doing it in a way that should be usable by other PHP applications. The details are at https://phabricator.wikimedia.org/T291015

@jlfwong
Copy link
Owner

jlfwong commented Jun 4, 2023

When I look at https://fonts.googleapis.com/css?family=Source+Code+Pro, it lists the woff2 formatted files rather than ttf. It looks like the npm package also contains these files: node_modules/source-code-pro/WOFF2/TTF/SourceCodePro-Regular.ttf.woff2. The woff files are smaller, which appeals to me. Is there a reason to use the ttf files instead of the woff2 files?

I'm also not sure what the licensing requirements of this become once it's bundled with the software.

From looking at https://github.com/adobe-fonts/source-code-pro/blob/release/LICENSE.md, it seems like I'm supposed to include licensing information somewhere in this project. If you're interested in having this merged, can you please look into that and see what, if anything, would need to change in this repository?

@tstarling
Copy link
Author

When I look at https://fonts.googleapis.com/css?family=Source+Code+Pro, it lists the woff2 formatted files rather than ttf. It looks like the npm package also contains these files: node_modules/source-code-pro/WOFF2/TTF/SourceCodePro-Regular.ttf.woff2. The woff files are smaller, which appeals to me. Is there a reason to use the ttf files instead of the woff2 files?

I didn't realise that it delivers different stylesheets depending on User-Agent. If you use curl, it only shows TTF.

I'm also not sure what the licensing requirements of this become once it's bundled with the software.

From looking at https://github.com/adobe-fonts/source-code-pro/blob/release/LICENSE.md, it seems like I'm supposed to include licensing information somewhere in this project. If you're interested in having this merged, can you please look into that and see what, if anything, would need to change in this repository?

Yes, I think you're right that a license notice is required. Although probably all of your runtime dependencies have this problem.

@jlfwong
Copy link
Owner

jlfwong commented Jun 5, 2023

I didn't realise that it delivers different stylesheets depending on User-Agent. If you use curl, it only shows TTF.

Gotcha. As far as I can tell, WOFF-2 is very widely supported now (https://caniuse.com/?search=woff2), so I'd prefer this woff-2 instead to keep things as compact as possible.

Yes, I think you're right that a license notice is required. Although probably all of your runtime dependencies have this problem.

This might be true, but AFAIK all of my other runtime dependencies have common software licenses like MIT, rather than a font-specific license. I've never heard of folks getting in trouble for bundling MIT licenses for npm dependencies (otherwise every single npm package would need this). But I know much less about legal issues around fonts.

@Krinkle
Copy link
Contributor

Krinkle commented Apr 12, 2024

Yes, I think you're right that a license notice is required. Although probably all of your runtime dependencies have this problem.

This might be true, but AFAIK all of my other runtime dependencies have common software licenses like MIT, rather than a font-specific license. I've never heard of folks getting in trouble for bundling MIT licenses for npm dependencies (otherwise every single npm package would need this).

Stuff about MIT and FLOSS licenses

I am not a lawyer, but my understanding based on working in open source at Wikimedia Foundation and in the jQuery Team (OpenJS/Linux Foundation) for over a decade, is as follows:

The MIT license is among the most liberal in open source. Practically the closest thing to being entirely in the public domain. You can do literally anything you want with it, as long as you attribute its authors/copyright line somewhere. Similar in spirit to the Creative Commons Attribution license in that sense. "Attribution", thus requires the copyright line to be preserved or otherwise presented in distributions that include copies of the software.

Most npm packages are not end-user applications or products, and thus don't have any practical involvement around this. For a typical library package, its only concern is its own license in the repo/package uploaded to npm, and anyting it uses or that others use it for, is handled transparently by npm-install later on when someone else uses it. When you upload your own package to npm, usually, you are not distributing your dependencies for they are not included in the package. package.json merely points to them by name, and it is later on, that someone else runs npm-install and then installs both it and the dependencies. Even for end-user applications like ESLint and Grunt, npm solves it automatically. Naturally, their dependencies are either not yet downloaded (you haven't ran npm-install yet) or both a copy of the code and its LICENSE file are present (after you run npm-install, in the node_modules directory). This requires no further action on your part.

If you do something special with the code and distribute it by other means outside npm, or if you attach artefacts to your npm package that bundle/embed/inline code from dependencies, then you have to preserve or present the attribution by other means yourself. For example:

  • If you distribute a tarball that includes the node_modules directory, you probably don't have to do anything. This naturally preserves any license header in source files and any separate license text files in directories.
  • Firefox (the finished binary on your computer) compiles and embeds lots of C++ and JS software under a variety of licenses. There is not really a way to view this in source form after compilation, and thus does not naturally preserve attribution. This is why Mozilla included a semi-automated way to present this in their UI. You can go to Firefox > About > License information (or about:license) and you'll find a list there of each package they embedded/compiled, and the legally required attribution (e.g. Copyright John Doe). Most FLOSS licenses, also require a copy of (or web link to) the license terms. You'll find similar "about" views in many desktop apps, as well as mobile apps for iOS/Android often have such a license page.

In the case of Speedscope, Parcel is used to produce a single JS file and a single CSS file, and any comments are stripped. My best guess is that this is technically in violation of the MIT license for your dependencies. When I publish a copy of Speedscope on my own domain, I'm (probably?) not in violation of your license, since the Speedscope UI links to your GitHub repo, where your license file can be viewed. However, there's not really a clear path to the copyright statement of your dependencies (much less which ones were bundled, or which versions, or which copyright statement they had at that time). Parcel strips it all out. I could not find anything in the the Speedscope release tarball, e.g. the HTML, JS file, or other text files, that indicates what code was bundled or who holds their copyright. The source repo also does not distinguish in package.json between dev and prod dependencies (noting that your dev dependencies are generally not distributed. E.g. the tools you use for bundling, testing, minification are not being distributed in the release and do not require attribution). Instead, it seems your prod dependencies are listed among dev-only, and some of them are then bunded into the JS file during release. This makes sense for efficiency, but means that the npm distribution is probably technically in violation as well.

Font

As for the font, https://openfontlicense.org/ofl-faq/ does a good job of answering the hard questions. It is in essense an even more liberal license than the MIT license, as it presents plethora of scenarios where you do not even have to attribute its author or copyright holders. Unlike commerically licensed fonts where you may have to pay to download them, and then pay pay for permission to use the font in a brand or publication.

The OFL is much like the MIT license in that it permits copying, modification and re-distribution, so long as you preserve a copy of the original license text and/or other attribution. In addition, it grants very useful and practical exemptions for work that merely applies the font (e.g. a logo or a screenshot of Speedscope where the font is used), where it requires no attribution at all. Given that Speedscope releases would include a copy of the file as-is, atttribution would be required, however.

While by no means required, I would personally choose the following as the simplest approach:

  • Create the assets/source-code-pro directory with in it the SourceCodePro-Regular.ttf.woff2 file and https://github.com/adobe-fonts/source-code-pro/blob/release/LICENSE.md.
  • Check, or update as needed, the release step so that during or after the Parcel step, there are both SourceCodePro-Regular.woff2 and SourceCodePro-Regular.license.md in the dist directory. Alternatively, you could create your own "NOTICE.txt" file in Git with the two attribution lines of the license and make sure to copy that into the dist folder. You may want to expand that at some point to include the attribution of this repo and other dependencies as well.

If this sounds acceptable, I'd be happy to submit a PR doing that.

@jlfwong
Copy link
Owner

jlfwong commented Apr 12, 2024

Hey @Krinkle! Thanks for the thorough rundown, and especially for hunting down the licensing details of source-code-pro!

Regarding the technical MIT violations, I think you're probably right too. That sounds, frankly, like a pain to resolve, and certainly a separate concern from what this PR is addressing. So for now I think I'll punt on that.

The simplest approach you propose sounds good to me, and I'd be happy to accept a PR doing that.

Krinkle pushed a commit to Krinkle/speedscope that referenced this pull request Apr 13, 2024
For performance, offline support and privacy.

Cut down the CSS file from the package so that we only need to bundle
the one font file, similar to what Google Fonts did.

Ref https://openfontlicense.org/ofl-faq/
Ref https://software.sil.org/fonts/faq/
Closes jlfwong#412
@Krinkle Krinkle mentioned this pull request Apr 13, 2024
Krinkle added a commit to Krinkle/speedscope that referenced this pull request Apr 13, 2024
For performance, offline support and privacy.

Cut down the CSS file from the package so that we only need to bundle
the one font file, similar to what Google Fonts did.

Ref https://openfontlicense.org/ofl-faq/
Ref https://software.sil.org/fonts/faq/
Closes jlfwong#412

Co-authored-by: Timo Tijhof <[email protected]>
jlfwong pushed a commit that referenced this pull request Apr 15, 2024
For performance, offline support and privacy.

Continues from #412
@jlfwong
Copy link
Owner

jlfwong commented Apr 15, 2024

Closing in favor of #472

@jlfwong jlfwong closed this Apr 15, 2024
sransara added a commit to sransara/speedscope that referenced this pull request Jul 13, 2024
commit d69f3d0
Author: Tom Levy <[email protected]>
Date:   Tue Apr 16 06:50:13 2024 +1200

    Fix bug where import after error continues failing (jlfwong#463)

    Steps to reproduce:
    1. Open https://www.speedscope.app/
    2. Try to import an invalid file such as [invalid.json](https://github.com/jlfwong/speedscope/files/14167326/invalid.json). The page says "Something went wrong. Check the JS console for more details."
    3. Now try to import a valid file such as [simple.json](https://github.com/jlfwong/speedscope/files/14167335/simple.json). The page says "Something went wrong. Check the JS console for more details." even though this second file is valid.

    Explanation of the fix (copied from the commit message):
    > We need to clear the error flag, otherwise once there is an error and we display "Something went wrong" we will keep displaying that forever even when the user imports a valid file.

commit 25f671e
Author: Timo Tijhof <[email protected]>
Date:   Mon Apr 15 19:45:12 2024 +0100

    Bundle the font (jlfwong#472)

    For performance, offline support and privacy.

    Continues from jlfwong#412

commit 0121cf9
Author: Tom Levy <[email protected]>
Date:   Tue Feb 6 07:58:18 2024 +1300

    Clarify specification of startValue in speedscope file format (jlfwong#464)

    The previous description ("event values will be relative to this
    startValue") was ambiguous.

    Suppose the profile starts at wall time 1000ms and the first event is
    at wall time 1003ms.

    The intention is that startValue should be set to 1000 and the "at"
    value of the event should be set to 1003. The viewer's time axis will
    start at 0ms and the first event will be displayed at 3ms.

    But the previous description could be incorrectly interpreted as
    saying that the "at" value of the first event should be set to 3 (the
    time relative to startValue, as opposed to the absolute wall time).

    Clarify that "relative" is referring to how the viewer displays the
    data, not about which values to store in the file.

commit 68fd88c
Author: Jamie Wong <[email protected]>
Date:   Fri Jan 12 09:57:30 2024 -0800

    1.20.0

commit a6d7001
Author: Zachary Marion <[email protected]>
Date:   Wed Jan 3 13:04:22 2024 -0800

    Partition based on samples instead of traceEvents when importing a sample-based chrome trace (jlfwong#460)

    In the existing code, if `traceEvents` did not have any importable events, the profile would be marked as empty. This was a bug, as the dev Hermes profiles I was testing with had one X event which made the code work. However we do not need to guarantee (and the spec doesn't seem to) any traceEvents being present as long as we have samples and stack frames.

    When I tested a production profile taken from Hermes it did not have any importable events, just a metadata event with a thread name. This PR updates the implementation to iterate over partitioned samples instead of traceEvents so we construct profiles for all thread + process pairs referenced in the samples array.

    | Before | After |
    | --- | ----- |
    | <img width="1454" alt="Screenshot 2024-01-03 at 9 58 56 AM" src="https://github.com/jlfwong/speedscope/assets/9957046/78c098a7-b374-4492-ad13-9ca79afdb40c"> | <img width="1450" alt="Screenshot 2024-01-03 at 9 58 17 AM" src="https://github.com/jlfwong/speedscope/assets/9957046/d2d5e82b-fa3e-43db-bf8a-e8c3b84cd13a"> |

commit a3c0b15
Author: Jamie Wong <[email protected]>
Date:   Wed Dec 27 23:57:14 2023 -0500

    1.19.0

commit e9a17d5
Author: Zachary Marion <[email protected]>
Date:   Wed Dec 27 23:56:27 2023 -0500

    Improve hermes profile frame keys to better group frames (jlfwong#459)

    Because all args are serialized in the key for hermes profiles, frames were not properly getting grouped since the "parent" property was different. This PR ensures that the frame key is properly constructed from the function name, file name, line + column number.

    | Before | After |
    | ----- | ----- |
    | <img width="1727" alt="Screenshot 2023-12-27 at 2 25 32 PM" src="https://github.com/jlfwong/speedscope/assets/9957046/4ec04653-5a80-4f34-9506-48e0eb415983"> | <img width="1728" alt="Screenshot 2023-12-27 at 2 26 02 PM" src="https://github.com/jlfwong/speedscope/assets/9957046/c3edc447-e8ad-4757-8576-cbb8c27eafe3"> |
    | <img width="1720" alt="Screenshot 2023-12-27 at 2 13 37 PM" src="https://github.com/jlfwong/speedscope/assets/9957046/10194aba-8f26-48ca-bf15-06917b44ea99"> | <img width="1728" alt="Screenshot 2023-12-27 at 2 14 20 PM" src="https://github.com/jlfwong/speedscope/assets/9957046/75c32401-1705-4b23-b71a-5f0a720160dd"> |

commit 3f3da22
Author: Jamie Wong <[email protected]>
Date:   Wed Dec 27 10:48:07 2023 -0500

    Update README.md

    - Add link to Hermes import instructions
    - Re-order import sources higher in the list
    - Update first line

commit bea0ef6
Author: Jamie Wong <[email protected]>
Date:   Tue Dec 26 17:12:59 2023 -0500

    1.18.0

commit 4feb1e5
Author: Zachary Marion <[email protected]>
Date:   Tue Dec 26 17:00:18 2023 -0500

    Add hermes-specific support for the trace event format (jlfwong#458)

    Profiles that are transformed into the hermes trace event format are guaranteed to have specific arguments that supply metadata that is useful for debugging - namely the filename, line + col number that the function call originated from, with sourcemaps applied. This PR adds specific support for this information to the trace event importer. This means that we can have the Frame name be just the name of the function, since we know all the information we want to be displayed in the UI is captured in the frame info, which makes the traces cleaner to look at.

    | Before | After |
    |-----|-----|
    | <img width="1728" alt="Screenshot 2023-12-26 at 2 40 01 PM" src="https://github.com/jlfwong/speedscope/assets/9957046/f9dff608-5df3-4098-b1f8-91a69185d906"> | <img width="1726" alt="Screenshot 2023-12-26 at 2 39 13 PM" src="https://github.com/jlfwong/speedscope/assets/9957046/b8ff360e-a316-4bef-8ebc-620c9ff1a998"> |
    | <img width="1728" alt="Screenshot 2023-12-26 at 2 41 03 PM" src="https://github.com/jlfwong/speedscope/assets/9957046/127a49b5-458e-4ac8-934a-202e565cb20f"> | <img width="1728" alt="Screenshot 2023-12-26 at 2 41 29 PM" src="https://github.com/jlfwong/speedscope/assets/9957046/ebb285ce-6b33-4535-8e45-b9ada4e4d97f"> |

commit 88f4fe6
Author: Jamie Wong <[email protected]>
Date:   Mon Dec 25 22:52:46 2023 -0500

    Update README-ADMINS.md with npm login instructions (jlfwong#457)

commit 60f1812
Author: Jamie Wong <[email protected]>
Date:   Mon Dec 25 22:40:40 2023 -0500

    1.17.0

commit 1717fec
Author: Jamie Wong <[email protected]>
Date:   Mon Dec 25 22:37:45 2023 -0500

    Upgrade prettier, update prettier & react-hooks eslint plugins (jlfwong#456)

    Re-ran prettier with latest version

commit c296f53
Author: Jamie Wong <[email protected]>
Date:   Mon Dec 25 22:23:33 2023 -0500

    Upgrade typescript & eslint to latest, fix resulting errors (jlfwong#455)

    Also updated the ci.yml node test versions to 18.x and 20.x, given current support: https://endoflife.date/nodejs

commit 8e0fa58
Author: Jamie Wong <[email protected]>
Date:   Mon Dec 25 21:22:56 2023 -0500

    Re-enable eslint prettier rule after being accidentally disabled for 3 years (jlfwong#454)

    It looks like in jlfwong#267 (which was 3 years ago!), I accidentally disabled prettier linting altogether 😱

    https://github.com/jlfwong/speedscope/pull/267/files#diff-e2954b558f2aa82baff0e30964490d12942e0e251c1aa56c3294de6ec67b7cf5

    There's no comment in that PR about this being an intentional thing, so I have to assume this was a dumb mistake.

commit b214804
Author: Zachary Marion <[email protected]>
Date:   Mon Dec 25 21:11:45 2023 -0500

    Support the chrome JSON trace format (allows viewing of hermes traces) (jlfwong#453)

commit dfd3a0d
Author: Zachary Marion <[email protected]>
Date:   Sat Dec 16 00:27:03 2023 -0800

    Fix bug in selectQueueToTakeFromNext for trace profiles (jlfwong#450)

    I have been taking a lot of profiles using the Hermes profiler, but I noticed that they sometimes to not show up properly. After debugging what exactly was going on, I realized it was because the logic in `selectQueueToTakeFromNext` only checks for name, instead of the key for the event. I had a bunch of events with the name `anonymous` that were getting improperly exited before they should have been due to this logic.

    This fix makes the code more robust if there are added "args" which differentiate an event from another (as is the case in Hermes profiles), however it would still be an issue if they key just defaults to the name.

    Example profile before:

    <img width="1728" alt="Screenshot 2023-12-15 at 12 54 04 AM" src="https://github.com/jlfwong/speedscope/assets/9957046/345f556e-f944-40f1-b59c-748133acb950">

    What it should look like (in Perfetto):
    <img width="1051" alt="Screenshot 2023-12-15 at 8 51 38 AM" src="https://github.com/jlfwong/speedscope/assets/9957046/7473cdf8-95f1-49de-a0c7-ef4ac081ff85">

    After the fix:

    <img width="1728" alt="Screenshot 2023-12-15 at 12 54 29 AM" src="https://github.com/jlfwong/speedscope/assets/9957046/56b0870a-538b-4916-acc8-de2b7dfd78eb">

commit ac4a015
Author: Jamie Wong <[email protected]>
Date:   Thu Dec 7 18:43:47 2023 -0800

    Add bounds checking for sampleTypeIndex (jlfwong#449)

    Wow this was surprising. As reported in jlfwong#448, the `simple.speedscope.json` file failed import. This was surprising to me because there's a test that covers that file to ensure it imports correctly.

    The file provided in jlfwong#448, however, is from a version of speedscope from 5 years ago before the file had been pretty printed. It turns out that when this *particular* file is not pretty-printed, it's a schematically valid pprof profile.

    The fix is to do some bounds checks and return null. After the change, the file imports as you'd expect after realizing its not actually a valid pprof profile.

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

Successfully merging this pull request may close these issues.

3 participants