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

Add dmgbuild as a dependency to improve mac dmg #2442

Merged

Conversation

ntindle
Copy link
Contributor

@ntindle ntindle commented Jun 6, 2024

Background

The current bdist_dmg command is limited in its functionality, to mainly the

  • naming the dmg
  • adding an app shortcut
  • doing it quietly

Changes

This PR

  • Moves bdist_dmg out of bdist_mac
  • Adds support for dmgbuild and its features such as backgrounds and layout customization

Related

Discussed in #2438 and closes #2438
Closes #2441

Copy link

codecov bot commented Jun 6, 2024

Codecov Report

Attention: Patch coverage is 94.47514% with 10 lines in your changes missing coverage. Please review.

Project coverage is 81.35%. Comparing base (4122946) to head (5befc25).
Report is 92 commits behind head on main.

Files with missing lines Patch % Lines
cx_Freeze/command/bdist_dmg.py 94.38% 10 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2442      +/-   ##
==========================================
+ Coverage   79.89%   81.35%   +1.45%     
==========================================
  Files          27       28       +1     
  Lines        4089     4231     +142     
==========================================
+ Hits         3267     3442     +175     
+ Misses        822      789      -33     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ntindle
Copy link
Contributor Author

ntindle commented Jun 7, 2024

There are a few paths forward to integrate dmgbuild.

For context, before we start discussing them, dmgbuild encourages a settings.py file similar to the setup.py cx_freeze uses.
Here is one of the settings.py files.
You can also use a settings.json. The downside to all of these options is there's not really a bindable interface for any of them unfortunately

  1. Make a settings.py file dynamically based on passed-in variables
  2. configure things via the command line
  3. Have the user make a settings.py and use that in combination with setup.py
  4. Use a settings.json settings file we build live
  5. call builddmg.core.build_dmg() directly, with parameters built from user_options in cx_Freeze

My intent is to try 5 -> 4 -> 1 -> 3 -> 2 and update as needed. Let me know if you have any opinions

@marcelotduarte
Copy link
Owner

I see dmbuild has a lot of options. Using settings.py has some advantages, such as using the Info.plist generated by bdist_mac (from what I saw on example), but it requires using another configuration file. We can think about how bdist_rpm was made. Rpm needs a .spec file and this file is generated by the options that the user passes, so the user does not need to deal with the .spec format.
On the other hand, cx_Freeze allows the user to use setup.py, command line and pyproject.toml, all directing the options to the chosen build/bdist command.
You can use a template to generate settings.py on the fly from the available options (which is probably an extension of what you thought of in 1). The way cx_Freeze is done, based on setuptools, you will need to think about the command line, and the setup.py and pyproject.toml options will automatically be available.

I believe that the finalize_options function can be built based on the load_json function.

@ntindle
Copy link
Contributor Author

ntindle commented Jun 11, 2024

For some of these options, like icon: https://dmgbuild.readthedocs.io/en/latest/settings.html#content-settings

Is this something better pulled from the bdist_mac config or should they be exposed here?

Current output:

 python setup.py bdist_dmg --help
Common commands: (see '--help-commands' for more)

  setup.py build      will build the package underneath 'build/'
  setup.py install    will install the package

Global options:
  --verbose (-v)  run verbosely (default)
  --quiet (-q)    run quietly (turns verbosity off)
  --dry-run (-n)  don't actually do anything
  --help (-h)     show detailed help message
  --no-user-cfg   ignore pydistutils.cfg in your home directory

Options for 'bdist_dmg' command:
  --volume-label           Volume label of the DMG disk image
  --applications-shortcut  Boolean for whether to include shortcut to
                           Applications in the DMG disk image
  --silent (-s)            suppress all output except warnings
  --format                 format of the disk image (default: UDZO)
  --filesystem             filesystem of the disk image (default: HFS+)
  --size                   If defined, specifies the size of the filesystem
                           within the image. If this is not defined, cx_Freeze
                           (and then dmgbuild) will attempt to determine a
                           reasonable size for the image. If you set this, you
                           should set it large enough to hold the files you
                           intend to copy into the image. The syntax is the
                           same as for the -size argument to hdiutil, i.e. you
                           can use the suffixes `b`, `k`, `m`, `g`, `t`, `p`
                           and `e` for bytes, kilobytes, megabytes, gigabytes,
                           terabytes, exabytes and petabytes respectively.
  --background (-b)        A rgb color in the form #3344ff, svg named color
                           like goldenrod, a path to an image, or the words
                           'builtin-arrow'
  --show-status-bar        Show the status bar in the Finder window. Default
                           is False.
  --show-tab-view          Show the tab view in the Finder window. Default is
                           False.
  --show-path-bar          Show the path bar in the Finder window. Default is
                           False.
  --show-sidebar           Show the sidebar in the Finder window. Default is
                           False.
  --sidebar-width          Width of the sidebar in the Finder window. Default
                           is None.
  --window-rect            Window rectangle in the form x,y,width,heightThe
                           position of the window in ((x, y), (w, h)) format,
                           with y co-ordinates running from bottom to top. The
                           Finder makes sure that the window will be on the
                           user's display, so if you want your window at the
                           top left of the display you could use (0, 100000)
                           as the x, y co-ordinates. Unfortunately it doesn't
                           appear to be possible to position the window
                           relative to the top left or relative to the centre
                           of the user's screen.
  --icon-locations         A dictionary specifying the co-ordinates of items
                           in the root directory of the disk image, where the
                           keys are filenames and the values are (x, y)
                           tuples. e.g.:icon-locations = { "Applications":
                           (100, 100), "README.txt": (200, 100) }
  --default-view           The default view of the Finder window. Possible
                           values are "icon-view", "list-view", "column-view",
                           "coverflow".
  --show-icon-preview      Show icon preview in the Finder window. Default is
                           False.
  --license                Dictionary specifying license details with 'default
                           -language', 'licenses', and 'buttons'.default-
                           language: Language code (e.g., 'en_US') if no
                           matching system language.licenses: Map of language
                           codes to license file paths (e.g., {'en_US':
                           'path/to/license_en.txt'}).buttons: Map of language
                           codes to UI strings ([language, agree, disagree,
                           print, save, instruction]).Example: {'default-
                           language': 'en_US', 'licenses': {'en_US':
                           'path/to/license_en.txt'}, 'buttons': {'en_US':
                           ['English', 'Agree', 'Disagree', 'Print', 'Save',
                           'Instruction text']}}

usage: setup.py [global_opts] cmd1 [cmd1_opts] [cmd2 [cmd2_opts] ...]
   or: setup.py --help [cmd1 cmd2 ...]
   or: setup.py --help-commands
   or: setup.py cmd --help

@marcelotduarte
Copy link
Owner

For some of these options, like icon: https://dmgbuild.readthedocs.io/en/latest/settings.html#content-settings

Is this something better pulled from the bdist_mac config or should they be exposed here?

It must be pulled from the bdist_mac configuration, but sometimes if the user does not realize this inheritance, it can cause confusion. So if possible inherit the configuration, but also expose it.
In the specific case of the icon, in bdist_appimage I use the icon specified in the Executable class, and if not, a default is used.
In bdist_mac, this is not done (I think the icon is ignored). Maybe we should fix it to standardize.

@ntindle
Copy link
Contributor Author

ntindle commented Jun 11, 2024

I’ll reuse the pattern from app image in that case

@ntindle
Copy link
Contributor Author

ntindle commented Jun 11, 2024

Still working on this, don’t worry :)

@ntindle
Copy link
Contributor Author

ntindle commented Jun 11, 2024

Is there a way to get Ruff to stop complaining about the commented-out code in dmg/setup.py and the type issue on self.distribution.executables in bdist_dmg.py?

Also can we change the CI to not always fail on failing to upload to codecov

@ntindle
Copy link
Contributor Author

ntindle commented Jun 11, 2024

image

image
Current status btw

@marcelotduarte
Copy link
Owner

marcelotduarte commented Jun 12, 2024

To ignore all violations across an entire file, add the line # ruff: noqa anywhere in the file, preferably towards the top, like so:
# ruff: noqa

Also can we change the CI to not always fail on failing to upload to codecov

# pragma: no cover

But, the real fail is:
https://github.com/marcelotduarte/cx_Freeze/actions/runs/9473801199/job/26102156665?pr=2442#step:6:436

@marcelotduarte
Copy link
Owner

Current status btw

It's getting really good, huh?

@ntindle
Copy link
Contributor Author

ntindle commented Jun 12, 2024

This one is probably a better example of what I was talking about. There’s likely failures in that last commit because it was my end of day commit. It triggered the ruff comment because it wouldn’t let me commit to the pr with issues without doing —no-verify

https://github.com/marcelotduarte/cx_Freeze/actions/runs/9472361645/job/26097601102

@marcelotduarte
Copy link
Owner

Earlier I modified a test exactly to see if it passed the tests. It was just to see if they passed the test. But you should ignore the comments for now.
There is also the option of putting it at the end of the commit message [ci skip]

@ntindle ntindle marked this pull request as ready for review June 13, 2024 00:18
@ntindle
Copy link
Contributor Author

ntindle commented Jun 13, 2024

Updated the code to pass the tests.
It looks like the failure now is the codecov lacking a token. I'll see if that's something I can run in my fork.
https://github.com/marcelotduarte/cx_Freeze/actions/runs/9491540029/job/26157164775?pr=2442#step:7:63

@ntindle
Copy link
Contributor Author

ntindle commented Jun 13, 2024

After this, I'll be working on licensing display for MSI if you have any resources to share 😄

@marcelotduarte
Copy link
Owner

It looks like the failure now is the codecov lacking a token. I'll see if that's something I can run in my fork.

The codecov/coverage is very interesting as it helps to resolve some errors. But it has this error, when a contributor makes more than one commit (apparently that's it), it gives this error, and I have to change the token.

@marcelotduarte marcelotduarte force-pushed the feat/macos-dmg-improvements branch from d7dd499 to 03a0495 Compare June 13, 2024 03:49
@marcelotduarte
Copy link
Owner

After a rebase, the tests pass, including coverage.

@ntindle
Copy link
Contributor Author

ntindle commented Jun 13, 2024

Awesome! Let me know any changes required to get merged, and I'll keep an eye on this 😄

@marcelotduarte
Copy link
Owner

I'll let you know later.
In another thread, you asked about things you can look at. For macOS, there are some issues that I can't check. Do you use any GUI (pyqt5/6, pyside2/6, etc)? Do you need to notarize, sign the executables?
Maybe you can look at some of these issues: 2212, 2180, 1228.

cx_Freeze/command/bdist_dmg.py Outdated Show resolved Hide resolved
tests/test_command_bdist_dmg.py Outdated Show resolved Hide resolved
tests/test_command_bdist_dmg.py Outdated Show resolved Hide resolved
@marcelotduarte marcelotduarte force-pushed the feat/macos-dmg-improvements branch from 03a0495 to d733510 Compare June 14, 2024 03:52
@ntindle
Copy link
Contributor Author

ntindle commented Jun 17, 2024

I can work on the changes requested here in a few hours. We currently don’t use a GUI and if we did so it may be a web ui. Not sure for now.

We will be doing code signing but I don’t have any certs on hand to do so yet

@ntindle ntindle requested a review from marcelotduarte June 18, 2024 00:20
@ntindle
Copy link
Contributor Author

ntindle commented Jun 18, 2024

Mentioned Issues for review:
We likely will take a look in a few months as we add internationalization to our system #2212,
I will be looking into #2180 when I get our signing certs from Apple,
As I understand, this behavior is expected on macOS and is the reason for PR #1228.

@marcelotduarte marcelotduarte force-pushed the feat/macos-dmg-improvements branch 2 times, most recently from 4d45919 to 2b2d8a9 Compare June 19, 2024 19:38
cx_Freeze/command/bdist_dmg.py Outdated Show resolved Hide resolved
cx_Freeze/command/bdist_dmg.py Outdated Show resolved Hide resolved
cx_Freeze/command/bdist_dmg.py Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
samples/dmg/setup.py Show resolved Hide resolved
@marcelotduarte marcelotduarte force-pushed the feat/macos-dmg-improvements branch from 2b2d8a9 to 4538183 Compare June 21, 2024 03:59
@marcelotduarte
Copy link
Owner

marcelotduarte commented Jun 27, 2024

You can test the PRs in the latest development build (dev31):
pip install --force --no-cache --pre --extra-index-url https://marcelotduarte.github.io/packages/ cx_Freeze

@marcelotduarte marcelotduarte force-pushed the feat/macos-dmg-improvements branch from 5b5edd3 to 5befc25 Compare July 9, 2024 04:11
@ntindle
Copy link
Contributor Author

ntindle commented Jul 10, 2024

I'll be going on vacation tomorrow, but I've already got tests working off my fork for our CI, so it shouldn't be a problem to wait. I would encourage more testing. I'll be back July 10 and can pick up from there anything needed

Anything needed here?

@marcelotduarte marcelotduarte merged commit 649e99a into marcelotduarte:main Jul 14, 2024
30 checks passed
@marcelotduarte
Copy link
Owner

Anything needed here?

spare time ;-)

Thanks!

@ntindle
Copy link
Contributor Author

ntindle commented Jul 14, 2024

Anything needed here?

spare time ;-)

Thanks!

I'll see if I can code a Time Machine up. No promises

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.

Extend DMG functionality by using an external library
2 participants