Skip to content

Conversation

@GrantComm
Copy link
Collaborator

Copies notice to macos app bundle . This ensures the third party licenses are visible in the about window on mac.

Manual Tests:
MacOS

  • Build and run dive. Click "About Dive" and ensure license info is visible
Screenshot 2025-12-18 at 4 54 15 PM

@GrantComm GrantComm marked this pull request as ready for review December 19, 2025 01:19
@hysw
Copy link
Collaborator

hysw commented Dec 19, 2025

Nit, can we write proper CMake instead of writing more adhoc scripts? e.g. #662

@angela28chen
Copy link
Collaborator

angela28chen commented Dec 19, 2025

I think we should attempt to have similar places for the NOTICE file for mac vs other host platforms and across release/debug builds. How about we place the NOTICE file next to the host tool executables:

  • On mac Instead of ${CMAKE_INSTALL_PREFIX}/dive.app/Contents/Resources/ we put it ${CMAKE_INSTALL_PREFIX}/dive.app/Contents/MacOS
  • We can extend the functionality of ResolveResourcesLocalPath() (I just merged this earlier today) to search the executable folder (exe_dir) as well, and we use it here in CreateLicenseLayout()
  • For windows/linux we add an install command to put NOTICE next to wherever the UI binary ends up in dev builds. In the release version it's already manually positioned correctly.

I'm suggesting we put it next to the host tools executables rather than the device resources since the latter is more "files built when we target android and may be deployed to the device", but NOTICE has more to do with the ui (host tool) build. We still have other stuff to fix regarding the mac resources stuff so @GrantComm if you want me to try tackling this instead that's fine, I was planning to rearrange macos build anyway.

EDIT: I'm fine with review/merging #673 or #662 to immediately address the issue of third party licenses not showing up in the mac build, but I think we should consider the above afterwards

@GrantComm
Copy link
Collaborator Author

This was just meant to immediately fix the third party licenses not showing up in the build. @hysw Is #662 almost ready for review? If so I can wait for that to go in and rebase.

@angela28chen I think putting the NOTICE file next to the host tool executables makes sense. I went ahead and created a bug for this an cc'd you.

@angela28chen
Copy link
Collaborator

Just FYI I'm working on #690 which should hopefully fix the copying over of files to the app bundle (and not need us to copy file by file), hopefully ready for review this week

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.

4 participants