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

Build macOS releases #2198

Merged
merged 11 commits into from May 24, 2021
Merged

Build macOS releases #2198

merged 11 commits into from May 24, 2021

Conversation

disrupted
Copy link
Contributor

This is an addition to the existing CI releases for Linux and Windows, adding support for self-contained macOS builds.

The pipeline run can be inspected here: https://github.com/disrupted/black/runs/2504846870

The result was uploaded here: https://github.com/disrupted/black/releases/tag/21.5b2

With those three targets in place I believe it would make sense to add the target names to the binaries to avoid ambiguity. I was thinking something like this:

  • black_macos
  • black_ubuntu.elf
  • black_windows.exe

@ichard26 ichard26 requested a review from cooperlees May 4, 2021 23:08
@ichard26
Copy link
Collaborator

ichard26 commented May 4, 2021

Hi @disrupted,

Thank you for the PR, it's real nice to improve the ease of using Black on various platforms. I'll review but expect this to be reviewed by someone else as well. I'll also approve the workflows to run, although it's really the lint job that matters.

Welcome to the community!

Also FYI you'll need to add a changelog entry for this enhancement. This is definitely something we want to advertise a little bit. You can read more here. Use the PR number already assigned, you don't need to use Next PR Number.


@cooperlees can you test and review this PR? I can check that the GHA configuration makes sense but ultimately I don't have a MacOS running machine to test on. Thank you in advance!

@ichard26 ichard26 added C: packaging Installation and packaging of Black T: enhancement New feature or request labels May 4, 2021
@disrupted
Copy link
Contributor Author

@ichard26 thanks for your feedback and warm welcome! I'll have a look at addressing the changelog tomorrow.

@cooperlees
Copy link
Collaborator

My main point here before testing is this is only going to be an x86_64 binary right. So should we name it that?

Does GitHub Action have the ability for a arm64 build for Apple Silicon yet? Will Apple Silicon just run the x86_64 binary with some emulator? I'll try build one and test. Luckily I have x86_64 + a arm64 Mac.

@cooperlees
Copy link
Collaborator

cooperlees commented May 5, 2021

The executable is by default is x86_64, even when building on my M1 Mac:

(tb) crl-m1:black cooper$ python -m PyInstaller -F --name black --add-data 'src/blib2to3:blib2to3' src/black/__main__.py
...
(tb) crl-m1:black cooper$ file dist/black
dist/black: Mach-O 64-bit executable x86_64
(tb) crl-m1:black cooper$ ls -lh dist/black
-rwxr-xr-x  1 cooper  staff   6.9M May  5 08:20 dist/black
(tb) crl-m1:black cooper$ dist/black --help
[18978] Error loading Python lib '/var/folders/y8/jx32nm1572v7qwnmgyk0gd7m0000gn/T/_MEIlVwWXx/Python': dlopen: dlopen(/var/folders/y8/jx32nm1572v7qwnmgyk0gd7m0000gn/T/_MEIlVwWXx/Python, 10): no suitable image found.  Did find:
	/var/folders/y8/jx32nm1572v7qwnmgyk0gd7m0000gn/T/_MEIlVwWXx/Python: mach-o, but wrong architecture
	/private/var/folders/y8/jx32nm1572v7qwnmgyk0gd7m0000gn/T/_MEIlVwWXx/Python: mach-o, but wrong architecture

The Action by default also seems to use PyInstaller 4.3 and all the talk in pyinstaller/pyinstaller#5315 has people using 5.0 dev versions + some settings.

This command from a member has the env + runs-on tweaks it seems we need to produce universal2 MacOS Binaries that run on both arm64 + x86_64. I think we should do this if we're going to support MacOS, let's do it for all. Thoughts?

Copy link
Collaborator

@cooperlees cooperlees left a comment

Choose a reason for hiding this comment

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

Lets support arm64 + x86_64 for the binary making a universal2 MacOS Binary.

@disrupted disrupted marked this pull request as draft May 5, 2021 21:42
@ichard26 ichard26 self-requested a review May 6, 2021 00:24
@disrupted
Copy link
Contributor Author

The macOS 11 runner seems to be down at the moment.

@cooperlees
Copy link
Collaborator

Bummer. Sorry to waste your time. I didn't mean to do that!

If this is too bleeding and does not work, I am happy to start with x86_64 only. Let's just state that somewhere in the docs and once we can lets move to a Universal binary rather than letting this great PR sit here and rot. Sound good?

@ichard26
Copy link
Collaborator

ichard26 commented May 6, 2021

Just found this lovely /s bit of information:

Note: The macOS 11.0 virtual environment is currently provided as a private preview only. Any users or organizations that are already using this runner can continue using it, but we're not accepting any further users or organizations at this time. The macos-latest YAML workflow label still uses the macOS 10.15 virtual environment.

https://docs.github.com/en/actions/using-github-hosted-runners/about-github-hosted-runners#supported-runners-and-hardware-resources

So yeah, I agree with Cooper, let's get x86-64 working for now. I guess while at it, let's rename the binaries to indicate the arch and os. Old me thought the file extensions would've been enough but sadly not.

@disrupted
Copy link
Contributor Author

Agreed! For now the process is rather hacky anyways and I am not too keen about adding a whole bunch of custom logic for the universal Mac build to the CI script. So I believe it's for the best to wait until PyInstaller 5.0 is stable and released. Hopefully by then the process of building universal x86/ARM binary will be a bit more straightforward.

@ichard26
Copy link
Collaborator

@disrupted just for clarity, what's the plan here? Are we waiting for PyInstaller 5 and the macOS 11.0 virtual environment to land so we can built universal binaries? Or are we going down the route Cooper suggested of just producing x86-64 macOS binaries for the time being?

@disrupted
Copy link
Contributor Author

disrupted commented May 16, 2021

@ichard26 I'd say we can follow through as suggested to build x86 binaries for now and add ARM/universal binary support at a later point. This is almost finished btw, just gotta do the part about renaming the release artifacts and update the changelog. I was a bit distracted with some other stuff recently but hope I can finish it up soon. (Marked as a draft in the meantime)

@disrupted disrupted marked this pull request as ready for review May 23, 2021 11:45
@disrupted disrupted requested a review from cooperlees May 23, 2021 11:46
Copy link
Collaborator

@cooperlees cooperlees left a comment

Choose a reason for hiding this comment

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

Thanks.

I'll open 2 issues here. Build a universal2 Mac Binary + see if we can move this exe builds to Python 3.9

@@ -13,6 +13,10 @@
- Add a lower bound for the `aiohttp-cors` dependency. Only 0.4.0 or higher is
supported. (#2231)

### _Packaging_

- Release self-contained macOS binaries as part of the GitHub release pipeline (#2198)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll edit when I release - Will state it's only x86_64 arch.

@ichard26
Copy link
Collaborator

Thank you so much for your contribution! This project is only possible by contributions like these 🖤. You're awesome, @disrupted! I couldn't do this myself when I first added this workflow due to me not owning an machine to test on. This will definitely make it easier for our users to use Black. Also congrats on your first PR here and for the PSF! Finally, I'm actually in the process of improving the contributing process so I'd love to hear your feedback, if you can spare some time checkout GH-2238 for details. Thank you once again!

@ichard26 ichard26 mentioned this pull request May 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: packaging Installation and packaging of Black T: enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants