Skip to content
This repository has been archived by the owner on Jun 4, 2024. It is now read-only.

Add MCAP mdimporter to macOS app bundle #4745

Merged
merged 4 commits into from
Nov 3, 2022
Merged

Conversation

jtbandes
Copy link
Member

@jtbandes jtbandes commented Nov 2, 2022

User-Facing Changes
Added a Spotlight importer for MCAP files to the macOS desktop app. The names of topics, schemas, attachments, and metadata in MCAP files can now be used as search terms in Finder and Spotlight.

Description
Added https://github.com/foxglove/MCAPSpotlightImporter to app bundle via afterPack script.

Also made some updates to Studio's Info.plist:

  • Make Studio the "owner/exporter" of .mcap and .foxe types
  • Rename MCAP UTI from dev.foxglove.mcap to dev.mcap.mcap now that mcap.dev is a registered domain
  • Added McapIcon.png which was missing from the app bundle
  • Add more conformances to "functional" type hierarchy for mcap and bag files: public.log and public.composite-content
  • Change .bag description from "ROS Bag File" to "ROS 1 Bag File"
  • Added reference URLs for all the file types

Finder Get Info window

Finder search

Finder search by topics and schemas

Spotlight search

desktop/afterPack.ts Outdated Show resolved Hide resolved
const appName = context.packager.appInfo.productFilename;
const appPath = path.join(appOutDir, `${appName}.app`);
const spotlightDirPath = path.join(appPath, "Contents", "Library", "Spotlight");
const zipPath = path.join(outDir, "MCAPSpotlightImporter.mdimporter.zip");
Copy link
Contributor

Choose a reason for hiding this comment

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

why not unzip from memory rather than downloading the zip into the FS first?

Copy link
Member Author

Choose a reason for hiding this comment

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

Mainly because there was a convenient extractZip function available from @actions/tool-cache

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this would cause an exciting bug if the extract dir was in ~/Downloads, and Mac OS went ahead and applied the com.apple.quarantine xattr to the bundled executable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is this a thing that happens automatically for arbitrary files? This file is being created by some JS code, not downloaded from a browser or by extracting a dmg. I don't think I've heard of the quarantine attribute being added for files created directly by programs that aren't browsers.

.update(await fs.readFile(zipPath))
.digest("hex");
if (actualSHA !== zipSHA) {
throw new Error(`SHA mismatch for ${zipURL}: expected ${zipSHA}, got ${actualSHA}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there an action that the user can take here to fix this? Or will the solution vary too much to mention an action?

Copy link
Member Author

Choose a reason for hiding this comment

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

This indicates the download was corrupt or the url/sha is simply wrong. It's not within the scope of this code to do something about it, the user will have to figure out what went wrong (e.g. a network error)

Hopefully 99.99% of the time nobody will encounter this :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Btw, "user" here means "studio release engineer"

Copy link
Contributor

@defunctzombie defunctzombie left a comment

Choose a reason for hiding this comment

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

I'm all for this feature.

I'd feel happier about the deployment story if we could package as part of the release process (even if it is built separately) using the extraResources feature: https://www.electron.build/configuration/contents.html

This would let us avoid an additional download step and potential for failure. We can limit this to the macos package so Windows and Linux users are not paying with larger bundle sizes.

If you looked into that and there were other complications with that approach then we can use this one and I'll approve the PR.

Copy link
Contributor

@defunctzombie defunctzombie left a comment

Choose a reason for hiding this comment

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

I now understand that none of the download is happening on the user's machine. These download steps are in afterPack and doing exactly what I imagined - downloading the file for inclusion into the package bundle.

@jtbandes jtbandes merged commit 1a0cbab into main Nov 3, 2022
@jtbandes jtbandes deleted the jacob/mcap-mdimporter branch November 3, 2022 19:25
jtbandes added a commit to foxglove/mcap that referenced this pull request Nov 3, 2022
The Swift indexed reader is being used in production as of foxglove/studio#4745
jtbandes added a commit to foxglove/mcap that referenced this pull request Nov 3, 2022
The Swift indexed reader is being used in production as of foxglove/studio#4745
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants