Improve and simplify macOS builds #1276

denizt · 2023-12-10T10:50:55Z

This series of commits aims to simplify the building of Veracrypt on macOS. This PR is currently draft and some commits require a rewrite as they are not atomic and require some fixes to the logic and the text.

Commit eca3086 needs to be rewritten with the packages location that Veracrypt internally uses, the public version in this commit is outdated
We need to make sure it's clear in the docs that homebrew instructions are only for development purposes

We need packages for the last build step on macOS, update docs to reflect the requirement.

On macOS, we can use a package manager to easily install dependencies. This simplifies onboarding and building Veracrypt.

When building, we can use prebuilt wxwidgets from homebrew to simplify and speed up local building. We also put the package behind a flag as it's optional during development.

When building with homebrew, skip signing. This can be put behind a flag to enable, if needed.

The binary in the repo is not universal (x86_64) and therefore building fails on arm architecture if Rosetta is not installed. Use local yasm if available.

When building via homebrew and locally, build only the local arch which skips ASM for arm(Mx) for MacOS. This removes the need to have rosetta installed for building.

Jertzukka · 2023-12-11T04:33:29Z

src/Makefile

@@ -350,8 +352,8 @@ ifeq "$(shell uname -s)" "Darwin"
 		S := $(C_CXX_FLAGS)
 		C_CXX_FLAGS = $(subst -MMD,,$(S))

-		C_CXX_FLAGS += -gfull -arch x86_64


At the beginning of Makefile CPU_ARCH is set to x64 based on my uname -m of x86_64, later CPU_ARCH is set to x86 based on CPU_ARCH being x64. This means that here it'll attempt to build with arch x86 which my Monterey doesn't have. This will error out with clang: error: invalid arch name '-arch x86'

Thank you so much for this @Jertzukka! I hadn't tested it on x86 and thought it could fail. I have now tried it on x86 and fixed all the problems with local builds using brew packages. I would appreciate it if you could confirm it on your end 😃

Jertzukka · 2023-12-11T04:44:26Z

src/Makefile

@@ -295,7 +295,7 @@ ifeq "$(shell uname -s)" "Darwin"
 	PLATFORM := MacOSX
 	APPNAME := VeraCrypt

-	export VC_OSX_TARGET ?= 10.7
+	export VC_OSX_TARGET ?= 10.9
 	export VC_OSX_SDK ?= $(VC_OSX_TARGET)


The VC_OSX_TARGET could probably be bumped upto 12 in both places, as VC version 1.25.9 was the last one that officially supported anything below that. The xcrun --show-sdk-version trick could be used here to guess the correct SDK, instead of assuming it is the same as target.

I fixed this one in a follow-up commit and bumped it to 12, to be on the side of err, as that was the target in the main script.

Jertzukka · 2023-12-11T05:05:01Z

src/Volume/Volume.make

@@ -37,6 +37,7 @@ endif

 ifeq "$(ENABLE_WOLFCRYPT)" "0"
 ifeq "$(PLATFORM)" "MacOSX"
+ifeq "$(COMPILE_ASM)" "true"


COMPILE_ASM variable needs to be also set somewhere else than only in build_veracrypt_macosx.sh, like in Makefile, or else normal make will not work properly as it is not defined without this build script.

Thanks for the review @Jertzukka!

Good call and find. I addressed this in 3e5168f, moving the check toifneq instead ifeq so the undefined variable doesn't fail the build. I'll try to follow up on the other issues you mentioned tomorrow.

Use a conditional check for COMPILE_ASM not being false instead of true. This avoids passing the variable to other parts of the build script.

Align the requirement to SDK 12 in both the makefile and script, and update the comment to remove confusion. I chose to leave this on 12 to be on the side of err and support as many building platforms as possible, when we can support. The local script now also sets the target using the local sdk version. This should improve the local development experience.

We now build only the current arch for local development builds in macOS. This change also fixes the x86 builds failing.

Flags to build a local build using homebrew packages are not default and require parameter -b to build. We also don't build packages directly, which requires -p.

denizt · 2023-12-16T11:24:43Z

@idrassi Can you please share/upload the packages build that is used during the build of Veracypt, I believe the one on the webpage and homebrew is outdated and hosted on free.fr (HTTP).

I would also appreciate it if you could review the PR and have comments so I can wrap this up this weekend 😃

Thanks!

vit9696 · 2023-12-31T11:28:47Z

@denizt, given the build changes, perhaps you could also introduce a switch to use a fuse-t implementation instead of macFUSE (ref #1055).

Jertzukka · 2024-01-03T07:45:14Z

src/Main/Main.make

@@ -210,7 +210,9 @@ else
 	sed -e 's/_VERSION_/$(patsubst %a,%.1,$(patsubst %b,%.2,$(TC_VERSION)))/' ../Build/Resources/MacOSX/Info.plist.xml >$(APPNAME).app/Contents/Info.plist
 endif
 	chmod -R go-w $(APPNAME).app
+ifneq ($(LOCAL_DEVELOPMENT_BUILD),"true")


Suggested change

ifneq ($(LOCAL_DEVELOPMENT_BUILD),"true")

ifneq ("$(LOCAL_DEVELOPMENT_BUILD)","true")

This needs to be quoted to work. Also I might've chosen something shorter like NOSIGN which makes it also clear what the flag does, just skips the package sign parts.

Also doesn't the productsign right under this also need to be turned off for dev build as it will also fail with missing certificates?

If we do not sign, the installer name in the later part will have wrong name that does not exist:

https://github.com/veracrypt/VeraCrypt/blob/6e28375060e043e9039bac4d292ecbcc5e94b08d/src/Main/Main.make#L233C2-L233C2

So, for no signing path, the copied installer name should be "$(BASE_DIR)/Setup/MacOSX/VeraCrypt $(TC_VERSION).pkg" instead of "$(BASE_DIR)/Setup/MacOSX/VeraCrypt_$(TC_VERSION).pkg"

Thanks! I'll have a look at this (and possibly others) once the maintainer reviews

denizt · 2024-02-24T22:48:10Z

@denizt, given the build changes, perhaps you could also introduce a switch to use a fuse-t implementation instead of macFUSE (ref #1055).

I can do that in a follow up commit, this PR is unfortunately stalled and I would like to wrap this up first 😄

@idrassi Can you please review?

denizt added 6 commits December 10, 2023 16:14

Add missing macOS requirement for 'make package'

eca3086

We need packages for the last build step on macOS, update docs to reflect the requirement.

Add build instructions using homebrew

c549449

On macOS, we can use a package manager to easily install dependencies. This simplifies onboarding and building Veracrypt.

Add flag to use homebrew packages

ad431d9

When building, we can use prebuilt wxwidgets from homebrew to simplify and speed up local building. We also put the package behind a flag as it's optional during development.

Skip signing for local builds

f3ff46b

When building with homebrew, skip signing. This can be put behind a flag to enable, if needed.

Use system yasm on macOS if available

0f6075e

The binary in the repo is not universal (x86_64) and therefore building fails on arm architecture if Rosetta is not installed. Use local yasm if available.

Build local arch only in development

ef75034

When building via homebrew and locally, build only the local arch which skips ASM for arm(Mx) for MacOS. This removes the need to have rosetta installed for building.

Jertzukka reviewed Dec 11, 2023

View reviewed changes

Fix compilation issue when COMPILE_ASM is undefined

3e5168f

Use a conditional check for COMPILE_ASM not being false instead of true. This avoids passing the variable to other parts of the build script.

denizt marked this pull request as draft December 12, 2023 02:46

denizt added 5 commits December 16, 2023 19:12

Fix wrong architecture for macOS in x86 builds

1bdc559

We now build only the current arch for local development builds in macOS. This change also fixes the x86 builds failing.

Add instructions brew backed macOS local builds

0616324

Flags to build a local build using homebrew packages are not default and require parameter -b to build. We also don't build packages directly, which requires -p.

Fix wxwidgets not linking in local x86 macOS development builds

5c4c75d

Clarify build location in the document

8506cc0

Jertzukka reviewed Jan 3, 2024

View reviewed changes

denizt marked this pull request as ready for review February 24, 2024 22:45

ZelnickB mentioned this pull request May 4, 2024

For Mac, have the option to use FUSE-T #1055

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve and simplify macOS builds #1276

Improve and simplify macOS builds #1276

denizt commented Dec 10, 2023

Jertzukka Dec 11, 2023

denizt Dec 16, 2023

Jertzukka Dec 11, 2023

denizt Dec 16, 2023

Jertzukka Dec 11, 2023

denizt Dec 11, 2023 •

edited

denizt commented Dec 16, 2023

vit9696 commented Dec 31, 2023

Jertzukka Jan 3, 2024 •

edited

Jertzukka Jan 3, 2024

denizt Feb 24, 2024

denizt commented Feb 24, 2024

	ifneq ($(LOCAL_DEVELOPMENT_BUILD),"true")
	ifneq ("$(LOCAL_DEVELOPMENT_BUILD)","true")

Improve and simplify macOS builds #1276

Are you sure you want to change the base?

Improve and simplify macOS builds #1276

Conversation

denizt commented Dec 10, 2023

Jertzukka Dec 11, 2023

Choose a reason for hiding this comment

denizt Dec 16, 2023

Choose a reason for hiding this comment

Jertzukka Dec 11, 2023

Choose a reason for hiding this comment

denizt Dec 16, 2023

Choose a reason for hiding this comment

Jertzukka Dec 11, 2023

Choose a reason for hiding this comment

denizt Dec 11, 2023 • edited

Choose a reason for hiding this comment

denizt commented Dec 16, 2023

vit9696 commented Dec 31, 2023

Jertzukka Jan 3, 2024 • edited

Choose a reason for hiding this comment

Jertzukka Jan 3, 2024

Choose a reason for hiding this comment

denizt Feb 24, 2024

Choose a reason for hiding this comment

denizt commented Feb 24, 2024

denizt Dec 11, 2023 •

edited

Jertzukka Jan 3, 2024 •

edited